Re: [PATCH 57/62] memcg: Convert object cgroups from struct page to struct slab

[Matthew Wilcox <willy@infradead.org>]

On Mon, Oct 11, 2021 at 01:13:18PM -0400, Johannes Weiner wrote:
> Because right now we can have user pages pointing to a memcg, random
> alloc_page(GFP_ACCOUNT) pages pointing to an objcg, and slab pages
> pointing to an array of objcgs - all in the same memcg_data member.

Ah!  I was missing the possibility that an alloc_page() could point to
an objcg.  I had thought that only slab pages could point to an objcg
and only anon/file pages could point to a memcg.

> After your patch, slab->memcg_data points to an array of objcgs,
> period. The only time it doesn't is when there is a bug. Once the
> memcg_data member is no longer physically shared between page and
> slab, we can do:
> 
> 	struct slab {
> 		struct obj_cgroup **objcgs;
> 	};
> 
> and ditch the accessor function altogether.

Yes.

> > - * page_objcgs_check - get the object cgroups vector associated with a page
> > - * @page: a pointer to the page struct
> > + * slab_objcgs_check - get the object cgroups vector associated with a page
> > + * @slab: a pointer to the slab struct
> >   *
> > - * Returns a pointer to the object cgroups vector associated with the page,
> > - * or NULL. This function is safe to use if the page can be directly associated
> > + * Returns a pointer to the object cgroups vector associated with the slab,
> > + * or NULL. This function is safe to use if the slab can be directly associated
> >   * with a memory cgroup.
> >   */
> > -static inline struct obj_cgroup **page_objcgs_check(struct page *page)
> > +static inline struct obj_cgroup **slab_objcgs_check(struct slab *slab)
> >  {
> > -	unsigned long memcg_data = READ_ONCE(page->memcg_data);
> > +	unsigned long memcg_data = READ_ONCE(slab->memcg_data);
> >  
> >  	if (!memcg_data || !(memcg_data & MEMCG_DATA_OBJCGS))
> >  		return NULL;
> >  
> > -	VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_KMEM, page);
> > +	VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_KMEM, slab_page(slab));
> >  
> >  	return (struct obj_cgroup **)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
> 
> This is a bit weird.
> 
> The function is used in one place, to check whether a random page is a
> slab page. It's essentially a generic type check on the page!
> 
> After your changes, you pass a struct slab that might well be invalid
> if this isn't a slab page, and you rely on the PAGE's memcg_data to
> tell you whether this is the case. It works because page->memcg_data
> is overlaid with slab->memcg_data, but that won't be the case if we
> allocate struct slab separately.
> 
> To avoid that trap down the road, I think it would be better to keep
> the *page* the ambiguous object for now, and only resolve to struct
> slab after the type check. So that every time you see struct slab, you
> know it's valid.
> 
> In fact, I think it would be best to just inline page_objcgs_check()
> into its sole caller. It would clarify the resolution from wildcard
> page to valid struct slab quite a bit:

Yes.  Every time I read through this, I was wondering if there was
something I was missing.  I mean, there was (the memcg/objcg/objcgs
distinction above), but yes, if we know we have a slab, we don't need
this function.

> > @@ -2819,38 +2819,39 @@ int memcg_alloc_page_obj_cgroups(struct page *page, struct kmem_cache *s,
> >   */
> >  struct mem_cgroup *mem_cgroup_from_obj(void *p)
> >  {
> > -	struct page *page;
> > +	struct slab *slab;
> >  
> >  	if (mem_cgroup_disabled())
> >  		return NULL;
> >  
> > -	page = virt_to_head_page(p);
> > +	slab = virt_to_slab(p);
> >  
> >  	/*
> >  	 * Slab objects are accounted individually, not per-page.
> >  	 * Memcg membership data for each individual object is saved in
> > -	 * the page->obj_cgroups.
> > +	 * the slab->obj_cgroups.
> >  	 */
> > -	if (page_objcgs_check(page)) {
> > +	if (slab_objcgs_check(slab)) {
> 
> I.e. do this instead:
> 
> 	page = virt_to_head_page(p);
> 
> 	/* object is backed by slab */
> 	if (page->memcg_data & MEMCG_DATA_OBJCGS) {
> 		struct slab *slab = (struct slab *)page;
> 
> 		objcg = slab_objcgs(...)[]
> 		return objcg ? obj_cgroup_memcg(objcg): NULL;
> 	}
> 
> 	/* object is backed by a regular kernel page */
> 	return page_memcg_check(page);

Maybe I'm missing something else, but why not discriminate based on
PageSlab()?  ie:

	slab = virt_to_slab(p);
	if (slab_test_cache(slab)) {
		...
	}
	return page_memcg_check((struct page *)slab);

... but see the response to your other email for why not exactly this.