* [PATCH v2 0/2] selftests: KVM: Add a test for eager page splitting @ 2023-01-23 19:03 Ben Gardon 2023-01-23 19:03 ` [PATCH v2 1/2] selftests: KVM: Move dirty logging functions to memstress.(c|h) Ben Gardon 2023-01-23 19:03 ` [PATCH v2 2/2] selftests: KVM: Add page splitting test Ben Gardon 0 siblings, 2 replies; 6+ messages in thread From: Ben Gardon @ 2023-01-23 19:03 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Peter Xu, Sean Christopherson, David Matlack, Vipin Sharma, Ricardo Koller, Ben Gardon David Matlack recently added a feature known as eager page splitting to x86 KVM. This feature improves vCPU performance during dirty logging because the splitting operation is moved out of the page fault path, avoiding EPT/NPT violations or allowing the vCPU threads to resolve the violation in the fast path. While this feature is a great performance improvement, it does not have adequate testing in KVM selftests. Add a test to provide coverage of eager page splitting. Patch 1 is a quick refactor to be able to re-use some code from dirty_log_perf_test. Patch 2 adds the actual test. V1->V2: Run test in multiple modes, as suggested by David and Ricardo Cleanups from shameful copy-pasta, as suggested by David Ben Gardon (2): selftests: KVM: Move dirty logging functions to memstress.(c|h) selftests: KVM: Add page splitting test tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/dirty_log_perf_test.c | 84 +----- .../selftests/kvm/include/kvm_util_base.h | 1 + .../testing/selftests/kvm/include/memstress.h | 8 + tools/testing/selftests/kvm/lib/kvm_util.c | 5 + tools/testing/selftests/kvm/lib/memstress.c | 72 +++++ .../kvm/x86_64/page_splitting_test.c | 278 ++++++++++++++++++ 7 files changed, 372 insertions(+), 77 deletions(-) create mode 100644 tools/testing/selftests/kvm/x86_64/page_splitting_test.c -- 2.39.1.405.gd4c25cc71f-goog ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] selftests: KVM: Move dirty logging functions to memstress.(c|h) 2023-01-23 19:03 [PATCH v2 0/2] selftests: KVM: Add a test for eager page splitting Ben Gardon @ 2023-01-23 19:03 ` Ben Gardon 2023-01-23 21:03 ` Vipin Sharma 2023-01-23 19:03 ` [PATCH v2 2/2] selftests: KVM: Add page splitting test Ben Gardon 1 sibling, 1 reply; 6+ messages in thread From: Ben Gardon @ 2023-01-23 19:03 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Peter Xu, Sean Christopherson, David Matlack, Vipin Sharma, Ricardo Koller, Ben Gardon Move some helper functions from dirty_log_perf_test.c to the memstress library so that they can be used in a future commit which tests page splitting during dirty logging. Signed-off-by: Ben Gardon <bgardon@google.com> --- .../selftests/kvm/dirty_log_perf_test.c | 84 ++----------------- .../testing/selftests/kvm/include/memstress.h | 8 ++ tools/testing/selftests/kvm/lib/memstress.c | 72 ++++++++++++++++ 3 files changed, 87 insertions(+), 77 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index e9d6d1aecf89c..416719e205183 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -136,77 +136,6 @@ struct test_params { bool random_access; }; -static void toggle_dirty_logging(struct kvm_vm *vm, int slots, bool enable) -{ - int i; - - for (i = 0; i < slots; i++) { - int slot = MEMSTRESS_MEM_SLOT_INDEX + i; - int flags = enable ? KVM_MEM_LOG_DIRTY_PAGES : 0; - - vm_mem_region_set_flags(vm, slot, flags); - } -} - -static inline void enable_dirty_logging(struct kvm_vm *vm, int slots) -{ - toggle_dirty_logging(vm, slots, true); -} - -static inline void disable_dirty_logging(struct kvm_vm *vm, int slots) -{ - toggle_dirty_logging(vm, slots, false); -} - -static void get_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], int slots) -{ - int i; - - for (i = 0; i < slots; i++) { - int slot = MEMSTRESS_MEM_SLOT_INDEX + i; - - kvm_vm_get_dirty_log(vm, slot, bitmaps[i]); - } -} - -static void clear_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], - int slots, uint64_t pages_per_slot) -{ - int i; - - for (i = 0; i < slots; i++) { - int slot = MEMSTRESS_MEM_SLOT_INDEX + i; - - kvm_vm_clear_dirty_log(vm, slot, bitmaps[i], 0, pages_per_slot); - } -} - -static unsigned long **alloc_bitmaps(int slots, uint64_t pages_per_slot) -{ - unsigned long **bitmaps; - int i; - - bitmaps = malloc(slots * sizeof(bitmaps[0])); - TEST_ASSERT(bitmaps, "Failed to allocate bitmaps array."); - - for (i = 0; i < slots; i++) { - bitmaps[i] = bitmap_zalloc(pages_per_slot); - TEST_ASSERT(bitmaps[i], "Failed to allocate slot bitmap."); - } - - return bitmaps; -} - -static void free_bitmaps(unsigned long *bitmaps[], int slots) -{ - int i; - - for (i = 0; i < slots; i++) - free(bitmaps[i]); - - free(bitmaps); -} - static void run_test(enum vm_guest_mode mode, void *arg) { struct test_params *p = arg; @@ -236,7 +165,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) host_num_pages = vm_num_host_pages(mode, guest_num_pages); pages_per_slot = host_num_pages / p->slots; - bitmaps = alloc_bitmaps(p->slots, pages_per_slot); + bitmaps = memstress_alloc_bitmaps(p->slots, pages_per_slot); if (dirty_log_manual_caps) vm_enable_cap(vm, KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2, @@ -277,7 +206,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) /* Enable dirty logging */ clock_gettime(CLOCK_MONOTONIC, &start); - enable_dirty_logging(vm, p->slots); + memstress_enable_dirty_logging(vm, p->slots); ts_diff = timespec_elapsed(start); pr_info("Enabling dirty logging time: %ld.%.9lds\n\n", ts_diff.tv_sec, ts_diff.tv_nsec); @@ -306,7 +235,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) iteration, ts_diff.tv_sec, ts_diff.tv_nsec); clock_gettime(CLOCK_MONOTONIC, &start); - get_dirty_log(vm, bitmaps, p->slots); + memstress_get_dirty_log(vm, bitmaps, p->slots); ts_diff = timespec_elapsed(start); get_dirty_log_total = timespec_add(get_dirty_log_total, ts_diff); @@ -315,7 +244,8 @@ static void run_test(enum vm_guest_mode mode, void *arg) if (dirty_log_manual_caps) { clock_gettime(CLOCK_MONOTONIC, &start); - clear_dirty_log(vm, bitmaps, p->slots, pages_per_slot); + memstress_clear_dirty_log(vm, bitmaps, p->slots, + pages_per_slot); ts_diff = timespec_elapsed(start); clear_dirty_log_total = timespec_add(clear_dirty_log_total, ts_diff); @@ -334,7 +264,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) /* Disable dirty logging */ clock_gettime(CLOCK_MONOTONIC, &start); - disable_dirty_logging(vm, p->slots); + memstress_disable_dirty_logging(vm, p->slots); ts_diff = timespec_elapsed(start); pr_info("Disabling dirty logging time: %ld.%.9lds\n", ts_diff.tv_sec, ts_diff.tv_nsec); @@ -359,7 +289,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) clear_dirty_log_total.tv_nsec, avg.tv_sec, avg.tv_nsec); } - free_bitmaps(bitmaps, p->slots); + memstress_free_bitmaps(bitmaps, p->slots); arch_cleanup_vm(vm); memstress_destroy_vm(vm); } diff --git a/tools/testing/selftests/kvm/include/memstress.h b/tools/testing/selftests/kvm/include/memstress.h index 72e3e358ef7bd..ce4e603050eaa 100644 --- a/tools/testing/selftests/kvm/include/memstress.h +++ b/tools/testing/selftests/kvm/include/memstress.h @@ -72,4 +72,12 @@ void memstress_guest_code(uint32_t vcpu_id); uint64_t memstress_nested_pages(int nr_vcpus); void memstress_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_vcpu *vcpus[]); +void memstress_enable_dirty_logging(struct kvm_vm *vm, int slots); +void memstress_disable_dirty_logging(struct kvm_vm *vm, int slots); +void memstress_get_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], int slots); +void memstress_clear_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], + int slots, uint64_t pages_per_slot); +unsigned long **memstress_alloc_bitmaps(int slots, uint64_t pages_per_slot); +void memstress_free_bitmaps(unsigned long *bitmaps[], int slots); + #endif /* SELFTEST_KVM_MEMSTRESS_H */ diff --git a/tools/testing/selftests/kvm/lib/memstress.c b/tools/testing/selftests/kvm/lib/memstress.c index 5f1d3173c238c..3632956c6bcf5 100644 --- a/tools/testing/selftests/kvm/lib/memstress.c +++ b/tools/testing/selftests/kvm/lib/memstress.c @@ -5,6 +5,7 @@ #define _GNU_SOURCE #include <inttypes.h> +#include <linux/bitmap.h> #include "kvm_util.h" #include "memstress.h" @@ -320,3 +321,74 @@ void memstress_join_vcpu_threads(int nr_vcpus) for (i = 0; i < nr_vcpus; i++) pthread_join(vcpu_threads[i].thread, NULL); } + +static void toggle_dirty_logging(struct kvm_vm *vm, int slots, bool enable) +{ + int i; + + for (i = 0; i < slots; i++) { + int slot = MEMSTRESS_MEM_SLOT_INDEX + i; + int flags = enable ? KVM_MEM_LOG_DIRTY_PAGES : 0; + + vm_mem_region_set_flags(vm, slot, flags); + } +} + +void memstress_enable_dirty_logging(struct kvm_vm *vm, int slots) +{ + toggle_dirty_logging(vm, slots, true); +} + +void memstress_disable_dirty_logging(struct kvm_vm *vm, int slots) +{ + toggle_dirty_logging(vm, slots, false); +} + +void memstress_get_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], int slots) +{ + int i; + + for (i = 0; i < slots; i++) { + int slot = MEMSTRESS_MEM_SLOT_INDEX + i; + + kvm_vm_get_dirty_log(vm, slot, bitmaps[i]); + } +} + +void memstress_clear_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], + int slots, uint64_t pages_per_slot) +{ + int i; + + for (i = 0; i < slots; i++) { + int slot = MEMSTRESS_MEM_SLOT_INDEX + i; + + kvm_vm_clear_dirty_log(vm, slot, bitmaps[i], 0, pages_per_slot); + } +} + +unsigned long **memstress_alloc_bitmaps(int slots, uint64_t pages_per_slot) +{ + unsigned long **bitmaps; + int i; + + bitmaps = malloc(slots * sizeof(bitmaps[0])); + TEST_ASSERT(bitmaps, "Failed to allocate bitmaps array."); + + for (i = 0; i < slots; i++) { + bitmaps[i] = bitmap_zalloc(pages_per_slot); + TEST_ASSERT(bitmaps[i], "Failed to allocate slot bitmap."); + } + + return bitmaps; +} + +void memstress_free_bitmaps(unsigned long *bitmaps[], int slots) +{ + int i; + + for (i = 0; i < slots; i++) + free(bitmaps[i]); + + free(bitmaps); +} -- 2.39.1.405.gd4c25cc71f-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] selftests: KVM: Move dirty logging functions to memstress.(c|h) 2023-01-23 19:03 ` [PATCH v2 1/2] selftests: KVM: Move dirty logging functions to memstress.(c|h) Ben Gardon @ 2023-01-23 21:03 ` Vipin Sharma 0 siblings, 0 replies; 6+ messages in thread From: Vipin Sharma @ 2023-01-23 21:03 UTC (permalink / raw) To: Ben Gardon Cc: linux-kernel, kvm, Paolo Bonzini, Peter Xu, Sean Christopherson, David Matlack, Ricardo Koller On Mon, Jan 23, 2023 at 11:03 AM Ben Gardon <bgardon@google.com> wrote: > > Move some helper functions from dirty_log_perf_test.c to the memstress > library so that they can be used in a future commit which tests page > splitting during dirty logging. > > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > .../selftests/kvm/dirty_log_perf_test.c | 84 ++----------------- > .../testing/selftests/kvm/include/memstress.h | 8 ++ > tools/testing/selftests/kvm/lib/memstress.c | 72 ++++++++++++++++ > 3 files changed, 87 insertions(+), 77 deletions(-) > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c > index e9d6d1aecf89c..416719e205183 100644 > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > @@ -136,77 +136,6 @@ struct test_params { > bool random_access; > }; > > -static void toggle_dirty_logging(struct kvm_vm *vm, int slots, bool enable) > -{ > - int i; > - > - for (i = 0; i < slots; i++) { > - int slot = MEMSTRESS_MEM_SLOT_INDEX + i; > - int flags = enable ? KVM_MEM_LOG_DIRTY_PAGES : 0; > - > - vm_mem_region_set_flags(vm, slot, flags); > - } > -} > - > -static inline void enable_dirty_logging(struct kvm_vm *vm, int slots) > -{ > - toggle_dirty_logging(vm, slots, true); > -} > - > -static inline void disable_dirty_logging(struct kvm_vm *vm, int slots) > -{ > - toggle_dirty_logging(vm, slots, false); > -} > - > -static void get_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], int slots) > -{ > - int i; > - > - for (i = 0; i < slots; i++) { > - int slot = MEMSTRESS_MEM_SLOT_INDEX + i; > - > - kvm_vm_get_dirty_log(vm, slot, bitmaps[i]); > - } > -} > - > -static void clear_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], > - int slots, uint64_t pages_per_slot) > -{ > - int i; > - > - for (i = 0; i < slots; i++) { > - int slot = MEMSTRESS_MEM_SLOT_INDEX + i; > - > - kvm_vm_clear_dirty_log(vm, slot, bitmaps[i], 0, pages_per_slot); > - } > -} > - > -static unsigned long **alloc_bitmaps(int slots, uint64_t pages_per_slot) > -{ > - unsigned long **bitmaps; > - int i; > - > - bitmaps = malloc(slots * sizeof(bitmaps[0])); > - TEST_ASSERT(bitmaps, "Failed to allocate bitmaps array."); > - > - for (i = 0; i < slots; i++) { > - bitmaps[i] = bitmap_zalloc(pages_per_slot); > - TEST_ASSERT(bitmaps[i], "Failed to allocate slot bitmap."); > - } > - > - return bitmaps; > -} > - > -static void free_bitmaps(unsigned long *bitmaps[], int slots) > -{ > - int i; > - > - for (i = 0; i < slots; i++) > - free(bitmaps[i]); > - > - free(bitmaps); > -} > - > static void run_test(enum vm_guest_mode mode, void *arg) > { > struct test_params *p = arg; > @@ -236,7 +165,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) > host_num_pages = vm_num_host_pages(mode, guest_num_pages); > pages_per_slot = host_num_pages / p->slots; > > - bitmaps = alloc_bitmaps(p->slots, pages_per_slot); > + bitmaps = memstress_alloc_bitmaps(p->slots, pages_per_slot); > > if (dirty_log_manual_caps) > vm_enable_cap(vm, KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2, > @@ -277,7 +206,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) > > /* Enable dirty logging */ > clock_gettime(CLOCK_MONOTONIC, &start); > - enable_dirty_logging(vm, p->slots); > + memstress_enable_dirty_logging(vm, p->slots); > ts_diff = timespec_elapsed(start); > pr_info("Enabling dirty logging time: %ld.%.9lds\n\n", > ts_diff.tv_sec, ts_diff.tv_nsec); > @@ -306,7 +235,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) > iteration, ts_diff.tv_sec, ts_diff.tv_nsec); > > clock_gettime(CLOCK_MONOTONIC, &start); > - get_dirty_log(vm, bitmaps, p->slots); > + memstress_get_dirty_log(vm, bitmaps, p->slots); > ts_diff = timespec_elapsed(start); > get_dirty_log_total = timespec_add(get_dirty_log_total, > ts_diff); > @@ -315,7 +244,8 @@ static void run_test(enum vm_guest_mode mode, void *arg) > > if (dirty_log_manual_caps) { > clock_gettime(CLOCK_MONOTONIC, &start); > - clear_dirty_log(vm, bitmaps, p->slots, pages_per_slot); > + memstress_clear_dirty_log(vm, bitmaps, p->slots, > + pages_per_slot); > ts_diff = timespec_elapsed(start); > clear_dirty_log_total = timespec_add(clear_dirty_log_total, > ts_diff); > @@ -334,7 +264,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) > > /* Disable dirty logging */ > clock_gettime(CLOCK_MONOTONIC, &start); > - disable_dirty_logging(vm, p->slots); > + memstress_disable_dirty_logging(vm, p->slots); > ts_diff = timespec_elapsed(start); > pr_info("Disabling dirty logging time: %ld.%.9lds\n", > ts_diff.tv_sec, ts_diff.tv_nsec); > @@ -359,7 +289,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) > clear_dirty_log_total.tv_nsec, avg.tv_sec, avg.tv_nsec); > } > > - free_bitmaps(bitmaps, p->slots); > + memstress_free_bitmaps(bitmaps, p->slots); > arch_cleanup_vm(vm); > memstress_destroy_vm(vm); > } > diff --git a/tools/testing/selftests/kvm/include/memstress.h b/tools/testing/selftests/kvm/include/memstress.h > index 72e3e358ef7bd..ce4e603050eaa 100644 > --- a/tools/testing/selftests/kvm/include/memstress.h > +++ b/tools/testing/selftests/kvm/include/memstress.h > @@ -72,4 +72,12 @@ void memstress_guest_code(uint32_t vcpu_id); > uint64_t memstress_nested_pages(int nr_vcpus); > void memstress_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_vcpu *vcpus[]); > > +void memstress_enable_dirty_logging(struct kvm_vm *vm, int slots); > +void memstress_disable_dirty_logging(struct kvm_vm *vm, int slots); > +void memstress_get_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], int slots); > +void memstress_clear_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], > + int slots, uint64_t pages_per_slot); > +unsigned long **memstress_alloc_bitmaps(int slots, uint64_t pages_per_slot); > +void memstress_free_bitmaps(unsigned long *bitmaps[], int slots); > + > #endif /* SELFTEST_KVM_MEMSTRESS_H */ > diff --git a/tools/testing/selftests/kvm/lib/memstress.c b/tools/testing/selftests/kvm/lib/memstress.c > index 5f1d3173c238c..3632956c6bcf5 100644 > --- a/tools/testing/selftests/kvm/lib/memstress.c > +++ b/tools/testing/selftests/kvm/lib/memstress.c > @@ -5,6 +5,7 @@ > #define _GNU_SOURCE > > #include <inttypes.h> > +#include <linux/bitmap.h> > > #include "kvm_util.h" > #include "memstress.h" > @@ -320,3 +321,74 @@ void memstress_join_vcpu_threads(int nr_vcpus) > for (i = 0; i < nr_vcpus; i++) > pthread_join(vcpu_threads[i].thread, NULL); > } > + > +static void toggle_dirty_logging(struct kvm_vm *vm, int slots, bool enable) > +{ > + int i; > + > + for (i = 0; i < slots; i++) { > + int slot = MEMSTRESS_MEM_SLOT_INDEX + i; > + int flags = enable ? KVM_MEM_LOG_DIRTY_PAGES : 0; > + > + vm_mem_region_set_flags(vm, slot, flags); > + } > +} > + > +void memstress_enable_dirty_logging(struct kvm_vm *vm, int slots) > +{ > + toggle_dirty_logging(vm, slots, true); > +} > + > +void memstress_disable_dirty_logging(struct kvm_vm *vm, int slots) > +{ > + toggle_dirty_logging(vm, slots, false); > +} Very simple functions, maybe just make it inline in the memstress header. > + > +void memstress_get_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], int slots) > +{ > + int i; > + > + for (i = 0; i < slots; i++) { > + int slot = MEMSTRESS_MEM_SLOT_INDEX + i; > + > + kvm_vm_get_dirty_log(vm, slot, bitmaps[i]); > + } > +} > + > +void memstress_clear_dirty_log(struct kvm_vm *vm, unsigned long *bitmaps[], > + int slots, uint64_t pages_per_slot) > +{ > + int i; > + > + for (i = 0; i < slots; i++) { > + int slot = MEMSTRESS_MEM_SLOT_INDEX + i; > + > + kvm_vm_clear_dirty_log(vm, slot, bitmaps[i], 0, pages_per_slot); > + } > +} > + > +unsigned long **memstress_alloc_bitmaps(int slots, uint64_t pages_per_slot) > +{ > + unsigned long **bitmaps; > + int i; > + > + bitmaps = malloc(slots * sizeof(bitmaps[0])); > + TEST_ASSERT(bitmaps, "Failed to allocate bitmaps array."); > + > + for (i = 0; i < slots; i++) { > + bitmaps[i] = bitmap_zalloc(pages_per_slot); > + TEST_ASSERT(bitmaps[i], "Failed to allocate slot bitmap."); > + } > + > + return bitmaps; > +} > + > +void memstress_free_bitmaps(unsigned long *bitmaps[], int slots) > +{ > + int i; > + > + for (i = 0; i < slots; i++) > + free(bitmaps[i]); > + > + free(bitmaps); > +} > -- > 2.39.1.405.gd4c25cc71f-goog > Other than the inline suggestion, which is optional. Reviewed-by: Vipin Sharma <vipinsh@google.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] selftests: KVM: Add page splitting test 2023-01-23 19:03 [PATCH v2 0/2] selftests: KVM: Add a test for eager page splitting Ben Gardon 2023-01-23 19:03 ` [PATCH v2 1/2] selftests: KVM: Move dirty logging functions to memstress.(c|h) Ben Gardon @ 2023-01-23 19:03 ` Ben Gardon 2023-01-23 21:32 ` Vipin Sharma 1 sibling, 1 reply; 6+ messages in thread From: Ben Gardon @ 2023-01-23 19:03 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Peter Xu, Sean Christopherson, David Matlack, Vipin Sharma, Ricardo Koller, Ben Gardon Add a test for page splitting during dirty logging and for hugepage recovery after dirty logging. Page splitting represents non-trivial behavior, which is complicated by MANUAL_PROTECT mode, which causes pages to be split on the first clear, instead of when dirty logging is enabled. Add a test which makes asserions about page counts to help define the expected behavior of page splitting and to provid needed coverage of the behavior. This also helps ensure that a failure in eager page splitting is not covered up by splitting in the vCPU path. Tested by running the test on an Intel Haswell machine w/wo MANUAL_PROTECT. Signed-off-by: Ben Gardon <bgardon@google.com> --- tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/include/kvm_util_base.h | 1 + tools/testing/selftests/kvm/lib/kvm_util.c | 5 + .../kvm/x86_64/page_splitting_test.c | 278 ++++++++++++++++++ 4 files changed, 285 insertions(+) create mode 100644 tools/testing/selftests/kvm/x86_64/page_splitting_test.c diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 1750f91dd9362..057ebd709e77a 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -76,6 +76,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/kvm_pv_test TEST_GEN_PROGS_x86_64 += x86_64/mmio_warning_test TEST_GEN_PROGS_x86_64 += x86_64/monitor_mwait_test TEST_GEN_PROGS_x86_64 += x86_64/nested_exceptions_test +TEST_GEN_PROGS_x86_64 += x86_64/page_splitting_test TEST_GEN_PROGS_x86_64 += x86_64/platform_info_test TEST_GEN_PROGS_x86_64 += x86_64/pmu_event_filter_test TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index fbc2a79369b8b..a089c356f354e 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -213,6 +213,7 @@ extern const struct vm_guest_mode_params vm_guest_mode_params[]; int open_path_or_exit(const char *path, int flags); int open_kvm_dev_path_or_exit(void); +bool get_kvm_param_bool(const char *param); bool get_kvm_intel_param_bool(const char *param); bool get_kvm_amd_param_bool(const char *param); diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 56d5ea949cbbe..fa6d69f731990 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -80,6 +80,11 @@ static bool get_module_param_bool(const char *module_name, const char *param) TEST_FAIL("Unrecognized value '%c' for boolean module param", value); } +bool get_kvm_param_bool(const char *param) +{ + return get_module_param_bool("kvm", param); +} + bool get_kvm_intel_param_bool(const char *param) { return get_module_param_bool("kvm_intel", param); diff --git a/tools/testing/selftests/kvm/x86_64/page_splitting_test.c b/tools/testing/selftests/kvm/x86_64/page_splitting_test.c new file mode 100644 index 0000000000000..2b4a28e6a95de --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/page_splitting_test.c @@ -0,0 +1,278 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KVM dirty logging page splitting test + * + * Based on dirty_log_perf.c + * + * Copyright (C) 2018, Red Hat, Inc. + * Copyright (C) 2020, Google, Inc. + */ + +#include <stdio.h> +#include <stdlib.h> +#include <time.h> +#include <pthread.h> +#include <linux/bitmap.h> + +#include "kvm_util.h" +#include "test_util.h" +#include "memstress.h" +#include "guest_modes.h" + +#define VCPUS 2 +#define SLOTS 2 +#define ITERATIONS 2 + +static uint64_t guest_percpu_mem_size = DEFAULT_PER_VCPU_MEM_SIZE; + +static enum vm_mem_backing_src_type backing_src = VM_MEM_SRC_ANONYMOUS_HUGETLB; + +static u64 dirty_log_manual_caps; +static bool host_quit; +static int iteration; +static int vcpu_last_completed_iteration[KVM_MAX_VCPUS]; + +struct kvm_page_stats { + uint64_t pages_4k; + uint64_t pages_2m; + uint64_t pages_1g; + uint64_t hugepages; +}; + +static void get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage) +{ + stats->pages_4k = vm_get_stat(vm, "pages_4k"); + stats->pages_2m = vm_get_stat(vm, "pages_2m"); + stats->pages_1g = vm_get_stat(vm, "pages_1g"); + stats->hugepages = stats->pages_2m + stats->pages_1g; + + pr_debug("\nGetting stats after %s: 4K: %ld 2M: %ld 1G: %ld huge: %ld\n", + stage, stats->pages_4k, stats->pages_2m, stats->pages_1g, + stats->hugepages); +} + +static void run_vcpus_get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage) +{ + int i; + + iteration++; + for (i = 0; i < VCPUS; i++) { + while (READ_ONCE(vcpu_last_completed_iteration[i]) != + iteration) + ; + } + + get_page_stats(vm, stats, stage); +} + +static void vcpu_worker(struct memstress_vcpu_args *vcpu_args) +{ + struct kvm_vcpu *vcpu = vcpu_args->vcpu; + int vcpu_idx = vcpu_args->vcpu_idx; + struct kvm_run *run; + + run = vcpu->run; + + while (!READ_ONCE(host_quit)) { + int current_iteration = READ_ONCE(iteration); + + vcpu_run(vcpu); + + TEST_ASSERT(get_ucall(vcpu, NULL) == UCALL_SYNC, + "Invalid guest sync status: exit_reason=%s\n", + exit_reason_str(run->exit_reason)); + + vcpu_last_completed_iteration[vcpu_idx] = current_iteration; + + /* Wait for the start of the next iteration to be signaled. */ + while (current_iteration == READ_ONCE(iteration) && + READ_ONCE(iteration) >= 0 && + !READ_ONCE(host_quit)) + ; + } +} + +static void run_test(enum vm_guest_mode mode, void *unused) +{ + struct kvm_vm *vm; + unsigned long **bitmaps; + uint64_t guest_num_pages; + uint64_t host_num_pages; + uint64_t pages_per_slot; + int i; + uint64_t total_4k_pages; + struct kvm_page_stats stats_populated; + struct kvm_page_stats stats_dirty_logging_enabled; + struct kvm_page_stats stats_dirty_pass[ITERATIONS]; + struct kvm_page_stats stats_clear_pass[ITERATIONS]; + struct kvm_page_stats stats_dirty_logging_disabled; + struct kvm_page_stats stats_repopulated; + + vm = memstress_create_vm(mode, VCPUS, guest_percpu_mem_size, + SLOTS, backing_src, false); + + guest_num_pages = (VCPUS * guest_percpu_mem_size) >> vm->page_shift; + guest_num_pages = vm_adjust_num_guest_pages(mode, guest_num_pages); + host_num_pages = vm_num_host_pages(mode, guest_num_pages); + pages_per_slot = host_num_pages / SLOTS; + + bitmaps = memstress_alloc_bitmaps(SLOTS, pages_per_slot); + + if (dirty_log_manual_caps) + vm_enable_cap(vm, KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2, + dirty_log_manual_caps); + + /* Start the iterations */ + iteration = -1; + host_quit = false; + + for (i = 0; i < VCPUS; i++) + vcpu_last_completed_iteration[i] = -1; + + memstress_start_vcpu_threads(VCPUS, vcpu_worker); + + run_vcpus_get_page_stats(vm, &stats_populated, "populating memory"); + + /* Enable dirty logging */ + memstress_enable_dirty_logging(vm, SLOTS); + + get_page_stats(vm, &stats_dirty_logging_enabled, "enabling dirty logging"); + + while (iteration < ITERATIONS) { + run_vcpus_get_page_stats(vm, &stats_dirty_pass[iteration - 1], + "dirtying memory"); + + memstress_get_dirty_log(vm, bitmaps, SLOTS); + + if (dirty_log_manual_caps) { + memstress_clear_dirty_log(vm, bitmaps, SLOTS, pages_per_slot); + + get_page_stats(vm, &stats_clear_pass[iteration - 1], "clearing dirty log"); + } + } + + /* Disable dirty logging */ + memstress_disable_dirty_logging(vm, SLOTS); + + get_page_stats(vm, &stats_dirty_logging_disabled, "disabling dirty logging"); + + /* Run vCPUs again to fault pages back in. */ + run_vcpus_get_page_stats(vm, &stats_repopulated, "repopulating memory"); + + /* + * Tell the vCPU threads to quit. No need to manually check that vCPUs + * have stopped running after disabling dirty logging, the join will + * wait for them to exit. + */ + host_quit = true; + memstress_join_vcpu_threads(VCPUS); + + memstress_free_bitmaps(bitmaps, SLOTS); + memstress_destroy_vm(vm); + + /* Make assertions about the page counts. */ + total_4k_pages = stats_populated.pages_4k; + total_4k_pages += stats_populated.pages_2m * 512; + total_4k_pages += stats_populated.pages_1g * 512 * 512; + + /* + * Check that all huge pages were split. Since large pages can only + * exist in the data slot, and the vCPUs should have dirtied all pages + * in the data slot, there should be no huge pages left after splitting. + * Splitting happens at dirty log enable time without + * KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 and after the first clear pass + * with that capability. + */ + if (dirty_log_manual_caps) { + TEST_ASSERT(stats_clear_pass[0].hugepages == 0, + "Unexpected huge page count after splitting. Expected 0, got %ld", + stats_clear_pass[0].hugepages); + TEST_ASSERT(stats_clear_pass[0].pages_4k == total_4k_pages, + "All memory should be mapped at 4k. Expected %ld 4k pages, got %ld", + total_4k_pages, stats_clear_pass[0].pages_4k); + TEST_ASSERT(stats_dirty_logging_enabled.hugepages == stats_populated.hugepages, + "Huge page count should not have changed from enabling dirty logging. Expected %ld, got %ld", + stats_populated.hugepages, stats_dirty_logging_enabled.hugepages); + } else { + TEST_ASSERT(stats_dirty_logging_enabled.hugepages == 0, + "Unexpected huge page count after splitting. Expected 0, got %ld", + stats_dirty_logging_enabled.hugepages); + TEST_ASSERT(stats_dirty_logging_enabled.pages_4k == total_4k_pages, + "All memory should be mapped at 4k. Expected %ld 4k pages, got %ld", + total_4k_pages, stats_dirty_logging_enabled.pages_4k); + } + + /* + * Once dirty logging is disabled and the vCPUs have touched all their + * memory again, the page counts should be the same as they were + * right after initial population of memory. + */ + TEST_ASSERT(stats_populated.pages_4k == stats_repopulated.pages_4k, + "4k page count did not return to its initial value after " + "dirty logging. Expected %ld, got %ld", + stats_populated.pages_4k, stats_repopulated.pages_4k); + TEST_ASSERT(stats_populated.pages_2m == stats_repopulated.pages_2m, + "2m page count did not return to its initial value after " + "dirty logging. Expected %ld, got %ld", + stats_populated.pages_2m, stats_repopulated.pages_2m); + TEST_ASSERT(stats_populated.pages_1g == stats_repopulated.pages_1g, + "1g page count did not return to its initial value after " + "dirty logging. Expected %ld, got %ld", + stats_populated.pages_1g, stats_repopulated.pages_1g); +} + +static void help(char *name) +{ + puts(""); + printf("usage: %s [-h] [-b vcpu bytes] [-s mem type]\n", + name); + puts(""); + printf(" -b: specify the size of the memory region which should be\n" + " dirtied by each vCPU. e.g. 10M or 3G.\n" + " (default: 1G)\n"); + backing_src_help("-s"); + puts(""); + exit(0); +} + +int main(int argc, char *argv[]) +{ + int opt; + + TEST_REQUIRE(get_kvm_param_bool("eager_page_split")); + TEST_REQUIRE(get_kvm_param_bool("tdp_mmu")); + + while ((opt = getopt(argc, argv, "b:hs:")) != -1) { + switch (opt) { + case 'b': + guest_percpu_mem_size = parse_size(optarg); + break; + case 's': + backing_src = parse_backing_src_type(optarg); + break; + case 'h': + default: + help(argv[0]); + break; + } + } + + if (!is_backing_src_hugetlb(backing_src)) { + pr_info("This test will only work reliably with HugeTLB memory. " + "It can work with THP, but that is best effort."); + return KSFT_SKIP; + } + + guest_modes_append_default(); + + dirty_log_manual_caps = 0; + for_each_guest_mode(run_test, NULL); + + dirty_log_manual_caps = + kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2); + dirty_log_manual_caps &= (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | + KVM_DIRTY_LOG_INITIALLY_SET); + for_each_guest_mode(run_test, NULL); + + return 0; +} -- 2.39.1.405.gd4c25cc71f-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] selftests: KVM: Add page splitting test 2023-01-23 19:03 ` [PATCH v2 2/2] selftests: KVM: Add page splitting test Ben Gardon @ 2023-01-23 21:32 ` Vipin Sharma 2023-01-23 23:42 ` Ben Gardon 0 siblings, 1 reply; 6+ messages in thread From: Vipin Sharma @ 2023-01-23 21:32 UTC (permalink / raw) To: Ben Gardon Cc: linux-kernel, kvm, Paolo Bonzini, Peter Xu, Sean Christopherson, David Matlack, Ricardo Koller On Mon, Jan 23, 2023 at 11:03 AM Ben Gardon <bgardon@google.com> wrote: > > Add a test for page splitting during dirty logging and for hugepage > recovery after dirty logging. > > Page splitting represents non-trivial behavior, which is complicated > by MANUAL_PROTECT mode, which causes pages to be split on the first > clear, instead of when dirty logging is enabled. > > Add a test which makes asserions about page counts to help define the nit: assertions > expected behavior of page splitting and to provid needed coverage of the nit: provide > behavior. This also helps ensure that a failure in eager page splitting > is not covered up by splitting in the vCPU path. > > Tested by running the test on an Intel Haswell machine w/wo > MANUAL_PROTECT. > > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > tools/testing/selftests/kvm/Makefile | 1 + > .../selftests/kvm/include/kvm_util_base.h | 1 + > tools/testing/selftests/kvm/lib/kvm_util.c | 5 + > .../kvm/x86_64/page_splitting_test.c | 278 ++++++++++++++++++ > 4 files changed, 285 insertions(+) > create mode 100644 tools/testing/selftests/kvm/x86_64/page_splitting_test.c > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > index 1750f91dd9362..057ebd709e77a 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -76,6 +76,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/kvm_pv_test > TEST_GEN_PROGS_x86_64 += x86_64/mmio_warning_test > TEST_GEN_PROGS_x86_64 += x86_64/monitor_mwait_test > TEST_GEN_PROGS_x86_64 += x86_64/nested_exceptions_test > +TEST_GEN_PROGS_x86_64 += x86_64/page_splitting_test Should the test be named dirty_log_page_splitting_test or dirty_log_page_split_and_recovery_test? page_splitting_test name is very generic and does not convey much information. > TEST_GEN_PROGS_x86_64 += x86_64/platform_info_test > TEST_GEN_PROGS_x86_64 += x86_64/pmu_event_filter_test > TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index fbc2a79369b8b..a089c356f354e 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -213,6 +213,7 @@ extern const struct vm_guest_mode_params vm_guest_mode_params[]; > int open_path_or_exit(const char *path, int flags); > int open_kvm_dev_path_or_exit(void); > > +bool get_kvm_param_bool(const char *param); > bool get_kvm_intel_param_bool(const char *param); > bool get_kvm_amd_param_bool(const char *param); > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index 56d5ea949cbbe..fa6d69f731990 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -80,6 +80,11 @@ static bool get_module_param_bool(const char *module_name, const char *param) > TEST_FAIL("Unrecognized value '%c' for boolean module param", value); > } > > +bool get_kvm_param_bool(const char *param) > +{ > + return get_module_param_bool("kvm", param); > +} > + > bool get_kvm_intel_param_bool(const char *param) > { > return get_module_param_bool("kvm_intel", param); > diff --git a/tools/testing/selftests/kvm/x86_64/page_splitting_test.c b/tools/testing/selftests/kvm/x86_64/page_splitting_test.c > new file mode 100644 > index 0000000000000..2b4a28e6a95de > --- /dev/null > +++ b/tools/testing/selftests/kvm/x86_64/page_splitting_test.c > @@ -0,0 +1,278 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * KVM dirty logging page splitting test > + * > + * Based on dirty_log_perf.c > + * > + * Copyright (C) 2018, Red Hat, Inc. Delete? > + * Copyright (C) 2020, Google, Inc. 2023 > + */ > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <time.h> > +#include <pthread.h> > +#include <linux/bitmap.h> > + > +#include "kvm_util.h" > +#include "test_util.h" > +#include "memstress.h" > +#include "guest_modes.h" > + > +#define VCPUS 2 > +#define SLOTS 2 > +#define ITERATIONS 2 > + > +static uint64_t guest_percpu_mem_size = DEFAULT_PER_VCPU_MEM_SIZE; > + > +static enum vm_mem_backing_src_type backing_src = VM_MEM_SRC_ANONYMOUS_HUGETLB; > + > +static u64 dirty_log_manual_caps; > +static bool host_quit; > +static int iteration; > +static int vcpu_last_completed_iteration[KVM_MAX_VCPUS]; > + > +struct kvm_page_stats { > + uint64_t pages_4k; > + uint64_t pages_2m; > + uint64_t pages_1g; > + uint64_t hugepages; > +}; > + > +static void get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage) > +{ > + stats->pages_4k = vm_get_stat(vm, "pages_4k"); > + stats->pages_2m = vm_get_stat(vm, "pages_2m"); > + stats->pages_1g = vm_get_stat(vm, "pages_1g"); > + stats->hugepages = stats->pages_2m + stats->pages_1g; > + > + pr_debug("\nGetting stats after %s: 4K: %ld 2M: %ld 1G: %ld huge: %ld\n", > + stage, stats->pages_4k, stats->pages_2m, stats->pages_1g, > + stats->hugepages); > +} > + > +static void run_vcpus_get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage) > +{ > + int i; > + > + iteration++; > + for (i = 0; i < VCPUS; i++) { > + while (READ_ONCE(vcpu_last_completed_iteration[i]) != > + iteration) > + ; > + } > + > + get_page_stats(vm, stats, stage); > +} > + > +static void vcpu_worker(struct memstress_vcpu_args *vcpu_args) > +{ > + struct kvm_vcpu *vcpu = vcpu_args->vcpu; > + int vcpu_idx = vcpu_args->vcpu_idx; > + struct kvm_run *run; > + > + run = vcpu->run; > + > + while (!READ_ONCE(host_quit)) { > + int current_iteration = READ_ONCE(iteration); > + > + vcpu_run(vcpu); > + > + TEST_ASSERT(get_ucall(vcpu, NULL) == UCALL_SYNC, > + "Invalid guest sync status: exit_reason=%s\n", > + exit_reason_str(run->exit_reason)); > + > + vcpu_last_completed_iteration[vcpu_idx] = current_iteration; > + > + /* Wait for the start of the next iteration to be signaled. */ > + while (current_iteration == READ_ONCE(iteration) && > + READ_ONCE(iteration) >= 0 && > + !READ_ONCE(host_quit)) > + ; > + } > +} > + > +static void run_test(enum vm_guest_mode mode, void *unused) > +{ > + struct kvm_vm *vm; > + unsigned long **bitmaps; > + uint64_t guest_num_pages; > + uint64_t host_num_pages; > + uint64_t pages_per_slot; > + int i; > + uint64_t total_4k_pages; > + struct kvm_page_stats stats_populated; > + struct kvm_page_stats stats_dirty_logging_enabled; > + struct kvm_page_stats stats_dirty_pass[ITERATIONS]; > + struct kvm_page_stats stats_clear_pass[ITERATIONS]; > + struct kvm_page_stats stats_dirty_logging_disabled; > + struct kvm_page_stats stats_repopulated; > + > + vm = memstress_create_vm(mode, VCPUS, guest_percpu_mem_size, > + SLOTS, backing_src, false); > + > + guest_num_pages = (VCPUS * guest_percpu_mem_size) >> vm->page_shift; > + guest_num_pages = vm_adjust_num_guest_pages(mode, guest_num_pages); > + host_num_pages = vm_num_host_pages(mode, guest_num_pages); > + pages_per_slot = host_num_pages / SLOTS; > + > + bitmaps = memstress_alloc_bitmaps(SLOTS, pages_per_slot); > + > + if (dirty_log_manual_caps) > + vm_enable_cap(vm, KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2, > + dirty_log_manual_caps); > + > + /* Start the iterations */ > + iteration = -1; > + host_quit = false; > + > + for (i = 0; i < VCPUS; i++) > + vcpu_last_completed_iteration[i] = -1; > + > + memstress_start_vcpu_threads(VCPUS, vcpu_worker); > + > + run_vcpus_get_page_stats(vm, &stats_populated, "populating memory"); > + > + /* Enable dirty logging */ > + memstress_enable_dirty_logging(vm, SLOTS); > + > + get_page_stats(vm, &stats_dirty_logging_enabled, "enabling dirty logging"); > + > + while (iteration < ITERATIONS) { > + run_vcpus_get_page_stats(vm, &stats_dirty_pass[iteration - 1], > + "dirtying memory"); > + > + memstress_get_dirty_log(vm, bitmaps, SLOTS); > + > + if (dirty_log_manual_caps) { > + memstress_clear_dirty_log(vm, bitmaps, SLOTS, pages_per_slot); > + > + get_page_stats(vm, &stats_clear_pass[iteration - 1], "clearing dirty log"); > + } > + } > + > + /* Disable dirty logging */ > + memstress_disable_dirty_logging(vm, SLOTS); > + > + get_page_stats(vm, &stats_dirty_logging_disabled, "disabling dirty logging"); > + > + /* Run vCPUs again to fault pages back in. */ > + run_vcpus_get_page_stats(vm, &stats_repopulated, "repopulating memory"); > + > + /* > + * Tell the vCPU threads to quit. No need to manually check that vCPUs > + * have stopped running after disabling dirty logging, the join will > + * wait for them to exit. > + */ > + host_quit = true; > + memstress_join_vcpu_threads(VCPUS); > + > + memstress_free_bitmaps(bitmaps, SLOTS); > + memstress_destroy_vm(vm); > + > + /* Make assertions about the page counts. */ > + total_4k_pages = stats_populated.pages_4k; > + total_4k_pages += stats_populated.pages_2m * 512; > + total_4k_pages += stats_populated.pages_1g * 512 * 512; > + > + /* > + * Check that all huge pages were split. Since large pages can only > + * exist in the data slot, and the vCPUs should have dirtied all pages > + * in the data slot, there should be no huge pages left after splitting. > + * Splitting happens at dirty log enable time without > + * KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 and after the first clear pass > + * with that capability. > + */ > + if (dirty_log_manual_caps) { > + TEST_ASSERT(stats_clear_pass[0].hugepages == 0, > + "Unexpected huge page count after splitting. Expected 0, got %ld", > + stats_clear_pass[0].hugepages); > + TEST_ASSERT(stats_clear_pass[0].pages_4k == total_4k_pages, > + "All memory should be mapped at 4k. Expected %ld 4k pages, got %ld", > + total_4k_pages, stats_clear_pass[0].pages_4k); > + TEST_ASSERT(stats_dirty_logging_enabled.hugepages == stats_populated.hugepages, > + "Huge page count should not have changed from enabling dirty logging. Expected %ld, got %ld", > + stats_populated.hugepages, stats_dirty_logging_enabled.hugepages); > + } else { > + TEST_ASSERT(stats_dirty_logging_enabled.hugepages == 0, > + "Unexpected huge page count after splitting. Expected 0, got %ld", > + stats_dirty_logging_enabled.hugepages); > + TEST_ASSERT(stats_dirty_logging_enabled.pages_4k == total_4k_pages, > + "All memory should be mapped at 4k. Expected %ld 4k pages, got %ld", > + total_4k_pages, stats_dirty_logging_enabled.pages_4k); > + } > + > + /* > + * Once dirty logging is disabled and the vCPUs have touched all their > + * memory again, the page counts should be the same as they were > + * right after initial population of memory. > + */ > + TEST_ASSERT(stats_populated.pages_4k == stats_repopulated.pages_4k, > + "4k page count did not return to its initial value after " > + "dirty logging. Expected %ld, got %ld", > + stats_populated.pages_4k, stats_repopulated.pages_4k); > + TEST_ASSERT(stats_populated.pages_2m == stats_repopulated.pages_2m, > + "2m page count did not return to its initial value after " > + "dirty logging. Expected %ld, got %ld", > + stats_populated.pages_2m, stats_repopulated.pages_2m); > + TEST_ASSERT(stats_populated.pages_1g == stats_repopulated.pages_1g, > + "1g page count did not return to its initial value after " > + "dirty logging. Expected %ld, got %ld", > + stats_populated.pages_1g, stats_repopulated.pages_1g); > +} I know David suggested in the previous version to use __ASSERT_EQ(). I will recommend using __ASSERT_EQ(). If you make the variables name meaningful then that should be sufficient in showing the expected intent. Some examples: ASSERT_EQ(after_clear_dirty_log.hugepages, 0); ASSERT_EQ(after_clear_dirty_log.pages_4k, total_4k_pages); ASSERT_EQ(after_enable_dirty_log.hugepages, initial_state.hugepages); ASSERT_EQ(initial_state.pages_4k, after_disable_dirty_logging.pages_4k); ... This makes code cleaner and error messages printed also self sufficient. > + > +static void help(char *name) > +{ > + puts(""); > + printf("usage: %s [-h] [-b vcpu bytes] [-s mem type]\n", > + name); > + puts(""); > + printf(" -b: specify the size of the memory region which should be\n" > + " dirtied by each vCPU. e.g. 10M or 3G.\n" > + " (default: 1G)\n"); > + backing_src_help("-s"); > + puts(""); > + exit(0); This should not be exit(0) if a user passed the wrong option. > +} > + > +int main(int argc, char *argv[]) > +{ > + int opt; > + > + TEST_REQUIRE(get_kvm_param_bool("eager_page_split")); > + TEST_REQUIRE(get_kvm_param_bool("tdp_mmu")); > + > + while ((opt = getopt(argc, argv, "b:hs:")) != -1) { > + switch (opt) { > + case 'b': > + guest_percpu_mem_size = parse_size(optarg); > + break; > + case 's': > + backing_src = parse_backing_src_type(optarg); > + break; > + case 'h': > + default: > + help(argv[0]); > + break; > + } > + } > + > + if (!is_backing_src_hugetlb(backing_src)) { > + pr_info("This test will only work reliably with HugeTLB memory. " > + "It can work with THP, but that is best effort."); > + return KSFT_SKIP; > + } > + > + guest_modes_append_default(); > + > + dirty_log_manual_caps = 0; > + for_each_guest_mode(run_test, NULL); > + > + dirty_log_manual_caps = > + kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2); Shouldn't the check be done in the beginning and skip the test f it is not supported? > + dirty_log_manual_caps &= (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | > + KVM_DIRTY_LOG_INITIALLY_SET); > + for_each_guest_mode(run_test, NULL); > + > + return 0; > +} > -- > 2.39.1.405.gd4c25cc71f-goog > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] selftests: KVM: Add page splitting test 2023-01-23 21:32 ` Vipin Sharma @ 2023-01-23 23:42 ` Ben Gardon 0 siblings, 0 replies; 6+ messages in thread From: Ben Gardon @ 2023-01-23 23:42 UTC (permalink / raw) To: Vipin Sharma Cc: linux-kernel, kvm, Paolo Bonzini, Peter Xu, Sean Christopherson, David Matlack, Ricardo Koller On Mon, Jan 23, 2023 at 1:32 PM Vipin Sharma <vipinsh@google.com> wrote: > > On Mon, Jan 23, 2023 at 11:03 AM Ben Gardon <bgardon@google.com> wrote: > > > > Add a test for page splitting during dirty logging and for hugepage > > recovery after dirty logging. > > > > Page splitting represents non-trivial behavior, which is complicated > > by MANUAL_PROTECT mode, which causes pages to be split on the first > > clear, instead of when dirty logging is enabled. > > > > Add a test which makes asserions about page counts to help define the > > nit: assertions > > > expected behavior of page splitting and to provid needed coverage of the > > nit: provide > > > behavior. This also helps ensure that a failure in eager page splitting > > is not covered up by splitting in the vCPU path. > > > > Tested by running the test on an Intel Haswell machine w/wo > > MANUAL_PROTECT. > > > > Signed-off-by: Ben Gardon <bgardon@google.com> > > --- > > tools/testing/selftests/kvm/Makefile | 1 + > > .../selftests/kvm/include/kvm_util_base.h | 1 + > > tools/testing/selftests/kvm/lib/kvm_util.c | 5 + > > .../kvm/x86_64/page_splitting_test.c | 278 ++++++++++++++++++ > > 4 files changed, 285 insertions(+) > > create mode 100644 tools/testing/selftests/kvm/x86_64/page_splitting_test.c > > > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > > index 1750f91dd9362..057ebd709e77a 100644 > > --- a/tools/testing/selftests/kvm/Makefile > > +++ b/tools/testing/selftests/kvm/Makefile > > @@ -76,6 +76,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/kvm_pv_test > > TEST_GEN_PROGS_x86_64 += x86_64/mmio_warning_test > > TEST_GEN_PROGS_x86_64 += x86_64/monitor_mwait_test > > TEST_GEN_PROGS_x86_64 += x86_64/nested_exceptions_test > > +TEST_GEN_PROGS_x86_64 += x86_64/page_splitting_test > > Should the test be named dirty_log_page_splitting_test or > dirty_log_page_split_and_recovery_test? > > page_splitting_test name is very generic and does not convey much information. Sure, we can do that. It's certainly a more descriptive name. > > > TEST_GEN_PROGS_x86_64 += x86_64/platform_info_test > > TEST_GEN_PROGS_x86_64 += x86_64/pmu_event_filter_test > > TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > index fbc2a79369b8b..a089c356f354e 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > @@ -213,6 +213,7 @@ extern const struct vm_guest_mode_params vm_guest_mode_params[]; > > int open_path_or_exit(const char *path, int flags); > > int open_kvm_dev_path_or_exit(void); > > > > +bool get_kvm_param_bool(const char *param); > > bool get_kvm_intel_param_bool(const char *param); > > bool get_kvm_amd_param_bool(const char *param); > > > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > > index 56d5ea949cbbe..fa6d69f731990 100644 > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > > @@ -80,6 +80,11 @@ static bool get_module_param_bool(const char *module_name, const char *param) > > TEST_FAIL("Unrecognized value '%c' for boolean module param", value); > > } > > > > +bool get_kvm_param_bool(const char *param) > > +{ > > + return get_module_param_bool("kvm", param); > > +} > > + > > bool get_kvm_intel_param_bool(const char *param) > > { > > return get_module_param_bool("kvm_intel", param); > > diff --git a/tools/testing/selftests/kvm/x86_64/page_splitting_test.c b/tools/testing/selftests/kvm/x86_64/page_splitting_test.c > > new file mode 100644 > > index 0000000000000..2b4a28e6a95de > > --- /dev/null > > +++ b/tools/testing/selftests/kvm/x86_64/page_splitting_test.c > > @@ -0,0 +1,278 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * KVM dirty logging page splitting test > > + * > > + * Based on dirty_log_perf.c > > + * > > + * Copyright (C) 2018, Red Hat, Inc. > > Delete? > > > + * Copyright (C) 2020, Google, Inc. > > 2023 As a mere engineer, copyright notices scare me and I don't know the rules for when to update things and when to add or remove credit. It seems kind of silly to have these notices in the first place so I'm generally inclined to skip them but if I'm copying the base code from an existing file and modifying it, as I did here, it seems appropriate to at least copy the copyright notice? I'm not sure when these would ever matter in practice. > > > > + */ > > + > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <time.h> > > +#include <pthread.h> > > +#include <linux/bitmap.h> > > + > > +#include "kvm_util.h" > > +#include "test_util.h" > > +#include "memstress.h" > > +#include "guest_modes.h" > > + > > +#define VCPUS 2 > > +#define SLOTS 2 > > +#define ITERATIONS 2 > > + > > +static uint64_t guest_percpu_mem_size = DEFAULT_PER_VCPU_MEM_SIZE; > > + > > +static enum vm_mem_backing_src_type backing_src = VM_MEM_SRC_ANONYMOUS_HUGETLB; > > + > > +static u64 dirty_log_manual_caps; > > +static bool host_quit; > > +static int iteration; > > +static int vcpu_last_completed_iteration[KVM_MAX_VCPUS]; > > + > > +struct kvm_page_stats { > > + uint64_t pages_4k; > > + uint64_t pages_2m; > > + uint64_t pages_1g; > > + uint64_t hugepages; > > +}; > > + > > +static void get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage) > > +{ > > + stats->pages_4k = vm_get_stat(vm, "pages_4k"); > > + stats->pages_2m = vm_get_stat(vm, "pages_2m"); > > + stats->pages_1g = vm_get_stat(vm, "pages_1g"); > > + stats->hugepages = stats->pages_2m + stats->pages_1g; > > + > > + pr_debug("\nGetting stats after %s: 4K: %ld 2M: %ld 1G: %ld huge: %ld\n", > > + stage, stats->pages_4k, stats->pages_2m, stats->pages_1g, > > + stats->hugepages); > > +} > > + > > +static void run_vcpus_get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage) > > +{ > > + int i; > > + > > + iteration++; > > + for (i = 0; i < VCPUS; i++) { > > + while (READ_ONCE(vcpu_last_completed_iteration[i]) != > > + iteration) > > + ; > > + } > > + > > + get_page_stats(vm, stats, stage); > > +} > > + > > +static void vcpu_worker(struct memstress_vcpu_args *vcpu_args) > > +{ > > + struct kvm_vcpu *vcpu = vcpu_args->vcpu; > > + int vcpu_idx = vcpu_args->vcpu_idx; > > + struct kvm_run *run; > > + > > + run = vcpu->run; > > + > > + while (!READ_ONCE(host_quit)) { > > + int current_iteration = READ_ONCE(iteration); > > + > > + vcpu_run(vcpu); > > + > > + TEST_ASSERT(get_ucall(vcpu, NULL) == UCALL_SYNC, > > + "Invalid guest sync status: exit_reason=%s\n", > > + exit_reason_str(run->exit_reason)); > > + > > + vcpu_last_completed_iteration[vcpu_idx] = current_iteration; > > + > > + /* Wait for the start of the next iteration to be signaled. */ > > + while (current_iteration == READ_ONCE(iteration) && > > + READ_ONCE(iteration) >= 0 && > > + !READ_ONCE(host_quit)) > > + ; > > + } > > +} > > + > > +static void run_test(enum vm_guest_mode mode, void *unused) > > +{ > > + struct kvm_vm *vm; > > + unsigned long **bitmaps; > > + uint64_t guest_num_pages; > > + uint64_t host_num_pages; > > + uint64_t pages_per_slot; > > + int i; > > + uint64_t total_4k_pages; > > + struct kvm_page_stats stats_populated; > > + struct kvm_page_stats stats_dirty_logging_enabled; > > + struct kvm_page_stats stats_dirty_pass[ITERATIONS]; > > + struct kvm_page_stats stats_clear_pass[ITERATIONS]; > > + struct kvm_page_stats stats_dirty_logging_disabled; > > + struct kvm_page_stats stats_repopulated; > > + > > + vm = memstress_create_vm(mode, VCPUS, guest_percpu_mem_size, > > + SLOTS, backing_src, false); > > + > > + guest_num_pages = (VCPUS * guest_percpu_mem_size) >> vm->page_shift; > > + guest_num_pages = vm_adjust_num_guest_pages(mode, guest_num_pages); > > + host_num_pages = vm_num_host_pages(mode, guest_num_pages); > > + pages_per_slot = host_num_pages / SLOTS; > > + > > + bitmaps = memstress_alloc_bitmaps(SLOTS, pages_per_slot); > > + > > + if (dirty_log_manual_caps) > > + vm_enable_cap(vm, KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2, > > + dirty_log_manual_caps); > > + > > + /* Start the iterations */ > > + iteration = -1; > > + host_quit = false; > > + > > + for (i = 0; i < VCPUS; i++) > > + vcpu_last_completed_iteration[i] = -1; > > + > > + memstress_start_vcpu_threads(VCPUS, vcpu_worker); > > + > > + run_vcpus_get_page_stats(vm, &stats_populated, "populating memory"); > > + > > + /* Enable dirty logging */ > > + memstress_enable_dirty_logging(vm, SLOTS); > > + > > + get_page_stats(vm, &stats_dirty_logging_enabled, "enabling dirty logging"); > > + > > + while (iteration < ITERATIONS) { > > + run_vcpus_get_page_stats(vm, &stats_dirty_pass[iteration - 1], > > + "dirtying memory"); > > + > > + memstress_get_dirty_log(vm, bitmaps, SLOTS); > > + > > + if (dirty_log_manual_caps) { > > + memstress_clear_dirty_log(vm, bitmaps, SLOTS, pages_per_slot); > > + > > + get_page_stats(vm, &stats_clear_pass[iteration - 1], "clearing dirty log"); > > + } > > + } > > + > > + /* Disable dirty logging */ > > + memstress_disable_dirty_logging(vm, SLOTS); > > + > > + get_page_stats(vm, &stats_dirty_logging_disabled, "disabling dirty logging"); > > + > > + /* Run vCPUs again to fault pages back in. */ > > + run_vcpus_get_page_stats(vm, &stats_repopulated, "repopulating memory"); > > + > > + /* > > + * Tell the vCPU threads to quit. No need to manually check that vCPUs > > + * have stopped running after disabling dirty logging, the join will > > + * wait for them to exit. > > + */ > > + host_quit = true; > > + memstress_join_vcpu_threads(VCPUS); > > + > > + memstress_free_bitmaps(bitmaps, SLOTS); > > + memstress_destroy_vm(vm); > > + > > + /* Make assertions about the page counts. */ > > + total_4k_pages = stats_populated.pages_4k; > > + total_4k_pages += stats_populated.pages_2m * 512; > > + total_4k_pages += stats_populated.pages_1g * 512 * 512; > > + > > + /* > > + * Check that all huge pages were split. Since large pages can only > > + * exist in the data slot, and the vCPUs should have dirtied all pages > > + * in the data slot, there should be no huge pages left after splitting. > > + * Splitting happens at dirty log enable time without > > + * KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 and after the first clear pass > > + * with that capability. > > + */ > > + if (dirty_log_manual_caps) { > > + TEST_ASSERT(stats_clear_pass[0].hugepages == 0, > > + "Unexpected huge page count after splitting. Expected 0, got %ld", > > + stats_clear_pass[0].hugepages); > > + TEST_ASSERT(stats_clear_pass[0].pages_4k == total_4k_pages, > > + "All memory should be mapped at 4k. Expected %ld 4k pages, got %ld", > > + total_4k_pages, stats_clear_pass[0].pages_4k); > > + TEST_ASSERT(stats_dirty_logging_enabled.hugepages == stats_populated.hugepages, > > + "Huge page count should not have changed from enabling dirty logging. Expected %ld, got %ld", > > + stats_populated.hugepages, stats_dirty_logging_enabled.hugepages); > > + } else { > > + TEST_ASSERT(stats_dirty_logging_enabled.hugepages == 0, > > + "Unexpected huge page count after splitting. Expected 0, got %ld", > > + stats_dirty_logging_enabled.hugepages); > > + TEST_ASSERT(stats_dirty_logging_enabled.pages_4k == total_4k_pages, > > + "All memory should be mapped at 4k. Expected %ld 4k pages, got %ld", > > + total_4k_pages, stats_dirty_logging_enabled.pages_4k); > > + } > > + > > + /* > > + * Once dirty logging is disabled and the vCPUs have touched all their > > + * memory again, the page counts should be the same as they were > > + * right after initial population of memory. > > + */ > > + TEST_ASSERT(stats_populated.pages_4k == stats_repopulated.pages_4k, > > + "4k page count did not return to its initial value after " > > + "dirty logging. Expected %ld, got %ld", > > + stats_populated.pages_4k, stats_repopulated.pages_4k); > > + TEST_ASSERT(stats_populated.pages_2m == stats_repopulated.pages_2m, > > + "2m page count did not return to its initial value after " > > + "dirty logging. Expected %ld, got %ld", > > + stats_populated.pages_2m, stats_repopulated.pages_2m); > > + TEST_ASSERT(stats_populated.pages_1g == stats_repopulated.pages_1g, > > + "1g page count did not return to its initial value after " > > + "dirty logging. Expected %ld, got %ld", > > + stats_populated.pages_1g, stats_repopulated.pages_1g); > > +} > > I know David suggested in the previous version to use __ASSERT_EQ(). > > I will recommend using __ASSERT_EQ(). If you make the variables name > meaningful then that should be sufficient in showing the expected > intent. Some examples: > ASSERT_EQ(after_clear_dirty_log.hugepages, 0); > ASSERT_EQ(after_clear_dirty_log.pages_4k, total_4k_pages); > ASSERT_EQ(after_enable_dirty_log.hugepages, initial_state.hugepages); > ASSERT_EQ(initial_state.pages_4k, after_disable_dirty_logging.pages_4k); > ... > > This makes code cleaner and error messages printed also self sufficient. Alright, that's 2 votes for ASSERT_EQ, so I'll change it. > > > + > > +static void help(char *name) > > +{ > > + puts(""); > > + printf("usage: %s [-h] [-b vcpu bytes] [-s mem type]\n", > > + name); > > + puts(""); > > + printf(" -b: specify the size of the memory region which should be\n" > > + " dirtied by each vCPU. e.g. 10M or 3G.\n" > > + " (default: 1G)\n"); > > + backing_src_help("-s"); > > + puts(""); > > + exit(0); > > This should not be exit(0) if a user passed the wrong option. Huh. That's a fair point, though many other tests also exit 0 after printing the help text. Annoyingly that would require exiting with a different code depending on whether the help text was printed because of the -h or a bad option but that's not too hard. > > > +} > > + > > +int main(int argc, char *argv[]) > > +{ > > + int opt; > > + > > + TEST_REQUIRE(get_kvm_param_bool("eager_page_split")); > > + TEST_REQUIRE(get_kvm_param_bool("tdp_mmu")); > > + > > + while ((opt = getopt(argc, argv, "b:hs:")) != -1) { > > + switch (opt) { > > + case 'b': > > + guest_percpu_mem_size = parse_size(optarg); > > + break; > > + case 's': > > + backing_src = parse_backing_src_type(optarg); > > + break; > > + case 'h': > > + default: > > + help(argv[0]); > > + break; > > + } > > + } > > + > > + if (!is_backing_src_hugetlb(backing_src)) { > > + pr_info("This test will only work reliably with HugeTLB memory. " > > + "It can work with THP, but that is best effort."); > > + return KSFT_SKIP; > > + } > > + > > + guest_modes_append_default(); > > + > > + dirty_log_manual_caps = 0; > > + for_each_guest_mode(run_test, NULL); > > + > > + dirty_log_manual_caps = > > + kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2); > > Shouldn't the check be done in the beginning and skip the test f it is > not supported? Hmmm, we'd only want to skip this second run of the test, so we probably wouldn't want to exit with an error. Exiting early is probably reasonable though. > > > + dirty_log_manual_caps &= (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | > > + KVM_DIRTY_LOG_INITIALLY_SET); > > + for_each_guest_mode(run_test, NULL); > > + > > + return 0; > > +} > > -- > > 2.39.1.405.gd4c25cc71f-goog > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-23 23:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-23 19:03 [PATCH v2 0/2] selftests: KVM: Add a test for eager page splitting Ben Gardon 2023-01-23 19:03 ` [PATCH v2 1/2] selftests: KVM: Move dirty logging functions to memstress.(c|h) Ben Gardon 2023-01-23 21:03 ` Vipin Sharma 2023-01-23 19:03 ` [PATCH v2 2/2] selftests: KVM: Add page splitting test Ben Gardon 2023-01-23 21:32 ` Vipin Sharma 2023-01-23 23:42 ` Ben Gardon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox