| Message ID | 20240724011037.3671523-2-jthoughton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: multi-gen LRU: Walk secondary MMU page tables while aging | expand |
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@google.com> > Signed-off-by: James Houghton <jthoughton@google.com> Aside from the cleanup suggestion (which should be in separate patches anyway): Reviewed-by: David Matlack <dmatlack@google.com> > --- > 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(). e.g. Something like this as the end result: diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 86fb2b560d98..0146c83e24bd 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -590,8 +590,8 @@ static void kvm_null_fn(void) node; \ node = interval_tree_iter_next(node, start, last)) \ -static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm, - const struct kvm_mmu_notifier_range *range) +static __always_inline kvm_mn_ret_t kvm_handle_hva_range(struct kvm *kvm, + const struct kvm_mmu_notifier_range *range) { struct kvm_mmu_notifier_return r = { .ret = false, @@ -674,48 +674,6 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm, return r; } -static __always_inline int kvm_handle_hva_range(struct mmu_notifier *mn, - unsigned long start, - unsigned long end, - gfn_handler_t handler) -{ - struct kvm *kvm = mmu_notifier_to_kvm(mn); - const struct kvm_mmu_notifier_range range = { - .start = start, - .end = end, - .handler = handler, - .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; -} - -static __always_inline int kvm_handle_hva_range_no_flush(struct mmu_notifier *mn, - unsigned long start, - unsigned long end, - gfn_handler_t handler, - bool fast_only) -{ - struct kvm *kvm = mmu_notifier_to_kvm(mn); - const struct kvm_mmu_notifier_range range = { - .start = start, - .end = end, - .handler = handler, - .on_lock = (void *)kvm_null_fn, - .flush_on_ret = false, - .may_block = false, - .lockless = - IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), - .arg.fast_only = fast_only, - }; - - return __kvm_handle_hva_range(kvm, &range).ret; -} - void kvm_mmu_invalidate_begin(struct kvm *kvm) { lockdep_assert_held_write(&kvm->mmu_lock); @@ -808,7 +766,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, * that guest memory has been reclaimed. This needs to be done *after* * dropping mmu_lock, as x86's reclaim path is slooooow. */ - if (__kvm_handle_hva_range(kvm, &hva_range).found_memslot) + if (kvm_handle_hva_range(kvm, &hva_range).found_memslot) kvm_arch_guest_memory_reclaimed(kvm); return 0; @@ -854,7 +812,7 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn, }; bool wake; - __kvm_handle_hva_range(kvm, &hva_range); + kvm_handle_hva_range(kvm, &hva_range); /* Pairs with the increment in range_start(). */ spin_lock(&kvm->mn_invalidate_lock); @@ -876,6 +834,17 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn, unsigned long start, unsigned long end) { + struct kvm *kvm = mmu_notifier_to_kvm(mn); + const struct kvm_mmu_notifier_range range = { + .start = start, + .end = end, + .handler = kvm_age_gfn, + .on_lock = (void *)kvm_null_fn, + .flush_on_ret = true, + .may_block = false, + .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), + }; + trace_kvm_age_hva(start, end, false); return kvm_handle_hva_range(mn, start, end, kvm_age_gfn); @@ -887,6 +856,18 @@ static int kvm_mmu_notifier_clear_young(struct mmu_notifier *mn, unsigned long end, bool fast_only) { + struct kvm *kvm = mmu_notifier_to_kvm(mn); + const struct kvm_mmu_notifier_range range = { + .start = start, + .end = end, + .handler = kvm_age_gfn, + .on_lock = (void *)kvm_null_fn, + .flush_on_ret = false, + .may_block = false, + .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), + .arg.fast_only = fast_only, + }; + trace_kvm_age_hva(start, end, fast_only); /* @@ -902,8 +883,7 @@ static int kvm_mmu_notifier_clear_young(struct mmu_notifier *mn, * cadence. If we find this inaccurate, we might come up with a * more sophisticated heuristic later. */ - return kvm_handle_hva_range_no_flush(mn, start, end, kvm_age_gfn, - fast_only); + return kvm_handle_hva_range(kvm, &range).ret; } static int kvm_mmu_notifier_test_young(struct mmu_notifier *mn, @@ -911,6 +891,18 @@ static int kvm_mmu_notifier_test_young(struct mmu_notifier *mn, unsigned long address, bool fast_only) { + struct kvm *kvm = mmu_notifier_to_kvm(mn); + const struct kvm_mmu_notifier_range range = { + .start = address, + .end = address + 1, + .handler = kvm_test_age_gfn, + .on_lock = (void *)kvm_null_fn, + .flush_on_ret = false, + .may_block = false, + .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), + .arg.fast_only = fast_only, + }; + trace_kvm_test_age_hva(address, fast_only); return kvm_handle_hva_range_no_flush(mn, address, address + 1, > }; > > return __kvm_handle_hva_range(kvm, &range).ret; > -- > 2.46.0.rc1.232.g9752f9e123-goog >
On Thu, Jul 25, 2024 at 9:39 AM David Matlack <dmatlack@google.com> 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@google.com> > > Signed-off-by: James Houghton <jthoughton@google.com> > > Aside from the cleanup suggestion (which should be in separate patches > anyway): > > Reviewed-by: David Matlack <dmatlack@google.com> 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.
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), }; return __kvm_handle_hva_range(kvm, &range).ret;
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@google.com> Signed-off-by: James Houghton <jthoughton@google.com> --- include/linux/kvm_host.h | 1 + virt/kvm/Kconfig | 3 +++ virt/kvm/kvm_main.c | 26 +++++++++++++++++++------- 3 files changed, 23 insertions(+), 7 deletions(-)