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

[Matthew Wilcox <willy@infradead.org>]

On Wed, Mar 15, 2023 at 12:04:10AM -0700, Yosry Ahmed wrote:
> On Tue, Mar 14, 2023 at 9:54 PM Matthew Wilcox <willy@infradead.org> 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));
> > > }
> >
> > If you look at my commit becacb04fdd4, I was preserving the existing
> > behaviour of page_memcg_check() when passed a tail page.  It would
> > previously, rightly or wrongly, read the memcg_data from the tail page
> > and get back NULL.
> 
> Right, I looked at that. I also looked at 1b7e4464d43a which added
> folio_memcg() and changed page_memcg()'s behavior to use page_folio()
> to retrieve the memcg from the head, which made me wonder why
> different decisions were made for these 2 helpers.
> 
> Were the users of page_memcg() already passing in head pages only?

There were 18 months between those commits ... I'd learned to be
more careful about maintaining the semantics instead of changing
them to "what they should have been".

> >
> > I suspect that was not the intended behaviour, but I do not think this
> > patch is the right fix; it simply papers over the problem and maybe
> > creates a new one.  Callers of page_memcg_check() should be eliminated,
> > precisely because of this ambiguity.  It's up to the people who understand
> > each of the callers who need to make the decision to always convert the
> > page that they have to a folio and ask about its memcg, or whether they
> > want to preserve the existing behaviour of returning NULL for tail pages.
> >
> > So, I say NACK to this patch as it does not preserve existing behaviour,
> > and does not advance our understanding of what we have wrought.
> 
> I am not sure which patch you are NACKing, the original patch from
> Hugh (adding compound_head() to page_cgroup_ino()) or the suggested
> alternative patch which changes page_memcg_check() to use
> page_folio().

Both patches are NACKed.  page_memcg_check() needs to go away
because it has the tail page ambiguity.  Both callers should be using
folio_memcg_check() directly and resolving for themselves what the
correct behaviour is when seeing a tail page.