Re: [PATCH v5 8/9] mm: multi-gen LRU: Have secondary MMUs participate in aging

[James Houghton <jthoughton@google.com>]

On Fri, Jul 5, 2024 at 11:36 AM Yu Zhao <yuzhao@google.com> wrote:
>
> On Mon, Jun 10, 2024 at 6:22 PM James Houghton <jthoughton@google.com> wrote:
> >
> > Secondary MMUs are currently consulted for access/age information at
> > eviction time, but before then, we don't get accurate age information.
> > That is, pages that are mostly accessed through a secondary MMU (like
> > guest memory, used by KVM) will always just proceed down to the oldest
> > generation, and then at eviction time, if KVM reports the page to be
> > young, the page will be activated/promoted back to the youngest
> > generation.
> >
> > The added feature bit (0x8), if disabled, will make MGLRU behave as if
> > there are no secondary MMUs subscribed to MMU notifiers except at
> > eviction time.
> >
> > Implement aging with the new mmu_notifier_test_clear_young_fast_only()
> > notifier. For architectures that do not support this notifier, this
> > becomes a no-op. For architectures that do implement it, it should be
> > fast enough to make aging worth it.
> >
> > Suggested-by: Yu Zhao <yuzhao@google.com>
> > Signed-off-by: James Houghton <jthoughton@google.com>
> > ---
> >
> > Notes:
> >     should_look_around() can sometimes use two notifiers now instead of one.
> >
> >     This simply comes from restricting myself from not changing
> >     mmu_notifier_clear_young() to return more than just "young or not".
> >
> >     I could change mmu_notifier_clear_young() (and
> >     mmu_notifier_test_young()) to return if it was fast or not. At that
> >     point, I could just as well combine all the notifiers into one notifier,
> >     like what was in v2 and v3.
> >
> >  Documentation/admin-guide/mm/multigen_lru.rst |   6 +-
> >  include/linux/mmzone.h                        |   6 +-
> >  mm/rmap.c                                     |   9 +-
> >  mm/vmscan.c                                   | 185 ++++++++++++++----
> >  4 files changed, 164 insertions(+), 42 deletions(-)
>
> ...
>
> >  static bool walk_pte_range(pmd_t *pmd, unsigned long start, unsigned long end,
> >                            struct mm_walk *args)
> >  {
> > @@ -3357,8 +3416,9 @@ static bool walk_pte_range(pmd_t *pmd, unsigned long start, unsigned long end,
> >         struct pglist_data *pgdat = lruvec_pgdat(walk->lruvec);
> >         DEFINE_MAX_SEQ(walk->lruvec);
> >         int old_gen, new_gen = lru_gen_from_seq(max_seq);
> > +       struct mm_struct *mm = args->mm;
> >
> > -       pte = pte_offset_map_nolock(args->mm, pmd, start & PMD_MASK, &ptl);
> > +       pte = pte_offset_map_nolock(mm, pmd, start & PMD_MASK, &ptl);
> >         if (!pte)
> >                 return false;
> >         if (!spin_trylock(ptl)) {
> > @@ -3376,11 +3436,12 @@ static bool walk_pte_range(pmd_t *pmd, unsigned long start, unsigned long end,
> >                 total++;
> >                 walk->mm_stats[MM_LEAF_TOTAL]++;
> >
> > -               pfn = get_pte_pfn(ptent, args->vma, addr);
> > +               pfn = get_pte_pfn(ptent, args->vma, addr, pgdat);
> >                 if (pfn == -1)
> >                         continue;
> >
> > -               if (!pte_young(ptent)) {
> > +               if (!pte_young(ptent) &&
> > +                   !lru_gen_notifier_test_young(mm, addr)) {
> >                         walk->mm_stats[MM_LEAF_OLD]++;
> >                         continue;
> >                 }
> > @@ -3389,8 +3450,9 @@ static bool walk_pte_range(pmd_t *pmd, unsigned long start, unsigned long end,
> >                 if (!folio)
> >                         continue;
> >
> > -               if (!ptep_test_and_clear_young(args->vma, addr, pte + i))
> > -                       VM_WARN_ON_ONCE(true);
> > +               lru_gen_notifier_clear_young(mm, addr, addr + PAGE_SIZE);
> > +               if (pte_young(ptent))
> > +                       ptep_test_and_clear_young(args->vma, addr, pte + i);
> >
> >                 young++;
> >                 walk->mm_stats[MM_LEAF_YOUNG]++;
>
>
> There are two ways to structure the test conditions in walk_pte_range():
> 1. a single pass into the MMU notifier (combine test/clear) which
> causes a cache miss from get_pfn_page() if the page is NOT young.
> 2. two passes into the MMU notifier (separate test/clear) if the page
> is young, which does NOT cause a cache miss if the page is NOT young.
>
> v2 can batch up to 64 PTEs, i.e., it only goes into the MMU notifier
> twice every 64 PTEs, and therefore the second option is a clear win.
>
> But you are doing twice per PTE. So what's the rationale behind going
> with the second option? Was the first option considered?

Hi Yu,

I didn't consider changing this from your v2[1]. Thanks for bringing it up.

The only real change I have made is that I reordered the
(!test_spte_young() && !pte_young()) to what it is now (!pte_young()
&& !lru_gen_notifier_test_young()) because pte_young() can be
evaluated much faster.

I am happy to change the initial test_young() notifier to a
clear_young() (and drop the later clear_young(). In fact, I think I
should. Making the condition (!pte_young() &&
!lru_gen_notifier_clear_young()) makes sense to me. This returns the
same result as if it were !lru_gen_notifier_test_young() instead,
there is no need for a second clear_young(), and we don't call
get_pfn_folio() on pages that are not young.

WDYT? Have I misunderstood your comment?

Also, I take it your comment was not just about walk_pte_range() but
about the similar bits in lru_gen_look_around() as well, so I'll make
whatever changes we agree on there too (or maybe factor out the common
bits).

[1]: https://lore.kernel.org/kvmarm/20230526234435.662652-11-yuzhao@google.com/

> In addition, what about the non-lockless cases? Would this change make
> them worse by grabbing the MMU lock twice per PTE?

That's a good point. Yes I think calling the notifier twice here would
indeed exacerbate problems with a non-lockless notifier.

Thanks!