On Mon, Jul 8, 2024 at 11:31 AM James Houghton <jthoughton@xxxxxxxxxx> wrote:
>
> 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.
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@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.
I think so too, but I haven't verified it. Please do?
