Re: [PATCH] memcg: page_cgroup_ino() get memcg from compound_head(page)

[Yosry Ahmed <yosryahmed@google.com>]

On Tue, Mar 14, 2023 at 7:34 PM Roman Gushchin <roman.gushchin@linux.dev> wrote:
>
> On Mon, Mar 13, 2023 at 02:08:53PM -0700, Yosry Ahmed wrote:
> > On Mon, Mar 13, 2023 at 12:44 PM Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> > >
> > > On Mon, 13 Mar 2023 08:34:52 +0000 Yosry Ahmed <yosryahmed@google.com> wrote:
> > >
> > > > From: Hugh Dickins <hughd@google.com>
> > > >
> > > > In a kernel with added WARN_ON_ONCE(PageTail) in page_memcg_check(), we
> > > > observed a warning from page_cgroup_ino() when reading
> > > > /proc/kpagecgroup.
> > >
> > > If this is the only known situation in which page_memcg_check() is
> > > passed a tail page, why does page_memcg_check() have
> > >
> > >         if (PageTail(page))
> > >                 return NULL;
> > >
> > > ?  Can we remove this to simplify, streamline and clarify?
> >
> > I guess it's a safety check so that we don't end up trying to cast a
> > tail page to a folio. My opinion is to go one step further and change
> > page_memcg_check() to do return the memcg of the head page, i.e:
> >
> > static inline struct mem_cgroup *page_memcg_check(struct page *page)
> > {
> >     return folio_memcg_check(page_folio(page));
> > }
> >
> > This makes it consistent with page_memcg(), and makes sure future
> > users are getting the "correct" memcg for whatever page they pass in.
> > I am interested to hear other folks' opinions here.
> >
> > The only other user today is print_page_owner_memcg(). I am not sure
> > if it's doing the right thing by explicitly reading page->memcg_data,
> > but it is already excluding pages that have page->memcg_data == 0,
> > which should be the case for tail pages.
>
> Yeah, I think it's a good idea. I'd do this.
> If you'll master a patch like this, please, apply my
> Acked-by: Roman Gushchin <roman.gushchin@linux.dev> .

Thanks, Roman. So far there have been different opinions (specifically
Michal's), so I'll wait until the discussion settles before sending
another patch.

>
> I'm ok with the current approach too (the one you posted),
> but the one above is preferable.
>
> Overall it would be nice to clarify & document our expectations
> from /proc/kpagecgroup (and /proc/kpageflags & /proc/kpagecount)
> in the new folio "epoch".

Agreed, though I don't think I am the right person for this.


>
> Thanks!