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

[Yosry Ahmed <yosryahmed@google.com>]

On Tue, Mar 14, 2023 at 3:02 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Mon 13-03-23 14:08:53, 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));
> > }
>
> I would just stick with the existing code and put a comment that this
> function shouldn't be used in any new code and the folio counterpart
> should be used instead.

Would you mind explaining the rationale?

If existing users are not passing in tail pages and we are telling new
users not to use it, what's the point of leaving the PageTail() check?

Is page owner doing the right thing by discounting pages with
page->memcg_data = 0 in print_page_owner_memcg() ? Wouldn't this not
show the memcg of tail pages?

If page owner also needs a compound_head()/ page_folio() call before
checking the page memcg, perhaps we should convert both call sites to
page_memcg_check() to folio_memcg_check() and remove it now?

>
> --
> Michal Hocko
> SUSE Labs