On Wed, Mar 15, 2023 at 8:07 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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)
