On Wed, Mar 15, 2023 at 8:16 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> 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)
Matthew, any thoughts on this?
