[permalink] [raw]

Subject: Re: [PATCH v3 0/7] mm/kvm: Improve parallelism for access bit harvesting

On Fri, Apr 19, 2024 at 01:57:03PM -0700, James Houghton wrote:
> On Fri, Apr 12, 2024 at 11:41 AM David Matlack <[email protected]> wrote:
> >
> > On 2024-04-01 11:29 PM, James Houghton wrote:
> > > This patchset adds a fast path in KVM to test and clear access bits on
> > > sptes without taking the mmu_lock. It also adds support for using a
> > > bitmap to (1) test the access bits for many sptes in a single call to
> > > mmu_notifier_test_young, and to (2) clear the access bits for many ptes
> > > in a single call to mmu_notifier_clear_young.
> >
> > How much improvement would we get if we _just_ made test/clear_young
> > lockless on x86 and hold the read-lock on arm64? And then how much
> > benefit does the bitmap look-around add on top of that?

Thanks David for providing the suggestion.

> I don't have these results right now. For the next version I will (1)
> separate the series into the locking change and the bitmap change, and
> I will (2) have performance data for each change separately. It is
> conceivable that the bitmap change should just be considered as a
> completely separate patchset.

That'd be great. Having the performance numbers will make it even more
compelling, but I'd be tempted to go for the lock improvement just
because it doesn't add any new complexity and leverages existing patterns
in the architectures that people seem to want improvements for.

The bitmap interface, OTOH, is rather complex. At least the current
implementation breaks some of the isolation we have between the MMU code
and the page table walker library on arm64, which I'm not ecstatic about.
It _could_ be justified by a massive performance uplift over locking, but
it'd have to be a sizable win.

--
Thanks,
Oliver

2024-04-21 00:20:24

by Yu Zhao

[permalink] [raw]

Subject: Re: [PATCH v3 5/7] KVM: x86: Participate in bitmap-based PTE aging

On Fri, Apr 19, 2024 at 3:48 PM James Houghton <[email protected]> wrote:
>
> On Fri, Apr 19, 2024 at 2:07 PM David Matlack <[email protected]> wrote:
> >
> > On 2024-04-19 01:47 PM, James Houghton wrote:
> > > On Thu, Apr 11, 2024 at 10:28 AM David Matlack <[email protected]> wrote:
> > > > On 2024-04-11 10:08 AM, David Matlack wrote:
> > > > bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
> > > > {
> > > > bool young = false;
> > > >
> > > > if (!range->arg.metadata->bitmap && kvm_memslots_have_rmaps(kvm))
> > > > young = kvm_handle_gfn_range(kvm, range, kvm_age_rmap);
> > > >
> > > > if (tdp_mmu_enabled)
> > > > young |= kvm_tdp_mmu_age_gfn_range(kvm, range);
> > > >
> > > > return young;
> > > > }
> > > >
> > > > bool kvm_test_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
> > > > {
> > > > bool young = false;
> > > >
> > > > if (!range->arg.metadata->bitmap && kvm_memslots_have_rmaps(kvm))
> > > > young = kvm_handle_gfn_range(kvm, range, kvm_test_age_rmap);
> > > >
> > > > if (tdp_mmu_enabled)
> > > > young |= kvm_tdp_mmu_test_age_gfn(kvm, range);
> > > >
> > > > return young;
> > >
> > >
> > > Yeah I think this is the right thing to do. Given your other
> > > suggestions (on patch 3), I think this will look something like this
> > > -- let me know if I've misunderstood something:
> > >
> > > bool check_rmap = !bitmap && kvm_memslot_have_rmaps(kvm);
> > >
> > > if (check_rmap)
> > > KVM_MMU_LOCK(kvm);
> > >
> > > rcu_read_lock(); // perhaps only do this when we don't take the MMU lock?
> > >
> > > if (check_rmap)
> > > kvm_handle_gfn_range(/* ... */ kvm_test_age_rmap)
> > >
> > > if (tdp_mmu_enabled)
> > > kvm_tdp_mmu_test_age_gfn() // modified to be RCU-safe
> > >
> > > rcu_read_unlock();
> > > if (check_rmap)
> > > KVM_MMU_UNLOCK(kvm);
> >
> > I was thinking a little different. If you follow my suggestion to first
> > make the TDP MMU aging lockless, you'll end up with something like this
> > prior to adding bitmap support (note: the comments are just for
> > demonstrative purposes):
> >
> > bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
> > {
> > bool young = false;
> >
> > /* Shadow MMU aging holds write-lock. */
> > if (kvm_memslots_have_rmaps(kvm)) {
> > write_lock(&kvm->mmu_lock);
> > young = kvm_handle_gfn_range(kvm, range, kvm_age_rmap);
> > write_unlock(&kvm->mmu_lock);
> > }
> >
> > /* TDM MMU aging is lockless. */
> > if (tdp_mmu_enabled)
> > young |= kvm_tdp_mmu_age_gfn_range(kvm, range);
> >
> > return young;
> > }
> >
> > Then when you add bitmap support it would look something like this:
> >
> > bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
> > {
> > unsigned long *bitmap = range->arg.metadata->bitmap;
> > bool young = false;
> >
> > /* SHadow MMU aging holds write-lock and does not support bitmap. */
> > if (kvm_memslots_have_rmaps(kvm) && !bitmap) {
> > write_lock(&kvm->mmu_lock);
> > young = kvm_handle_gfn_range(kvm, range, kvm_age_rmap);
> > write_unlock(&kvm->mmu_lock);
> > }
> >
> > /* TDM MMU aging is lockless and supports bitmap. */
> > if (tdp_mmu_enabled)
> > young |= kvm_tdp_mmu_age_gfn_range(kvm, range);
> >
> > return young;
> > }
> >
> > rcu_read_lock/unlock() would be called in kvm_tdp_mmu_age_gfn_range().
>
> Oh yes this is a lot better. I hope I would have seen this when it
> came time to actually update this patch. Thanks.
>
> >
> > That brings up a question I've been wondering about. If KVM only
> > advertises support for the bitmap lookaround when shadow roots are not
> > allocated, does that mean MGLRU will be blind to accesses made by L2
> > when nested virtualization is enabled? And does that mean the Linux MM
> > will think all L2 memory is cold (i.e. good candidate for swapping)
> > because it isn't seeing accesses made by L2?
>
> Yes, I think so (for both questions). That's better than KVM not
> participating in MGLRU aging at all, which is the case today (IIUC --
> also ignoring the case where KVM accesses guest memory directly). We
> could have MGLRU always invoke the mmu notifiers, but frequently
> taking the MMU lock for writing might be worse than evicting when we
> shouldn't. Maybe Yu tried this at some point, but I can't find any
> results for this.

No, in this case only the fast path (page table scanning) is disabled.
MGLRU still sees the A-bit from L2 using the rmap, i.e., the slow path
calling folio_check_references().