Re: [PATCH RFC 05/12] mm/slab: abstract slabobj_ext.objcg access

On 7/15/26 12:10, Vlastimil Babka (SUSE) wrote:
> In preparation for changes to the structure, abstract access to the
> objcg field with a slab_obj_ext_objcgp() function.
> Rename the field to _objcg to make an unexpected direct access a compile
> error.
> 
> No functional change intended.
> 
> Signed-off-by: Vlastimil Babka (SUSE) <[email protected]>

Hm sashiko finds [1] that this is breaks tools/mm/show_page_info.py

But I think it was already buggy. get_memcg_info() is supposed to return
memcg info for a page but in case of MEMCG_DATA_OBJEXTS there's no such info
for the whole page, there is only for individual slab objects and it doesn't
know which object so it effectively gets the first one. I think it should
just stop handling MEMCG_DATA_OBJEXTS, but that's a fix that should be sent
unrelated to this series.

There's also tools/cgroup/memcg_slabinfo.py and that I think has been
already broken with memalloc profiling as it assuems slab.memcg_data is
still a raw struct obj_cgroup * and not struct slabobj_ext.

I think it will be easier to fix it at once to handle the layout after this
series than trying to fix it first to handle the pre-series layout and then
update it with every relevant change.

[1]
https://sashiko.dev/#/patchset/[email protected]?part=5

> ---
>  mm/kfence/core.c |  2 +-
>  mm/memcontrol.c  | 23 +++++++++++++++--------
>  mm/slab.h        |  9 ++++++++-
>  mm/slub.c        |  2 +-
>  4 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 6577bd76954e..717e8baf7e5d 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -1249,7 +1249,7 @@ void __kfence_free(void *addr)
>  	struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
>  
>  #ifdef CONFIG_MEMCG
> -	KFENCE_WARN_ON(meta->obj_exts.objcg);
> +	KFENCE_WARN_ON(*slab_obj_ext_objcgp(&meta->obj_exts));
>  #endif
>  	/*
>  	 * If the objects of the cache are SLAB_TYPESAFE_BY_RCU, defer freeing
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4e427286a88a..6303a2b1a9d0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2865,6 +2865,7 @@ struct mem_cgroup *mem_cgroup_from_obj_slab(struct slab *slab, void *p)
>  	 */
>  	unsigned long obj_exts;
>  	struct slabobj_ext *obj_ext;
> +	struct obj_cgroup *objcg;
>  
>  	obj_exts = slab_obj_exts(slab);
>  	if (!obj_exts)
> @@ -2872,9 +2873,8 @@ struct mem_cgroup *mem_cgroup_from_obj_slab(struct slab *slab, void *p)
>  
>  	get_slab_obj_exts(obj_exts);
>  	obj_ext = slab_obj_ext(slab->slab_cache, slab, obj_exts, p);
> -	if (obj_ext->objcg) {
> -		struct obj_cgroup *objcg = obj_ext->objcg;
> -
> +	objcg = *slab_obj_ext_objcgp(obj_ext);
> +	if (objcg) {
>  		put_slab_obj_exts(obj_exts);
>  		return obj_cgroup_memcg(objcg);
>  	}
> @@ -3577,6 +3577,7 @@ bool __memcg_slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
>  		unsigned long obj_exts;
>  		struct slabobj_ext *obj_ext;
>  		struct obj_stock_pcp *stock;
> +		struct obj_cgroup **objcgp;
>  
>  		slab = virt_to_slab(p[i]);
>  
> @@ -3612,10 +3613,15 @@ bool __memcg_slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
>  		unlock_stock(stock);
>  
>  		obj_exts = slab_obj_exts(slab);
> +
>  		get_slab_obj_exts(obj_exts);
> +
>  		obj_ext = slab_obj_ext(s, slab, obj_exts, p[i]);
> +		objcgp = slab_obj_ext_objcgp(obj_ext);
> +
>  		obj_cgroup_get(objcg);
> -		obj_ext->objcg = objcg;
> +		*objcgp = objcg;
> +
>  		put_slab_obj_exts(obj_exts);
>  	}
>  
> @@ -3628,16 +3634,17 @@ void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
>  	size_t obj_size = obj_full_size(s);
>  
>  	for (int i = 0; i < objects; i++) {
> -		struct obj_cgroup *objcg;
> +		struct obj_cgroup **objcgp, *objcg;
>  		struct slabobj_ext *obj_ext;
>  		struct obj_stock_pcp *stock;
>  
>  		obj_ext = slab_obj_ext(s, slab, obj_exts, p[i]);
> -		objcg = obj_ext->objcg;
> -		if (!objcg)
> +		objcgp = slab_obj_ext_objcgp(obj_ext);
> +		if (!*objcgp)
>  			continue;
>  
> -		obj_ext->objcg = NULL;
> +		objcg = *objcgp;
> +		*objcgp = NULL;
>  
>  		stock = trylock_stock();
>  		__refill_obj_stock(objcg, stock, obj_size, true);
> diff --git a/mm/slab.h b/mm/slab.h
> index 36d067d6e7c0..789bd292075f 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -555,7 +555,7 @@ static inline bool need_kmalloc_no_objext(void)
>   */
>  struct slabobj_ext {
>  #ifdef CONFIG_MEMCG
> -	struct obj_cgroup *objcg;
> +	struct obj_cgroup *_objcg;
>  #endif
>  #ifdef CONFIG_MEM_ALLOC_PROFILING
>  	union codetag_ref ref;
> @@ -661,6 +661,13 @@ slab_obj_ext(struct kmem_cache *s, struct slab *slab, unsigned long obj_exts,
>  	return kasan_reset_tag(obj_ext);
>  }
>  
> +#ifdef CONFIG_MEMCG
> +static inline struct obj_cgroup **slab_obj_ext_objcgp(struct slabobj_ext *obj_ext)
> +{
> +	return &obj_ext->_objcg;
> +}
> +#endif
> +
>  int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
>  			gfp_t gfp, unsigned int alloc_flags);
>  
> diff --git a/mm/slub.c b/mm/slub.c
> index 5e3f53bcd0d3..48e10198a3ce 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2523,7 +2523,7 @@ bool memcg_slab_post_charge(void *p, gfp_t flags)
>  	if (obj_exts) {
>  		get_slab_obj_exts(obj_exts);
>  		obj_ext = slab_obj_ext(s, slab, obj_exts, p);
> -		if (unlikely(obj_ext->objcg)) {
> +		if (unlikely(*slab_obj_ext_objcgp(obj_ext))) {
>  			put_slab_obj_exts(obj_exts);
>  			return true;
>  		}
>