On Tue, Mar 14, 2023 at 9:54 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, 13 Mar 2023 08:34:52 +0000 Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> > >
> > > > From: Hugh Dickins <hughd@xxxxxxxxxx>
> > > >
> > > > 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?
>
> 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().
I am assuming the latter (based on where this reply was placed). If
that's the case, are you okay with the original patch? If yes, then
changing page_memcg_check() to use page_folio() should be a NOP as the
only 2 callers today are page_cgroup_ino() and
print_page_owner_memcg(). The latter already implicitly excludes tail
pages by checking if memcg_data == 0, and we can add an explicit
PageTail() check there instead.
Or is it the former (original patch)?
