* [PATCH 0/3] KVM: selftests: Small fixes for dirty_log_perf_test
@ 2021-09-15 21:30 David Matlack
2021-09-15 21:30 ` [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test David Matlack
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: David Matlack @ 2021-09-15 21:30 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm, Andrew Jones, Ben Gardon, Axel Rasmussen, Yanan Wang, David Matlack
This series fixes 2 bugs in dirty_log_perf_test:
- Incorrect interleaving of help messages for -s and -x (patch 2)
- Buffer overflow when using multiple slots (patch 3)
Both bugs were introduced by commit 609e6202ea5f ("KVM: selftests:
Support multiple slots in dirty_log_perf_test").
Patch 1 is a small tangentially related cleanup to use a consistent
flag for the backing source across all selftests.
David Matlack (3):
KVM: selftests: Change backing_src flag to -s in demand_paging_test
KVM: selftests: Refactor help message for -s backing_src
KVM: selftests: Fix dirty bitmap offset calculation
.../selftests/kvm/access_tracking_perf_test.c | 6 ++---
.../selftests/kvm/demand_paging_test.c | 13 +++++------
.../selftests/kvm/dirty_log_perf_test.c | 23 +++++++++++++------
.../testing/selftests/kvm/include/test_util.h | 5 +++-
.../selftests/kvm/kvm_page_table_test.c | 7 ++----
tools/testing/selftests/kvm/lib/test_util.c | 17 ++++++++++----
6 files changed, 43 insertions(+), 28 deletions(-)
--
2.33.0.309.g3052b89438-goog
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test 2021-09-15 21:30 [PATCH 0/3] KVM: selftests: Small fixes for dirty_log_perf_test David Matlack @ 2021-09-15 21:30 ` David Matlack 2021-09-15 21:56 ` Ben Gardon 2021-09-16 8:34 ` Andrew Jones 2021-09-15 21:30 ` [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src David Matlack 2021-09-15 21:30 ` [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation David Matlack 2 siblings, 2 replies; 14+ messages in thread From: David Matlack @ 2021-09-15 21:30 UTC (permalink / raw) To: Paolo Bonzini Cc: kvm, Andrew Jones, Ben Gardon, Axel Rasmussen, Yanan Wang, David Matlack Every other KVM selftest uses -s for the backing_src, so switch demand_paging_test to match. Signed-off-by: David Matlack <dmatlack@google.com> --- tools/testing/selftests/kvm/demand_paging_test.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index e79c1b64977f..735c081e774e 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -416,7 +416,7 @@ static void help(char *name) { puts(""); printf("usage: %s [-h] [-m vm_mode] [-u uffd_mode] [-d uffd_delay_usec]\n" - " [-b memory] [-t type] [-v vcpus] [-o]\n", name); + " [-b memory] [-s type] [-v vcpus] [-o]\n", name); guest_modes_help(); printf(" -u: use userfaultfd to handle vCPU page faults. Mode is a\n" " UFFD registration mode: 'MISSING' or 'MINOR'.\n"); @@ -426,7 +426,7 @@ static void help(char *name) printf(" -b: specify the size of the memory region which should be\n" " demand paged by each vCPU. e.g. 10M or 3G.\n" " Default: 1G\n"); - printf(" -t: The type of backing memory to use. Default: anonymous\n"); + printf(" -s: The type of backing memory to use. Default: anonymous\n"); backing_src_help(); printf(" -v: specify the number of vCPUs to run.\n"); printf(" -o: Overlap guest memory accesses instead of partitioning\n" @@ -446,7 +446,7 @@ int main(int argc, char *argv[]) guest_modes_append_default(); - while ((opt = getopt(argc, argv, "hm:u:d:b:t:v:o")) != -1) { + while ((opt = getopt(argc, argv, "hm:u:d:b:s:v:o")) != -1) { switch (opt) { case 'm': guest_modes_cmdline(optarg); @@ -465,7 +465,7 @@ int main(int argc, char *argv[]) case 'b': guest_percpu_mem_size = parse_size(optarg); break; - case 't': + case 's': p.src_type = parse_backing_src_type(optarg); break; case 'v': @@ -485,7 +485,7 @@ int main(int argc, char *argv[]) if (p.uffd_mode == UFFDIO_REGISTER_MODE_MINOR && !backing_src_is_shared(p.src_type)) { - TEST_FAIL("userfaultfd MINOR mode requires shared memory; pick a different -t"); + TEST_FAIL("userfaultfd MINOR mode requires shared memory; pick a different -s"); } for_each_guest_mode(run_test, &p); -- 2.33.0.309.g3052b89438-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test 2021-09-15 21:30 ` [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test David Matlack @ 2021-09-15 21:56 ` Ben Gardon 2021-09-16 8:34 ` Andrew Jones 1 sibling, 0 replies; 14+ messages in thread From: Ben Gardon @ 2021-09-15 21:56 UTC (permalink / raw) To: David Matlack Cc: Paolo Bonzini, kvm, Andrew Jones, Axel Rasmussen, Yanan Wang On Wed, Sep 15, 2021 at 2:30 PM David Matlack <dmatlack@google.com> wrote: > > Every other KVM selftest uses -s for the backing_src, so switch > demand_paging_test to match. > > Signed-off-by: David Matlack <dmatlack@google.com> Reviewed-by: Ben Gardon <bgardon@google.com> > --- > tools/testing/selftests/kvm/demand_paging_test.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c > index e79c1b64977f..735c081e774e 100644 > --- a/tools/testing/selftests/kvm/demand_paging_test.c > +++ b/tools/testing/selftests/kvm/demand_paging_test.c > @@ -416,7 +416,7 @@ static void help(char *name) > { > puts(""); > printf("usage: %s [-h] [-m vm_mode] [-u uffd_mode] [-d uffd_delay_usec]\n" > - " [-b memory] [-t type] [-v vcpus] [-o]\n", name); > + " [-b memory] [-s type] [-v vcpus] [-o]\n", name); > guest_modes_help(); > printf(" -u: use userfaultfd to handle vCPU page faults. Mode is a\n" > " UFFD registration mode: 'MISSING' or 'MINOR'.\n"); > @@ -426,7 +426,7 @@ static void help(char *name) > printf(" -b: specify the size of the memory region which should be\n" > " demand paged by each vCPU. e.g. 10M or 3G.\n" > " Default: 1G\n"); > - printf(" -t: The type of backing memory to use. Default: anonymous\n"); > + printf(" -s: The type of backing memory to use. Default: anonymous\n"); > backing_src_help(); > printf(" -v: specify the number of vCPUs to run.\n"); > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > @@ -446,7 +446,7 @@ int main(int argc, char *argv[]) > > guest_modes_append_default(); > > - while ((opt = getopt(argc, argv, "hm:u:d:b:t:v:o")) != -1) { > + while ((opt = getopt(argc, argv, "hm:u:d:b:s:v:o")) != -1) { > switch (opt) { > case 'm': > guest_modes_cmdline(optarg); > @@ -465,7 +465,7 @@ int main(int argc, char *argv[]) > case 'b': > guest_percpu_mem_size = parse_size(optarg); > break; > - case 't': > + case 's': > p.src_type = parse_backing_src_type(optarg); > break; > case 'v': > @@ -485,7 +485,7 @@ int main(int argc, char *argv[]) > > if (p.uffd_mode == UFFDIO_REGISTER_MODE_MINOR && > !backing_src_is_shared(p.src_type)) { > - TEST_FAIL("userfaultfd MINOR mode requires shared memory; pick a different -t"); > + TEST_FAIL("userfaultfd MINOR mode requires shared memory; pick a different -s"); > } > > for_each_guest_mode(run_test, &p); > -- > 2.33.0.309.g3052b89438-goog > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test 2021-09-15 21:30 ` [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test David Matlack 2021-09-15 21:56 ` Ben Gardon @ 2021-09-16 8:34 ` Andrew Jones 1 sibling, 0 replies; 14+ messages in thread From: Andrew Jones @ 2021-09-16 8:34 UTC (permalink / raw) To: David Matlack; +Cc: Paolo Bonzini, kvm, Ben Gardon, Axel Rasmussen, Yanan Wang On Wed, Sep 15, 2021 at 09:30:32PM +0000, David Matlack wrote: > Every other KVM selftest uses -s for the backing_src, so switch > demand_paging_test to match. > > Signed-off-by: David Matlack <dmatlack@google.com> > --- > tools/testing/selftests/kvm/demand_paging_test.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > Reviewed-by: Andrew Jones <drjones@redhat.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src 2021-09-15 21:30 [PATCH 0/3] KVM: selftests: Small fixes for dirty_log_perf_test David Matlack 2021-09-15 21:30 ` [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test David Matlack @ 2021-09-15 21:30 ` David Matlack 2021-09-15 21:55 ` Ben Gardon 2021-09-16 8:41 ` Andrew Jones 2021-09-15 21:30 ` [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation David Matlack 2 siblings, 2 replies; 14+ messages in thread From: David Matlack @ 2021-09-15 21:30 UTC (permalink / raw) To: Paolo Bonzini Cc: kvm, Andrew Jones, Ben Gardon, Axel Rasmussen, Yanan Wang, David Matlack All selftests that support the backing_src option were printing their own description of the flag and then calling backing_src_help() to dump the list of available backing sources. Consolidate the flag printing in backing_src_help() to align indentation, reduce duplicated strings, and improve consistency across tests. Note: Passing "-s" to backing_src_help is unnecessary since every test uses the same flag. However I decided to keep it for code readability at the call sites. While here this opportunistically fixes the incorrectly interleaved printing -x help message and list of backing source types in dirty_log_perf_test. Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") Signed-off-by: David Matlack <dmatlack@google.com> --- .../selftests/kvm/access_tracking_perf_test.c | 6 ++---- .../testing/selftests/kvm/demand_paging_test.c | 5 ++--- .../testing/selftests/kvm/dirty_log_perf_test.c | 8 +++----- tools/testing/selftests/kvm/include/test_util.h | 5 ++++- .../testing/selftests/kvm/kvm_page_table_test.c | 7 ++----- tools/testing/selftests/kvm/lib/test_util.c | 17 +++++++++++++---- 6 files changed, 26 insertions(+), 22 deletions(-) diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c index 71e277c7c3f3..5d95113c7b7c 100644 --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c @@ -371,9 +371,7 @@ static void help(char *name) printf(" -v: specify the number of vCPUs to run.\n"); printf(" -o: Overlap guest memory accesses instead of partitioning\n" " them into a separate region of memory for each vCPU.\n"); - printf(" -s: specify the type of memory that should be used to\n" - " back the guest data region.\n\n"); - backing_src_help(); + backing_src_help("-s"); puts(""); exit(0); } @@ -381,7 +379,7 @@ static void help(char *name) int main(int argc, char *argv[]) { struct test_params params = { - .backing_src = VM_MEM_SRC_ANONYMOUS, + .backing_src = DEFAULT_VM_MEM_SRC, .vcpu_memory_bytes = DEFAULT_PER_VCPU_MEM_SIZE, .vcpus = 1, }; diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index 735c081e774e..96cd3e0357f6 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -426,8 +426,7 @@ static void help(char *name) printf(" -b: specify the size of the memory region which should be\n" " demand paged by each vCPU. e.g. 10M or 3G.\n" " Default: 1G\n"); - printf(" -s: The type of backing memory to use. Default: anonymous\n"); - backing_src_help(); + backing_src_help("-s"); printf(" -v: specify the number of vCPUs to run.\n"); printf(" -o: Overlap guest memory accesses instead of partitioning\n" " them into a separate region of memory for each vCPU.\n"); @@ -439,7 +438,7 @@ int main(int argc, char *argv[]) { int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); struct test_params p = { - .src_type = VM_MEM_SRC_ANONYMOUS, + .src_type = DEFAULT_VM_MEM_SRC, .partition_vcpu_memory_access = true, }; int opt; diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index 3c30d0045d8d..5ad9f2bc7369 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -308,11 +308,9 @@ static void help(char *name) printf(" -v: specify the number of vCPUs to run.\n"); printf(" -o: Overlap guest memory accesses instead of partitioning\n" " them into a separate region of memory for each vCPU.\n"); - printf(" -s: specify the type of memory that should be used to\n" - " back the guest data region.\n\n"); + backing_src_help("-s"); printf(" -x: Split the memory region into this number of memslots.\n" - " (default: 1)"); - backing_src_help(); + " (default: 1)\n"); puts(""); exit(0); } @@ -324,7 +322,7 @@ int main(int argc, char *argv[]) .iterations = TEST_HOST_LOOP_N, .wr_fract = 1, .partition_vcpu_memory_access = true, - .backing_src = VM_MEM_SRC_ANONYMOUS, + .backing_src = DEFAULT_VM_MEM_SRC, .slots = 1, }; int opt; diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h index d79be15dd3d2..2f09f2994733 100644 --- a/tools/testing/selftests/kvm/include/test_util.h +++ b/tools/testing/selftests/kvm/include/test_util.h @@ -68,6 +68,7 @@ struct timespec timespec_sub(struct timespec ts1, struct timespec ts2); struct timespec timespec_elapsed(struct timespec start); struct timespec timespec_div(struct timespec ts, int divisor); + enum vm_mem_backing_src_type { VM_MEM_SRC_ANONYMOUS, VM_MEM_SRC_ANONYMOUS_THP, @@ -90,6 +91,8 @@ enum vm_mem_backing_src_type { NUM_SRC_TYPES, }; +#define DEFAULT_VM_MEM_SRC VM_MEM_SRC_ANONYMOUS + struct vm_mem_backing_src_alias { const char *name; uint32_t flag; @@ -100,7 +103,7 @@ size_t get_trans_hugepagesz(void); size_t get_def_hugetlb_pagesz(void); const struct vm_mem_backing_src_alias *vm_mem_backing_src_alias(uint32_t i); size_t get_backing_src_pagesz(uint32_t i); -void backing_src_help(void); +void backing_src_help(const char *flag); enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name); /* diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c index 0d04a7db7f24..36407cb0ec85 100644 --- a/tools/testing/selftests/kvm/kvm_page_table_test.c +++ b/tools/testing/selftests/kvm/kvm_page_table_test.c @@ -456,10 +456,7 @@ static void help(char *name) " (default: 1G)\n"); printf(" -v: specify the number of vCPUs to run\n" " (default: 1)\n"); - printf(" -s: specify the type of memory that should be used to\n" - " back the guest data region.\n" - " (default: anonymous)\n\n"); - backing_src_help(); + backing_src_help("-s"); puts(""); } @@ -468,7 +465,7 @@ int main(int argc, char *argv[]) int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); struct test_params p = { .test_mem_size = DEFAULT_TEST_MEM_SIZE, - .src_type = VM_MEM_SRC_ANONYMOUS, + .src_type = DEFAULT_VM_MEM_SRC, }; int opt; diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c index af1031fed97f..ea23a86132ed 100644 --- a/tools/testing/selftests/kvm/lib/test_util.c +++ b/tools/testing/selftests/kvm/lib/test_util.c @@ -279,13 +279,22 @@ size_t get_backing_src_pagesz(uint32_t i) } } -void backing_src_help(void) +void print_available_backing_src_types(const char *prefix) { int i; - printf("Available backing src types:\n"); + printf("%sAvailable backing src types:\n", prefix); + for (i = 0; i < NUM_SRC_TYPES; i++) - printf("\t%s\n", vm_mem_backing_src_alias(i)->name); + printf("%s %s\n", prefix, vm_mem_backing_src_alias(i)->name); +} + +void backing_src_help(const char *flag) +{ + printf(" %s: specify the type of memory that should be used to\n" + " back the guest data region. (default: %s)\n", + flag, vm_mem_backing_src_alias(DEFAULT_VM_MEM_SRC)->name); + print_available_backing_src_types(" "); } enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name) @@ -296,7 +305,7 @@ enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name) if (!strcmp(type_name, vm_mem_backing_src_alias(i)->name)) return i; - backing_src_help(); + print_available_backing_src_types(""); TEST_FAIL("Unknown backing src type: %s", type_name); return -1; } -- 2.33.0.309.g3052b89438-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src 2021-09-15 21:30 ` [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src David Matlack @ 2021-09-15 21:55 ` Ben Gardon 2021-09-16 8:41 ` Andrew Jones 1 sibling, 0 replies; 14+ messages in thread From: Ben Gardon @ 2021-09-15 21:55 UTC (permalink / raw) To: David Matlack Cc: Paolo Bonzini, kvm, Andrew Jones, Axel Rasmussen, Yanan Wang On Wed, Sep 15, 2021 at 2:30 PM David Matlack <dmatlack@google.com> wrote: > > All selftests that support the backing_src option were printing their > own description of the flag and then calling backing_src_help() to dump > the list of available backing sources. Consolidate the flag printing in > backing_src_help() to align indentation, reduce duplicated strings, and > improve consistency across tests. > > Note: Passing "-s" to backing_src_help is unnecessary since every test > uses the same flag. However I decided to keep it for code readability > at the call sites. > > While here this opportunistically fixes the incorrectly interleaved > printing -x help message and list of backing source types in > dirty_log_perf_test. > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > Signed-off-by: David Matlack <dmatlack@google.com> Reviewed-by: Ben Gardon <bgardon@google.com> > --- > .../selftests/kvm/access_tracking_perf_test.c | 6 ++---- > .../testing/selftests/kvm/demand_paging_test.c | 5 ++--- > .../testing/selftests/kvm/dirty_log_perf_test.c | 8 +++----- > tools/testing/selftests/kvm/include/test_util.h | 5 ++++- > .../testing/selftests/kvm/kvm_page_table_test.c | 7 ++----- > tools/testing/selftests/kvm/lib/test_util.c | 17 +++++++++++++---- > 6 files changed, 26 insertions(+), 22 deletions(-) > > diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c > index 71e277c7c3f3..5d95113c7b7c 100644 > --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c > +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c > @@ -371,9 +371,7 @@ static void help(char *name) > printf(" -v: specify the number of vCPUs to run.\n"); > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > " them into a separate region of memory for each vCPU.\n"); > - printf(" -s: specify the type of memory that should be used to\n" > - " back the guest data region.\n\n"); > - backing_src_help(); > + backing_src_help("-s"); > puts(""); > exit(0); > } > @@ -381,7 +379,7 @@ static void help(char *name) > int main(int argc, char *argv[]) > { > struct test_params params = { > - .backing_src = VM_MEM_SRC_ANONYMOUS, > + .backing_src = DEFAULT_VM_MEM_SRC, > .vcpu_memory_bytes = DEFAULT_PER_VCPU_MEM_SIZE, > .vcpus = 1, > }; > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c > index 735c081e774e..96cd3e0357f6 100644 > --- a/tools/testing/selftests/kvm/demand_paging_test.c > +++ b/tools/testing/selftests/kvm/demand_paging_test.c > @@ -426,8 +426,7 @@ static void help(char *name) > printf(" -b: specify the size of the memory region which should be\n" > " demand paged by each vCPU. e.g. 10M or 3G.\n" > " Default: 1G\n"); > - printf(" -s: The type of backing memory to use. Default: anonymous\n"); > - backing_src_help(); > + backing_src_help("-s"); > printf(" -v: specify the number of vCPUs to run.\n"); > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > " them into a separate region of memory for each vCPU.\n"); > @@ -439,7 +438,7 @@ int main(int argc, char *argv[]) > { > int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); > struct test_params p = { > - .src_type = VM_MEM_SRC_ANONYMOUS, > + .src_type = DEFAULT_VM_MEM_SRC, > .partition_vcpu_memory_access = true, > }; > int opt; > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c > index 3c30d0045d8d..5ad9f2bc7369 100644 > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > @@ -308,11 +308,9 @@ static void help(char *name) > printf(" -v: specify the number of vCPUs to run.\n"); > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > " them into a separate region of memory for each vCPU.\n"); > - printf(" -s: specify the type of memory that should be used to\n" > - " back the guest data region.\n\n"); > + backing_src_help("-s"); > printf(" -x: Split the memory region into this number of memslots.\n" > - " (default: 1)"); > - backing_src_help(); > + " (default: 1)\n"); > puts(""); > exit(0); > } > @@ -324,7 +322,7 @@ int main(int argc, char *argv[]) > .iterations = TEST_HOST_LOOP_N, > .wr_fract = 1, > .partition_vcpu_memory_access = true, > - .backing_src = VM_MEM_SRC_ANONYMOUS, > + .backing_src = DEFAULT_VM_MEM_SRC, > .slots = 1, > }; > int opt; > diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h > index d79be15dd3d2..2f09f2994733 100644 > --- a/tools/testing/selftests/kvm/include/test_util.h > +++ b/tools/testing/selftests/kvm/include/test_util.h > @@ -68,6 +68,7 @@ struct timespec timespec_sub(struct timespec ts1, struct timespec ts2); > struct timespec timespec_elapsed(struct timespec start); > struct timespec timespec_div(struct timespec ts, int divisor); > > + > enum vm_mem_backing_src_type { > VM_MEM_SRC_ANONYMOUS, > VM_MEM_SRC_ANONYMOUS_THP, > @@ -90,6 +91,8 @@ enum vm_mem_backing_src_type { > NUM_SRC_TYPES, > }; > > +#define DEFAULT_VM_MEM_SRC VM_MEM_SRC_ANONYMOUS > + > struct vm_mem_backing_src_alias { > const char *name; > uint32_t flag; > @@ -100,7 +103,7 @@ size_t get_trans_hugepagesz(void); > size_t get_def_hugetlb_pagesz(void); > const struct vm_mem_backing_src_alias *vm_mem_backing_src_alias(uint32_t i); > size_t get_backing_src_pagesz(uint32_t i); > -void backing_src_help(void); > +void backing_src_help(const char *flag); > enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name); > > /* > diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c > index 0d04a7db7f24..36407cb0ec85 100644 > --- a/tools/testing/selftests/kvm/kvm_page_table_test.c > +++ b/tools/testing/selftests/kvm/kvm_page_table_test.c > @@ -456,10 +456,7 @@ static void help(char *name) > " (default: 1G)\n"); > printf(" -v: specify the number of vCPUs to run\n" > " (default: 1)\n"); > - printf(" -s: specify the type of memory that should be used to\n" > - " back the guest data region.\n" > - " (default: anonymous)\n\n"); > - backing_src_help(); > + backing_src_help("-s"); > puts(""); > } > > @@ -468,7 +465,7 @@ int main(int argc, char *argv[]) > int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); > struct test_params p = { > .test_mem_size = DEFAULT_TEST_MEM_SIZE, > - .src_type = VM_MEM_SRC_ANONYMOUS, > + .src_type = DEFAULT_VM_MEM_SRC, > }; > int opt; > > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c > index af1031fed97f..ea23a86132ed 100644 > --- a/tools/testing/selftests/kvm/lib/test_util.c > +++ b/tools/testing/selftests/kvm/lib/test_util.c > @@ -279,13 +279,22 @@ size_t get_backing_src_pagesz(uint32_t i) > } > } > > -void backing_src_help(void) > +void print_available_backing_src_types(const char *prefix) > { > int i; > > - printf("Available backing src types:\n"); > + printf("%sAvailable backing src types:\n", prefix); > + > for (i = 0; i < NUM_SRC_TYPES; i++) > - printf("\t%s\n", vm_mem_backing_src_alias(i)->name); > + printf("%s %s\n", prefix, vm_mem_backing_src_alias(i)->name); > +} > + > +void backing_src_help(const char *flag) > +{ > + printf(" %s: specify the type of memory that should be used to\n" > + " back the guest data region. (default: %s)\n", > + flag, vm_mem_backing_src_alias(DEFAULT_VM_MEM_SRC)->name); > + print_available_backing_src_types(" "); > } > > enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name) > @@ -296,7 +305,7 @@ enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name) > if (!strcmp(type_name, vm_mem_backing_src_alias(i)->name)) > return i; > > - backing_src_help(); > + print_available_backing_src_types(""); > TEST_FAIL("Unknown backing src type: %s", type_name); > return -1; > } > -- > 2.33.0.309.g3052b89438-goog > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src 2021-09-15 21:30 ` [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src David Matlack 2021-09-15 21:55 ` Ben Gardon @ 2021-09-16 8:41 ` Andrew Jones 2021-09-16 16:24 ` David Matlack 1 sibling, 1 reply; 14+ messages in thread From: Andrew Jones @ 2021-09-16 8:41 UTC (permalink / raw) To: David Matlack; +Cc: Paolo Bonzini, kvm, Ben Gardon, Axel Rasmussen, Yanan Wang On Wed, Sep 15, 2021 at 09:30:33PM +0000, David Matlack wrote: > All selftests that support the backing_src option were printing their > own description of the flag and then calling backing_src_help() to dump > the list of available backing sources. Consolidate the flag printing in > backing_src_help() to align indentation, reduce duplicated strings, and > improve consistency across tests. > > Note: Passing "-s" to backing_src_help is unnecessary since every test > uses the same flag. However I decided to keep it for code readability > at the call sites. I think I'd prefer not passing "-s", but I won't insist. > > While here this opportunistically fixes the incorrectly interleaved > printing -x help message and list of backing source types in > dirty_log_perf_test. > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > Signed-off-by: David Matlack <dmatlack@google.com> > --- > .../selftests/kvm/access_tracking_perf_test.c | 6 ++---- > .../testing/selftests/kvm/demand_paging_test.c | 5 ++--- > .../testing/selftests/kvm/dirty_log_perf_test.c | 8 +++----- > tools/testing/selftests/kvm/include/test_util.h | 5 ++++- > .../testing/selftests/kvm/kvm_page_table_test.c | 7 ++----- > tools/testing/selftests/kvm/lib/test_util.c | 17 +++++++++++++---- > 6 files changed, 26 insertions(+), 22 deletions(-) > > diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c > index 71e277c7c3f3..5d95113c7b7c 100644 > --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c > +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c > @@ -371,9 +371,7 @@ static void help(char *name) > printf(" -v: specify the number of vCPUs to run.\n"); > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > " them into a separate region of memory for each vCPU.\n"); > - printf(" -s: specify the type of memory that should be used to\n" > - " back the guest data region.\n\n"); > - backing_src_help(); > + backing_src_help("-s"); > puts(""); > exit(0); > } > @@ -381,7 +379,7 @@ static void help(char *name) > int main(int argc, char *argv[]) > { > struct test_params params = { > - .backing_src = VM_MEM_SRC_ANONYMOUS, > + .backing_src = DEFAULT_VM_MEM_SRC, > .vcpu_memory_bytes = DEFAULT_PER_VCPU_MEM_SIZE, > .vcpus = 1, > }; > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c > index 735c081e774e..96cd3e0357f6 100644 > --- a/tools/testing/selftests/kvm/demand_paging_test.c > +++ b/tools/testing/selftests/kvm/demand_paging_test.c > @@ -426,8 +426,7 @@ static void help(char *name) > printf(" -b: specify the size of the memory region which should be\n" > " demand paged by each vCPU. e.g. 10M or 3G.\n" > " Default: 1G\n"); > - printf(" -s: The type of backing memory to use. Default: anonymous\n"); > - backing_src_help(); > + backing_src_help("-s"); > printf(" -v: specify the number of vCPUs to run.\n"); > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > " them into a separate region of memory for each vCPU.\n"); > @@ -439,7 +438,7 @@ int main(int argc, char *argv[]) > { > int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); > struct test_params p = { > - .src_type = VM_MEM_SRC_ANONYMOUS, > + .src_type = DEFAULT_VM_MEM_SRC, > .partition_vcpu_memory_access = true, > }; > int opt; > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c > index 3c30d0045d8d..5ad9f2bc7369 100644 > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > @@ -308,11 +308,9 @@ static void help(char *name) > printf(" -v: specify the number of vCPUs to run.\n"); > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > " them into a separate region of memory for each vCPU.\n"); > - printf(" -s: specify the type of memory that should be used to\n" > - " back the guest data region.\n\n"); > + backing_src_help("-s"); > printf(" -x: Split the memory region into this number of memslots.\n" > - " (default: 1)"); > - backing_src_help(); > + " (default: 1)\n"); > puts(""); > exit(0); > } > @@ -324,7 +322,7 @@ int main(int argc, char *argv[]) > .iterations = TEST_HOST_LOOP_N, > .wr_fract = 1, > .partition_vcpu_memory_access = true, > - .backing_src = VM_MEM_SRC_ANONYMOUS, > + .backing_src = DEFAULT_VM_MEM_SRC, > .slots = 1, > }; > int opt; > diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h > index d79be15dd3d2..2f09f2994733 100644 > --- a/tools/testing/selftests/kvm/include/test_util.h > +++ b/tools/testing/selftests/kvm/include/test_util.h > @@ -68,6 +68,7 @@ struct timespec timespec_sub(struct timespec ts1, struct timespec ts2); > struct timespec timespec_elapsed(struct timespec start); > struct timespec timespec_div(struct timespec ts, int divisor); > > + New extra blank line. Added on purpose? > enum vm_mem_backing_src_type { > VM_MEM_SRC_ANONYMOUS, > VM_MEM_SRC_ANONYMOUS_THP, > @@ -90,6 +91,8 @@ enum vm_mem_backing_src_type { > NUM_SRC_TYPES, > }; > > +#define DEFAULT_VM_MEM_SRC VM_MEM_SRC_ANONYMOUS > + > struct vm_mem_backing_src_alias { > const char *name; > uint32_t flag; > @@ -100,7 +103,7 @@ size_t get_trans_hugepagesz(void); > size_t get_def_hugetlb_pagesz(void); > const struct vm_mem_backing_src_alias *vm_mem_backing_src_alias(uint32_t i); > size_t get_backing_src_pagesz(uint32_t i); > -void backing_src_help(void); > +void backing_src_help(const char *flag); > enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name); > > /* > diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c > index 0d04a7db7f24..36407cb0ec85 100644 > --- a/tools/testing/selftests/kvm/kvm_page_table_test.c > +++ b/tools/testing/selftests/kvm/kvm_page_table_test.c > @@ -456,10 +456,7 @@ static void help(char *name) > " (default: 1G)\n"); > printf(" -v: specify the number of vCPUs to run\n" > " (default: 1)\n"); > - printf(" -s: specify the type of memory that should be used to\n" > - " back the guest data region.\n" > - " (default: anonymous)\n\n"); > - backing_src_help(); > + backing_src_help("-s"); > puts(""); > } > > @@ -468,7 +465,7 @@ int main(int argc, char *argv[]) > int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); > struct test_params p = { > .test_mem_size = DEFAULT_TEST_MEM_SIZE, > - .src_type = VM_MEM_SRC_ANONYMOUS, > + .src_type = DEFAULT_VM_MEM_SRC, > }; > int opt; > > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c > index af1031fed97f..ea23a86132ed 100644 > --- a/tools/testing/selftests/kvm/lib/test_util.c > +++ b/tools/testing/selftests/kvm/lib/test_util.c > @@ -279,13 +279,22 @@ size_t get_backing_src_pagesz(uint32_t i) > } > } > > -void backing_src_help(void) > +void print_available_backing_src_types(const char *prefix) This can probably be static. > { > int i; > > - printf("Available backing src types:\n"); > + printf("%sAvailable backing src types:\n", prefix); > + > for (i = 0; i < NUM_SRC_TYPES; i++) > - printf("\t%s\n", vm_mem_backing_src_alias(i)->name); > + printf("%s %s\n", prefix, vm_mem_backing_src_alias(i)->name); > +} > + > +void backing_src_help(const char *flag) > +{ > + printf(" %s: specify the type of memory that should be used to\n" > + " back the guest data region. (default: %s)\n", > + flag, vm_mem_backing_src_alias(DEFAULT_VM_MEM_SRC)->name); > + print_available_backing_src_types(" "); > } > > enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name) > @@ -296,7 +305,7 @@ enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name) > if (!strcmp(type_name, vm_mem_backing_src_alias(i)->name)) > return i; > > - backing_src_help(); > + print_available_backing_src_types(""); > TEST_FAIL("Unknown backing src type: %s", type_name); > return -1; > } > -- > 2.33.0.309.g3052b89438-goog > Reviewed-by: Andrew Jones <drjones@redhat.com> Thanks, drew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src 2021-09-16 8:41 ` Andrew Jones @ 2021-09-16 16:24 ` David Matlack 0 siblings, 0 replies; 14+ messages in thread From: David Matlack @ 2021-09-16 16:24 UTC (permalink / raw) To: Andrew Jones Cc: Paolo Bonzini, kvm list, Ben Gardon, Axel Rasmussen, Yanan Wang On Thu, Sep 16, 2021 at 1:41 AM Andrew Jones <drjones@redhat.com> wrote: > > On Wed, Sep 15, 2021 at 09:30:33PM +0000, David Matlack wrote: > > All selftests that support the backing_src option were printing their > > own description of the flag and then calling backing_src_help() to dump > > the list of available backing sources. Consolidate the flag printing in > > backing_src_help() to align indentation, reduce duplicated strings, and > > improve consistency across tests. > > > > Note: Passing "-s" to backing_src_help is unnecessary since every test > > uses the same flag. However I decided to keep it for code readability > > at the call sites. > > I think I'd prefer not passing "-s", but I won't insist. > > > > > While here this opportunistically fixes the incorrectly interleaved > > printing -x help message and list of backing source types in > > dirty_log_perf_test. > > > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > > Signed-off-by: David Matlack <dmatlack@google.com> > > --- > > .../selftests/kvm/access_tracking_perf_test.c | 6 ++---- > > .../testing/selftests/kvm/demand_paging_test.c | 5 ++--- > > .../testing/selftests/kvm/dirty_log_perf_test.c | 8 +++----- > > tools/testing/selftests/kvm/include/test_util.h | 5 ++++- > > .../testing/selftests/kvm/kvm_page_table_test.c | 7 ++----- > > tools/testing/selftests/kvm/lib/test_util.c | 17 +++++++++++++---- > > 6 files changed, 26 insertions(+), 22 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c > > index 71e277c7c3f3..5d95113c7b7c 100644 > > --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c > > +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c > > @@ -371,9 +371,7 @@ static void help(char *name) > > printf(" -v: specify the number of vCPUs to run.\n"); > > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > > " them into a separate region of memory for each vCPU.\n"); > > - printf(" -s: specify the type of memory that should be used to\n" > > - " back the guest data region.\n\n"); > > - backing_src_help(); > > + backing_src_help("-s"); > > puts(""); > > exit(0); > > } > > @@ -381,7 +379,7 @@ static void help(char *name) > > int main(int argc, char *argv[]) > > { > > struct test_params params = { > > - .backing_src = VM_MEM_SRC_ANONYMOUS, > > + .backing_src = DEFAULT_VM_MEM_SRC, > > .vcpu_memory_bytes = DEFAULT_PER_VCPU_MEM_SIZE, > > .vcpus = 1, > > }; > > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c > > index 735c081e774e..96cd3e0357f6 100644 > > --- a/tools/testing/selftests/kvm/demand_paging_test.c > > +++ b/tools/testing/selftests/kvm/demand_paging_test.c > > @@ -426,8 +426,7 @@ static void help(char *name) > > printf(" -b: specify the size of the memory region which should be\n" > > " demand paged by each vCPU. e.g. 10M or 3G.\n" > > " Default: 1G\n"); > > - printf(" -s: The type of backing memory to use. Default: anonymous\n"); > > - backing_src_help(); > > + backing_src_help("-s"); > > printf(" -v: specify the number of vCPUs to run.\n"); > > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > > " them into a separate region of memory for each vCPU.\n"); > > @@ -439,7 +438,7 @@ int main(int argc, char *argv[]) > > { > > int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); > > struct test_params p = { > > - .src_type = VM_MEM_SRC_ANONYMOUS, > > + .src_type = DEFAULT_VM_MEM_SRC, > > .partition_vcpu_memory_access = true, > > }; > > int opt; > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c > > index 3c30d0045d8d..5ad9f2bc7369 100644 > > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > > @@ -308,11 +308,9 @@ static void help(char *name) > > printf(" -v: specify the number of vCPUs to run.\n"); > > printf(" -o: Overlap guest memory accesses instead of partitioning\n" > > " them into a separate region of memory for each vCPU.\n"); > > - printf(" -s: specify the type of memory that should be used to\n" > > - " back the guest data region.\n\n"); > > + backing_src_help("-s"); > > printf(" -x: Split the memory region into this number of memslots.\n" > > - " (default: 1)"); > > - backing_src_help(); > > + " (default: 1)\n"); > > puts(""); > > exit(0); > > } > > @@ -324,7 +322,7 @@ int main(int argc, char *argv[]) > > .iterations = TEST_HOST_LOOP_N, > > .wr_fract = 1, > > .partition_vcpu_memory_access = true, > > - .backing_src = VM_MEM_SRC_ANONYMOUS, > > + .backing_src = DEFAULT_VM_MEM_SRC, > > .slots = 1, > > }; > > int opt; > > diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h > > index d79be15dd3d2..2f09f2994733 100644 > > --- a/tools/testing/selftests/kvm/include/test_util.h > > +++ b/tools/testing/selftests/kvm/include/test_util.h > > @@ -68,6 +68,7 @@ struct timespec timespec_sub(struct timespec ts1, struct timespec ts2); > > struct timespec timespec_elapsed(struct timespec start); > > struct timespec timespec_div(struct timespec ts, int divisor); > > > > + > > New extra blank line. Added on purpose? No that's a mistake. Will fix in v2. > > > enum vm_mem_backing_src_type { > > VM_MEM_SRC_ANONYMOUS, > > VM_MEM_SRC_ANONYMOUS_THP, > > @@ -90,6 +91,8 @@ enum vm_mem_backing_src_type { > > NUM_SRC_TYPES, > > }; > > > > +#define DEFAULT_VM_MEM_SRC VM_MEM_SRC_ANONYMOUS > > + > > struct vm_mem_backing_src_alias { > > const char *name; > > uint32_t flag; > > @@ -100,7 +103,7 @@ size_t get_trans_hugepagesz(void); > > size_t get_def_hugetlb_pagesz(void); > > const struct vm_mem_backing_src_alias *vm_mem_backing_src_alias(uint32_t i); > > size_t get_backing_src_pagesz(uint32_t i); > > -void backing_src_help(void); > > +void backing_src_help(const char *flag); > > enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name); > > > > /* > > diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c > > index 0d04a7db7f24..36407cb0ec85 100644 > > --- a/tools/testing/selftests/kvm/kvm_page_table_test.c > > +++ b/tools/testing/selftests/kvm/kvm_page_table_test.c > > @@ -456,10 +456,7 @@ static void help(char *name) > > " (default: 1G)\n"); > > printf(" -v: specify the number of vCPUs to run\n" > > " (default: 1)\n"); > > - printf(" -s: specify the type of memory that should be used to\n" > > - " back the guest data region.\n" > > - " (default: anonymous)\n\n"); > > - backing_src_help(); > > + backing_src_help("-s"); > > puts(""); > > } > > > > @@ -468,7 +465,7 @@ int main(int argc, char *argv[]) > > int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); > > struct test_params p = { > > .test_mem_size = DEFAULT_TEST_MEM_SIZE, > > - .src_type = VM_MEM_SRC_ANONYMOUS, > > + .src_type = DEFAULT_VM_MEM_SRC, > > }; > > int opt; > > > > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c > > index af1031fed97f..ea23a86132ed 100644 > > --- a/tools/testing/selftests/kvm/lib/test_util.c > > +++ b/tools/testing/selftests/kvm/lib/test_util.c > > @@ -279,13 +279,22 @@ size_t get_backing_src_pagesz(uint32_t i) > > } > > } > > > > -void backing_src_help(void) > > +void print_available_backing_src_types(const char *prefix) > > This can probably be static. Ack. Will fix in v2. > > > { > > int i; > > > > - printf("Available backing src types:\n"); > > + printf("%sAvailable backing src types:\n", prefix); > > + > > for (i = 0; i < NUM_SRC_TYPES; i++) > > - printf("\t%s\n", vm_mem_backing_src_alias(i)->name); > > + printf("%s %s\n", prefix, vm_mem_backing_src_alias(i)->name); > > +} > > + > > +void backing_src_help(const char *flag) > > +{ > > + printf(" %s: specify the type of memory that should be used to\n" > > + " back the guest data region. (default: %s)\n", > > + flag, vm_mem_backing_src_alias(DEFAULT_VM_MEM_SRC)->name); > > + print_available_backing_src_types(" "); > > } > > > > enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name) > > @@ -296,7 +305,7 @@ enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name) > > if (!strcmp(type_name, vm_mem_backing_src_alias(i)->name)) > > return i; > > > > - backing_src_help(); > > + print_available_backing_src_types(""); > > TEST_FAIL("Unknown backing src type: %s", type_name); > > return -1; > > } > > -- > > 2.33.0.309.g3052b89438-goog > > > > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > Thanks, > drew > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation 2021-09-15 21:30 [PATCH 0/3] KVM: selftests: Small fixes for dirty_log_perf_test David Matlack 2021-09-15 21:30 ` [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test David Matlack 2021-09-15 21:30 ` [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src David Matlack @ 2021-09-15 21:30 ` David Matlack 2021-09-15 21:55 ` Ben Gardon 2 siblings, 1 reply; 14+ messages in thread From: David Matlack @ 2021-09-15 21:30 UTC (permalink / raw) To: Paolo Bonzini Cc: kvm, Andrew Jones, Ben Gardon, Axel Rasmussen, Yanan Wang, David Matlack The calculation to get the per-slot dirty bitmap was incorrect leading to a buffer overrun. Fix it by dividing the number of pages by BITS_PER_LONG, since each element of the bitmap is a long and there is one bit per page. Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") Signed-off-by: David Matlack <dmatlack@google.com> --- tools/testing/selftests/kvm/dirty_log_perf_test.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index 5ad9f2bc7369..0dd4626571e9 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -118,6 +118,12 @@ static inline void disable_dirty_logging(struct kvm_vm *vm, int slots) toggle_dirty_logging(vm, slots, false); } +static unsigned long *get_slot_bitmap(unsigned long *bitmap, uint64_t page_offset) +{ + /* Guaranteed to be evenly divisible by the TEST_ASSERT in run_test. */ + return &bitmap[page_offset / BITS_PER_LONG]; +} + static void get_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, uint64_t nr_pages) { @@ -126,7 +132,8 @@ static void get_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, for (i = 0; i < slots; i++) { int slot = PERF_TEST_MEM_SLOT_INDEX + i; - unsigned long *slot_bitmap = bitmap + i * slot_pages; + uint64_t page_offset = slot_pages * i; + unsigned long *slot_bitmap = get_slot_bitmap(bitmap, page_offset); kvm_vm_get_dirty_log(vm, slot, slot_bitmap); } @@ -140,7 +147,8 @@ static void clear_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, for (i = 0; i < slots; i++) { int slot = PERF_TEST_MEM_SLOT_INDEX + i; - unsigned long *slot_bitmap = bitmap + i * slot_pages; + uint64_t page_offset = slot_pages * i; + unsigned long *slot_bitmap = get_slot_bitmap(bitmap, page_offset); kvm_vm_clear_dirty_log(vm, slot, slot_bitmap, 0, slot_pages); } @@ -172,6 +180,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) guest_num_pages = vm_adjust_num_guest_pages(mode, guest_num_pages); host_num_pages = vm_num_host_pages(mode, guest_num_pages); bmap = bitmap_alloc(host_num_pages); + TEST_ASSERT((host_num_pages / p->slots) % BITS_PER_LONG == 0, + "The number of pages per slot must be divisible by %d.", + BITS_PER_LONG); if (dirty_log_manual_caps) { cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2; -- 2.33.0.309.g3052b89438-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation 2021-09-15 21:30 ` [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation David Matlack @ 2021-09-15 21:55 ` Ben Gardon 2021-09-16 8:49 ` Andrew Jones 0 siblings, 1 reply; 14+ messages in thread From: Ben Gardon @ 2021-09-15 21:55 UTC (permalink / raw) To: David Matlack Cc: Paolo Bonzini, kvm, Andrew Jones, Axel Rasmussen, Yanan Wang On Wed, Sep 15, 2021 at 2:30 PM David Matlack <dmatlack@google.com> wrote: > > The calculation to get the per-slot dirty bitmap was incorrect leading > to a buffer overrun. Fix it by dividing the number of pages by > BITS_PER_LONG, since each element of the bitmap is a long and there is > one bit per page. > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > Signed-off-by: David Matlack <dmatlack@google.com> I was a little confused initially because we're allocating only one dirty bitmap in userspace even when we have multiple slots, but that's not a problem. Reviewed-by: Ben Gardon <bgardon@google.com> > > --- > tools/testing/selftests/kvm/dirty_log_perf_test.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c > index 5ad9f2bc7369..0dd4626571e9 100644 > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > @@ -118,6 +118,12 @@ static inline void disable_dirty_logging(struct kvm_vm *vm, int slots) > toggle_dirty_logging(vm, slots, false); > } > > +static unsigned long *get_slot_bitmap(unsigned long *bitmap, uint64_t page_offset) > +{ > + /* Guaranteed to be evenly divisible by the TEST_ASSERT in run_test. */ > + return &bitmap[page_offset / BITS_PER_LONG]; > +} > + > static void get_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, > uint64_t nr_pages) > { > @@ -126,7 +132,8 @@ static void get_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, > > for (i = 0; i < slots; i++) { > int slot = PERF_TEST_MEM_SLOT_INDEX + i; > - unsigned long *slot_bitmap = bitmap + i * slot_pages; > + uint64_t page_offset = slot_pages * i; > + unsigned long *slot_bitmap = get_slot_bitmap(bitmap, page_offset); > > kvm_vm_get_dirty_log(vm, slot, slot_bitmap); > } > @@ -140,7 +147,8 @@ static void clear_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, > > for (i = 0; i < slots; i++) { > int slot = PERF_TEST_MEM_SLOT_INDEX + i; > - unsigned long *slot_bitmap = bitmap + i * slot_pages; > + uint64_t page_offset = slot_pages * i; > + unsigned long *slot_bitmap = get_slot_bitmap(bitmap, page_offset); > > kvm_vm_clear_dirty_log(vm, slot, slot_bitmap, 0, slot_pages); > } > @@ -172,6 +180,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) > guest_num_pages = vm_adjust_num_guest_pages(mode, guest_num_pages); > host_num_pages = vm_num_host_pages(mode, guest_num_pages); > bmap = bitmap_alloc(host_num_pages); > + TEST_ASSERT((host_num_pages / p->slots) % BITS_PER_LONG == 0, > + "The number of pages per slot must be divisible by %d.", > + BITS_PER_LONG); > > if (dirty_log_manual_caps) { > cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2; > -- > 2.33.0.309.g3052b89438-goog > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation 2021-09-15 21:55 ` Ben Gardon @ 2021-09-16 8:49 ` Andrew Jones 2021-09-16 16:37 ` David Matlack 2021-09-22 11:09 ` Paolo Bonzini 0 siblings, 2 replies; 14+ messages in thread From: Andrew Jones @ 2021-09-16 8:49 UTC (permalink / raw) To: Ben Gardon; +Cc: David Matlack, Paolo Bonzini, kvm, Axel Rasmussen, Yanan Wang On Wed, Sep 15, 2021 at 02:55:03PM -0700, Ben Gardon wrote: > On Wed, Sep 15, 2021 at 2:30 PM David Matlack <dmatlack@google.com> wrote: > > > > The calculation to get the per-slot dirty bitmap was incorrect leading > > to a buffer overrun. Fix it by dividing the number of pages by > > BITS_PER_LONG, since each element of the bitmap is a long and there is > > one bit per page. > > > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > > Signed-off-by: David Matlack <dmatlack@google.com> > > I was a little confused initially because we're allocating only one > dirty bitmap in userspace even when we have multiple slots, but that's > not a problem. It's also confusing to me. Wouldn't it be better to create a bitmap per slot? I think the new constraint that host mem must be a multiple of 64 is unfortunate. Thanks, drew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation 2021-09-16 8:49 ` Andrew Jones @ 2021-09-16 16:37 ` David Matlack 2021-09-22 11:09 ` Paolo Bonzini 1 sibling, 0 replies; 14+ messages in thread From: David Matlack @ 2021-09-16 16:37 UTC (permalink / raw) To: Andrew Jones; +Cc: Ben Gardon, Paolo Bonzini, kvm, Axel Rasmussen, Yanan Wang On Thu, Sep 16, 2021 at 1:49 AM Andrew Jones <drjones@redhat.com> wrote: > > On Wed, Sep 15, 2021 at 02:55:03PM -0700, Ben Gardon wrote: > > On Wed, Sep 15, 2021 at 2:30 PM David Matlack <dmatlack@google.com> wrote: > > > > > > The calculation to get the per-slot dirty bitmap was incorrect leading > > > to a buffer overrun. Fix it by dividing the number of pages by > > > BITS_PER_LONG, since each element of the bitmap is a long and there is > > > one bit per page. > > > > > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > > > Signed-off-by: David Matlack <dmatlack@google.com> > > > > I was a little confused initially because we're allocating only one > > dirty bitmap in userspace even when we have multiple slots, but that's > > not a problem. > > It's also confusing to me. Wouldn't it be better to create a bitmap per > slot? I think the new constraint that host mem must be a multiple of 64 > is unfortunate. I don't think think the multiple-of-64 (256K) constraint will matter much in practice. But it wouldn't be very complicated to switch to a bitmap per slot, and would prevent further confusion. I'll switch it over in v2. > > Thanks, > drew > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation 2021-09-16 8:49 ` Andrew Jones 2021-09-16 16:37 ` David Matlack @ 2021-09-22 11:09 ` Paolo Bonzini 2021-09-22 11:19 ` Andrew Jones 1 sibling, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2021-09-22 11:09 UTC (permalink / raw) To: Andrew Jones, Ben Gardon; +Cc: David Matlack, kvm, Axel Rasmussen, Yanan Wang On 16/09/21 10:49, Andrew Jones wrote: >> I was a little confused initially because we're allocating only one >> dirty bitmap in userspace even when we have multiple slots, but that's >> not a problem. > It's also confusing to me. Wouldn't it be better to create a bitmap per > slot? I think the new constraint that host mem must be a multiple of 64 > is unfortunate. Yeah, I wouldn't mind if someone took a look at that. Also because anyway this patch doesn't apply to master right now, I've queued 1-2 only. Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation 2021-09-22 11:09 ` Paolo Bonzini @ 2021-09-22 11:19 ` Andrew Jones 0 siblings, 0 replies; 14+ messages in thread From: Andrew Jones @ 2021-09-22 11:19 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Ben Gardon, David Matlack, kvm, Axel Rasmussen, Yanan Wang On Wed, Sep 22, 2021 at 01:09:57PM +0200, Paolo Bonzini wrote: > On 16/09/21 10:49, Andrew Jones wrote: > > > I was a little confused initially because we're allocating only one > > > dirty bitmap in userspace even when we have multiple slots, but that's > > > not a problem. > > It's also confusing to me. Wouldn't it be better to create a bitmap per > > slot? I think the new constraint that host mem must be a multiple of 64 > > is unfortunate. > > Yeah, I wouldn't mind if someone took a look at that. Also because anyway > this patch doesn't apply to master right now, I've queued 1-2 only. David posted a v2 on Sept. 17 with the bitmap split up per slot for patch 3/3. I think the other patches got some tweaks too. Thanks drew ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-09-22 11:20 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-15 21:30 [PATCH 0/3] KVM: selftests: Small fixes for dirty_log_perf_test David Matlack 2021-09-15 21:30 ` [PATCH 1/3] KVM: selftests: Change backing_src flag to -s in demand_paging_test David Matlack 2021-09-15 21:56 ` Ben Gardon 2021-09-16 8:34 ` Andrew Jones 2021-09-15 21:30 ` [PATCH 2/3] KVM: selftests: Refactor help message for -s backing_src David Matlack 2021-09-15 21:55 ` Ben Gardon 2021-09-16 8:41 ` Andrew Jones 2021-09-16 16:24 ` David Matlack 2021-09-15 21:30 ` [PATCH 3/3] KVM: selftests: Fix dirty bitmap offset calculation David Matlack 2021-09-15 21:55 ` Ben Gardon 2021-09-16 8:49 ` Andrew Jones 2021-09-16 16:37 ` David Matlack 2021-09-22 11:09 ` Paolo Bonzini 2021-09-22 11:19 ` Andrew Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.