Idle THPs

Idle THPs

[Matthew Wilcox]

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?

Re: Idle THPs

[SeongJae Park]

From: SeongJae Park <sjpark@amazon.de>

On Thu, 10 Jun 2021 04:43:18 +0100 Matthew Wilcox <willy@infradead.org> 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?

I think this is an intended behavior, as the document[1] says as below:

    For huge pages the idle flag is set only on the head page, so one has to
    read /proc/kpageflags in order to correctly count idle huge pages.

[1] https://www.kernel.org/doc/html/latest/admin-guide/mm/idle_page_tracking.html

Thanks,
SeongJae Park

Re: Idle THPs

[Shakeel Butt]

+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