Re: Idle THPs

[Shakeel Butt <shakeelb@google.com>]

+Yu Zhao

On Thu, Jun 10, 2021 at 3:54 PM Yang Shi <shy828301@gmail.com> wrote:
>
> On Thu, Jun 10, 2021 at 2:47 PM Hugh Dickins <hughd@google.com> wrote:
> >
> > On Thu, 10 Jun 2021, Matthew Wilcox wrote:
> >
> > > As part of the folio work, I'm looking at PageIdle and PageYoung and
> > > they're defined to operate on PF_ANY.  So, for example, in
> > > pagecache_get_page(), we will call clear_page_idle() on the head page
> > > (actually, I changed this in a8cf7f272b5a -- before, it would call
> > > clear_page_idle() on the tail page).
> > >
> > > However, we never actually call set_page_idle() on tail pages.  This is
> > > because we only call it here:
> > >
> > >                         page = page_idle_get_page(pfn);
> > >                         if (page) {
> > >                                 page_idle_clear_pte_refs(page);
> > >                                 set_page_idle(page);
> > >                                 put_page(page);
> > >                         }
> > >
> > > where page_idle_get_page() does:
> > >
> > >         struct page *page = pfn_to_online_page(pfn);
> > >
> > >         if (!page || !PageLRU(page) ||
> > >             !get_page_unless_zero(page))
> > >                 return NULL;
> > >
> > > get_page_unless_zero() will always fail for tail pages (as it uses
> > > page_ref_add_unless(), which does not redirect to the head page's
> > > refcount).  So all tail pages read back as !idle in
> > > page_idle_bitmap_read().  Is this intended?  Should they rather
> > > mirror the state of their head page?

From what I understand the idle bitmap is supposed to be used along
with /proc/kpageflags. So, the users should skip tail pages for
setting/getting the idle bits.

> >
> > Good point.
> >
> > I bet when I made that no-lru_lock cleanup in page_idle_get_page(),
> > I was expecting the PageLRU to fail on tail, so get_page_unless_zero()
> > irrelevant; but apparently PageLRU is PF_HEAD redirecting to head.
> > Either way, yes, it will return NULL on tail, which may not be right.
> >
> > But maybe the physical scan works out okay with all the action
> > happening on the head (Kirill got that to scan the tails in pvmw),
> > then skipping the tails in the local scan.
> >
> > I'm not a page_idle user and don't want to get into the mechanics of it.
> > Seems to be largely in maintenance mode these days, maybe nobody cares.
> >
> > Yang Shi was the last to make a real mod there, f0849ac0b8e0 ("mm: thp: fix
> > potential clearing to referenced flag in page_idle_clear_pte_refs_one()"):
> > likely he will know best.
>
> It was more than 3 years ago :-)
>
> Since the whole THP is considered referenced if any one of sub page is
> referenced, so IMHO the tail page should mirror the state of their
> head page.
>
> And AFAIK Google uses the idle flag to reclaim memory proactively, but
> it is an out-of-tree feature. Loop Shakeel in this thread.

We are not directly using upstream idle page infrastructure but
surgically using the parts we need. Yu can provide more details.

>
> >
> > Hugh