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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux