Re: [PATCH v14 07/14] mm: multi-gen LRU: exploit locality in rmap

[Yu Zhao <yuzhao@google.com>]

On Thu, Sep 1, 2022 at 7:17 PM Yu Zhao <yuzhao@google.com> wrote:
>
> On Thu, Sep 1, 2022 at 3:18 AM Nadav Amit <nadav.amit@gmail.com> wrote:
> >
> >
> >
> > > On Aug 15, 2022, at 12:13 AM, Yu Zhao <yuzhao@google.com> wrote:
> > >
> > > Searching the rmap for PTEs mapping each page on an LRU list (to test
> > > and clear the accessed bit) can be expensive because pages from
> > > different VMAs (PA space) are not cache friendly to the rmap (VA
> > > space). For workloads mostly using mapped pages, searching the rmap
> > > can incur the highest CPU cost in the reclaim path.
> >
> > Impressive work.

Thanks.

> > Sorry if my feedback is not timely.
> >
> > Just one minor point for thought, that can be left for a later cleanup.
> >
> > >
> > > +     for (i = 0, addr = start; addr != end; i++, addr += PAGE_SIZE) {
> > > +             unsigned long pfn;
> > > +
> > > +             pfn = get_pte_pfn(pte[i], pvmw->vma, addr);
> > > +             if (pfn == -1)
> > > +                     continue;
> > > +
> > > +             if (!pte_young(pte[i]))
> > > +                     continue;
> > > +
> > > +             folio = get_pfn_folio(pfn, memcg, pgdat);
> > > +             if (!folio)
> > > +                     continue;
> > > +
> > > +             if (!ptep_test_and_clear_young(pvmw->vma, addr, pte + i))
> > > +                     continue;
> > > +
> >
> > You have already checked that the PTE is old (not young) so this check
> > seems redundant.
>
> You are right, for x86, which belongs to category 1: hardware and
> OS share the same paging data structure.
>
> > I do not see a way in which the access-bit can be cleared
> > since you hold the ptl.
>
> There is also category 2: the OS paging data structure is a shadow of what
> hardware actually uses, e.g., POWER9 radix.
>
> To make both categories work, the general rule is that the OS paging
> data structure must be more strict, i.e., it can have A/D bits set
> while the hardware paging data structure may not. The opposite is not
> allowed, even for the A bit, because the A bit can also be used to
> determine whether a TLB flush is required. The Linux kernel doesn't do
> this but there are other OSes that do.
>
> For prefaulted PTEs, we generally mark them young unless
> arch_wants_old_prefaulted_pte() returns true (currently only ARMv8.2+
> do). On POWER9, we'd see those PTEs pass the first check but fail the
> second.

Because the first check (non-atomic) is allowed to fetch from the OS
paging data structure (which is more strict) while the second check
(atomic) must fetch from the hardware page data structure (which does
not have the A bit because those PTEs are preffaulted).

> > IOW, there is no need for the “if" and “continue".
> >
> > Makes me also wonder whether having a separate ptep_clear_young() can
> > slightly help, since anyhow the access-bit is more of an estimation,
> > and having a separate ptep_clear_young() can enable optimizations.
> >
> > On x86, for instance, if the PTE is dirty, we may be able to clear the
> > access-bit without an atomic operation, which should be faster.
>
> Agreed.