On Mon, Jul 8, 2024 at 4:42 PM Yu Zhao <yuzhao@xxxxxxxxxx> wrote:
>
> 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:
> > > > @@ -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))
walk->mm_stats[MM_LEAF_OLD]++ should be here, I take it.
> + 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?
I have some results now, sorry for the wait.
It seems like one notifier is definitely better. It doesn't look like
the read lock actually made anything worse with what I was testing
(faulting memory in while doing aging). This is kind of surprising,
but either way, I'll change it to the single notifier in v6. Thanks
Yu!
Here are the results I'm basing this conclusion on, using the selftest
added at the end of this series.
# Use taskset to minimize NUMA concern.
# Give an extra core for the aging thread.
# THPs disabled (echo never > /sys/kernel/mm/transparent_hugepage/enabled)
x86:
# taskset -c 0-32 ./access_tracking_perf_test -l -v 32
# # One notifier
Populating memory : 1.933017284s
Writing to populated memory : 0.017323539s
Reading from populated memory : 0.013113260s
lru_gen: Aging : 0.894133259s
lru_gen: Aging : 0.738950525s
Writing to idle memory : 0.059661329s
lru_gen: Aging : 0.922719935s
lru_gen: Aging : 0.829129877s
Reading from idle memory : 0.059095098s
lru_gen: Aging : 0.922689975s
# # Two notifiers
Populating memory : 1.842645795s
Writing to populated memory : 0.017277075s
Reading from populated memory : 0.013047457s
lru_gen: Aging : 0.900751764s
lru_gen: Aging : 0.707203167s
Writing to idle memory : 0.060663733s
lru_gen: Aging : 1.539957250s <------ got longer
lru_gen: Aging : 0.797475887s
Reading from idle memory : 0.084415591s
lru_gen: Aging : 1.539417121s <------ got longer
arm64*:
(*Patched to do aging; not done in v5 or v6. Doing this to see if the read
lock is made substantially worse by using two notifiers vs. one.)
# taskset -c 0-16 ./access_tracking_perf_test -l -v 16 -m 3
# # One notifier
Populating memory : 1.439261355s
Writing to populated memory : 0.009755279s
Reading from populated memory : 0.007714120s
lru_gen: Aging : 0.540183328s
lru_gen: Aging : 0.455427973s
Writing to idle memory : 0.010130399s
lru_gen: Aging : 0.563424247s
lru_gen: Aging : 0.500419850s
Reading from idle memory : 0.008519640s
lru_gen: Aging : 0.563178643s
# # Two notifiers
Populating memory : 1.526805625s
Writing to populated memory : 0.009836118s
Reading from populated memory : 0.007757280s
lru_gen: Aging : 0.537770978s
lru_gen: Aging : 0.421915391s
Writing to idle memory : 0.010281959s
lru_gen: Aging : 0.971448688s <------ got longer
lru_gen: Aging : 0.466956547s
Reading from idle memory : 0.008588559s
lru_gen: Aging : 0.971030648s <------ got longer
arm64, faulting memory in while aging:
# perf record -g -- taskset -c 0-16 ./access_tracking_perf_test -l -v 16 -m 3 -p
# # One notifier
vcpu wall time : 1.433908058s
lru_gen avg pass duration : 0.172128073s, (passes:11, total:1.893408807s)
# # Two notifiers
vcpu wall time : 1.450387765s
lru_gen avg pass duration : 0.175652974s, (passes:10, total:1.756529744s)
# perf report
# # One notifier
- 6.25% 0.00% access_tracking [kernel.kallsyms] [k] try_to_inc_max_seq
- try_to_inc_max_seq
- 6.06% walk_page_range
__walk_page_range
- walk_pgd_range
- 6.04% walk_pud_range
- 4.73% __mmu_notifier_clear_young
+ 4.29% kvm_mmu_notifier_clear_young
# # Two notifiers
- 6.43% 0.00% access_tracking [kernel.kallsyms] [k] try_to_inc_max_seq
- try_to_inc_max_seq
- 6.25% walk_page_range
__walk_page_range
- walk_pgd_range
- 6.23% walk_pud_range
- 2.75% __mmu_notifier_test_young
+ 2.48% kvm_mmu_notifier_test_young
- 2.39% __mmu_notifier_clear_young
+ 2.19% kvm_mmu_notifier_clear_young
