On Fri, Jul 5, 2024 at 11:36 AM Yu Zhao <yuzhao@xxxxxxxxxx> wrote:
>
> On Mon, Jun 10, 2024 at 6:22 PM James Houghton <jthoughton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: James Houghton <jthoughton@xxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxx/
> 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!
