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

[Yu Zhao <yuzhao@google.com>]

On Mon, Jul 8, 2024 at 11:31 AM James Houghton <jthoughton@google.com> wrote:
>
> 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.

We don't want to do that because we would lose the A-bit for a folio
that's beyond the current reclaim scope, i.e., the cases where
get_pfn_folio() returns NULL (a folio from another memcg, e.g.).

> WDYT? Have I misunderstood your comment?

I hope this is clear enough:

@@ -3395,7 +3395,7 @@ static bool walk_pte_range(pmd_t *pmd, unsigned
long start, unsigned long end,
                if (pfn == -1)
                        continue;

-               if (!pte_young(ptent)) {
+               if (!pte_young(ptent) && !mm_has_notifiers(args->mm)) {
                        walk->mm_stats[MM_LEAF_OLD]++;
                        continue;
                }
@@ -3404,8 +3404,8 @@ 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);
+               if (!ptep_clear_young_notify(args->vma, addr, pte + i))
+                       continue;

                young++;
                walk->mm_stats[MM_LEAF_YOUNG]++;

> 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.

I think so too, but I haven't verified it. Please do?