On Thu, Jul 25, 2024 at 9:39 AM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On 2024-07-24 01:10 AM, James Houghton wrote:
> > Provide flexibility to the architecture to synchronize as optimally as
> > they can instead of always taking the MMU lock for writing.
> >
> > Architectures that do their own locking must select
> > CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS.
> >
> > The immediate application is to allow architectures to implement the
> > test/clear_young MMU notifiers more cheaply.
> >
> > Suggested-by: Yu Zhao <yuzhao@xxxxxxxxxx>
> > Signed-off-by: James Houghton <jthoughton@xxxxxxxxxx>
>
> Aside from the cleanup suggestion (which should be in separate patches
> anyway):
>
> Reviewed-by: David Matlack <dmatlack@xxxxxxxxxx>
Thanks David!
>
> > ---
> > include/linux/kvm_host.h | 1 +
> > virt/kvm/Kconfig | 3 +++
> > virt/kvm/kvm_main.c | 26 +++++++++++++++++++-------
> > 3 files changed, 23 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 689e8be873a7..8cd80f969cff 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -266,6 +266,7 @@ struct kvm_gfn_range {
> > gfn_t end;
> > union kvm_mmu_notifier_arg arg;
> > bool may_block;
> > + bool lockless;
> > };
> > bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range);
> > bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range);
> > diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> > index b14e14cdbfb9..632334861001 100644
> > --- a/virt/kvm/Kconfig
> > +++ b/virt/kvm/Kconfig
> > @@ -100,6 +100,9 @@ config KVM_GENERIC_MMU_NOTIFIER
> > select MMU_NOTIFIER
> > bool
> >
> > +config KVM_MMU_NOTIFIER_YOUNG_LOCKLESS
> > + bool
> > +
> > config KVM_GENERIC_MEMORY_ATTRIBUTES
> > depends on KVM_GENERIC_MMU_NOTIFIER
> > bool
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index d0788d0a72cc..33f8997a5c29 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -555,6 +555,7 @@ struct kvm_mmu_notifier_range {
> > on_lock_fn_t on_lock;
> > bool flush_on_ret;
> > bool may_block;
> > + bool lockless;
> > };
> >
> > /*
> > @@ -609,6 +610,10 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
> > IS_KVM_NULL_FN(range->handler)))
> > return r;
> >
> > + /* on_lock will never be called for lockless walks */
> > + if (WARN_ON_ONCE(range->lockless && !IS_KVM_NULL_FN(range->on_lock)))
> > + return r;
> > +
> > idx = srcu_read_lock(&kvm->srcu);
> >
> > for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
> > @@ -640,15 +645,18 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
> > gfn_range.start = hva_to_gfn_memslot(hva_start, slot);
> > gfn_range.end = hva_to_gfn_memslot(hva_end + PAGE_SIZE - 1, slot);
> > gfn_range.slot = slot;
> > + gfn_range.lockless = range->lockless;
> >
> > if (!r.found_memslot) {
> > r.found_memslot = true;
> > - KVM_MMU_LOCK(kvm);
> > - if (!IS_KVM_NULL_FN(range->on_lock))
> > - range->on_lock(kvm);
> > -
> > - if (IS_KVM_NULL_FN(range->handler))
> > - goto mmu_unlock;
> > + if (!range->lockless) {
> > + KVM_MMU_LOCK(kvm);
> > + if (!IS_KVM_NULL_FN(range->on_lock))
> > + range->on_lock(kvm);
> > +
> > + if (IS_KVM_NULL_FN(range->handler))
> > + goto mmu_unlock;
> > + }
> > }
> > r.ret |= range->handler(kvm, &gfn_range);
> > }
> > @@ -658,7 +666,7 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
> > kvm_flush_remote_tlbs(kvm);
> >
> > mmu_unlock:
> > - if (r.found_memslot)
> > + if (r.found_memslot && !range->lockless)
> > KVM_MMU_UNLOCK(kvm);
> >
> > srcu_read_unlock(&kvm->srcu, idx);
> > @@ -679,6 +687,8 @@ static __always_inline int kvm_handle_hva_range(struct mmu_notifier *mn,
> > .on_lock = (void *)kvm_null_fn,
> > .flush_on_ret = true,
> > .may_block = false,
> > + .lockless =
> > + IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
> > };
> >
> > return __kvm_handle_hva_range(kvm, &range).ret;
> > @@ -697,6 +707,8 @@ static __always_inline int kvm_handle_hva_range_no_flush(struct mmu_notifier *mn
> > .on_lock = (void *)kvm_null_fn,
> > .flush_on_ret = false,
> > .may_block = false,
> > + .lockless =
> > + IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
>
> kvm_handle_hva_range{,_no_flush}() have very generic names but
> they're intimately tied to the "young" notifiers. Whereas
> __kvm_handle_hva_range() is the truly generic handler function.
>
> This is arguably a pre-existing issue, but adding
> CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS makes these functions even more
> intamtely tied to the "young" notifiers.
>
> We could rename kvm_handle_hva_range{,_no_flush}() but I think the
> cleanest thing to do might be to just drop them entirely and move their
> contents into their callers (there are only 2 callers of these 3
> functions). That will create a little duplication but IMO will make the
> code easier to read.
>
> And then we can also rename __kvm_handle_hva_range() to
> kvm_handle_hva_range().
Thanks for the suggestion, I think this is a good idea. I'm curious
how others feel, as this indeed does duplicate the code some. Perhaps
it is better just to rename kvm_handle_hva_range() to
kvm_handle_hva_range_age() or something like that, and something
similar for _no_flush(). :/
But yeah I think it's fine to just do the manipulation you're
suggesting. I'll include it in v7 unless others say not to.
