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

[Yosry Ahmed <yosryahmed@google.com>]

On Wed, Mar 15, 2023 at 8:07 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Mar 15, 2023 at 05:25:49PM -0700, Yosry Ahmed wrote:
> [snipped 80 lines.  please learn to trim]
> > I think instead of explicitly checking page->memcg_data, we can check
> > PageTail() and return explicitly for tail pages tails, check
> > PageSlab() to print the message for slab pages, then get the page's
> > memcg through folio_memcg_check(page_folio(page)).
> >
> > Something like:
> >
> > static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> > struct page *page)
> > {
> >     ...
> >     rcu_read_lock();
> >
> >     /* Only head pages hold refs to a memcg */
> >     if (PageTail(page))
> >         goto out_unlock;
> >
> >     if (PageSlab(page))
> >         ret += scnprintf(kbuf + ret, count - ret, "Slab cache page\n");
> >
> >     memcg = folio_memcg_check(page_folio(page));
> >     if (!memcg)
> >         goto out_unlock;
> >     ...
> > }
> >
> > Matthew, What do you think?
>
> Brrr, this is hard.  read_page_owner() holds no locks or references,
> so pages can transform between being head/tail/order-0 while we're
> running.
>
> It _tries_ to skip over tail pages in the most inefficient way possible:
>
>                 if (!IS_ALIGNED(pfn, 1 << page_owner->order))
>                         goto ext_put_continue;
>
> But any attempt to use folio APIs is going to risk tripping the
> assertions in the folio code that it's not a tail.  This requires
> more thought.

Ugh yeah. I thought the worst that could happen is that if a page
becomes a tail page after the PageTail() check, then we will not skip
it and we will read the memcg from the head instead, which shouldn't
be the end of the world. I missed the fact that the folio returned by
page_folio() can change before folio_memcg_check() gets to read its
memcg_data.

I guess this race exists with the current implementation as well.
page_memcg_check() will check for tail pages then cast the page to a
folio. If the page becomes a tail page after the PageTail() check
inside page_memcg_check() we risk running into the same situation.

(FWIW folio_memcg_check() doesn't seem to have assertions for tail pages today)