Re: [PATCH v4 14/16] memcg: Convert mem_cgroup_from_obj_folio() to mem_cgroup_from_obj_slab()

[Harry Yoo <harry.yoo@oracle.com>]

On Thu, Nov 13, 2025 at 04:39:41PM +0000, Matthew Wilcox wrote:
> On Thu, Nov 13, 2025 at 11:14:24AM -0500, Johannes Weiner wrote:
> > On Thu, Nov 13, 2025 at 12:09:28AM +0000, Matthew Wilcox (Oracle) wrote:
> > > -	/*
> > > -	 * folio_memcg_check() is used here, because in theory we can encounter
> > > -	 * a folio where the slab flag has been cleared already, but
> > > -	 * slab->obj_exts has not been freed yet
> > > -	 * folio_memcg_check() will guarantee that a proper memory
> > > -	 * cgroup pointer or NULL will be returned.
> > > -	 */
> > > -	return folio_memcg_check(folio);
> > > +	off = obj_to_index(slab->slab_cache, slab, p);
> > > +	if (obj_exts[off].objcg)
> > > +		return obj_cgroup_memcg(obj_exts[off].objcg);
> > > +
> > > +	return NULL;
> > >  }
> > >  
> > >  /*
> > > @@ -2637,7 +2627,7 @@ struct mem_cgroup *mem_cgroup_from_slab_obj(void *p)
> > >  	if (mem_cgroup_disabled())
> > >  		return NULL;
> > >  
> > > -	return mem_cgroup_from_obj_folio(virt_to_folio(p), p);
> > > +	return mem_cgroup_from_obj_slab(virt_to_slab(p), p);
> > 
> > The name undoubtedly sucks, but there is a comment above this function
> > that this can be used on non-slab kernel pages as well.
> 
> Oh, I see.  Usercopy calls this kind of thing 'heap object', so
> perhaps eventually rename this function to mem_cgroup_from_heap_obj()?
> 
> > E.g. !vmap kernel stack pages -> mod_lruvec_kmem_state -> mem_cgroup_from_obj_slab
> 
> That actually seems to be the only user ... and I wasn't testing on a
> !VMAP_STACK build, so I wouldn't've caught it.
> 
> > How about:
> > 
> > 	if ((slab = virt_to_slap(p)))
> > 		return mem_cgroup_from_obj_slab(slab, p);
> > 	return folio_memcg_check(virt_to_folio(p), p);
> 
> Mild updates, here's my counteroffer:
> 
> commit 6ca8243530e4
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> Date:   Thu Nov 13 11:30:59 2025 -0500
> 
>     fix-memcg-slab

With this folded,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry / Hyeonggon

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 597673e272a9..a2e6f409c5e8 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2599,9 +2599,6 @@ struct mem_cgroup *mem_cgroup_from_obj_slab(struct slab *slab, void *p)
>  	struct slabobj_ext *obj_exts;
>  	unsigned int off;
>  
> -	if (!slab)
> -		return NULL;
> -
>  	obj_exts = slab_obj_exts(slab);
>  	if (!obj_exts)
>  		return NULL;
> @@ -2624,10 +2621,15 @@ struct mem_cgroup *mem_cgroup_from_obj_slab(struct slab *slab, void *p)
>   */
>  struct mem_cgroup *mem_cgroup_from_slab_obj(void *p)
>  {
> +	struct slab *slab;
> +
>  	if (mem_cgroup_disabled())
>  		return NULL;
>  
> -	return mem_cgroup_from_obj_slab(virt_to_slab(p), p);
> +	slab = virt_to_slab(p);
> +	if (slab)
> +		return mem_cgroup_from_obj_slab(slab, p);
> +	return folio_memcg_check(virt_to_folio(p));
>  }
>  
>  static struct obj_cgroup *__get_obj_cgroup_from_memcg(struct mem_cgroup *memcg)