* [PATCH v6 0/5] dirty_log_perf_test vCPU pinning
@ 2022-10-21 21:18 Vipin Sharma
2022-10-21 21:18 ` [PATCH v6 1/5] KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test Vipin Sharma
` (4 more replies)
0 siblings, 5 replies; 20+ messages in thread
From: Vipin Sharma @ 2022-10-21 21:18 UTC (permalink / raw)
To: seanjc, pbonzini, dmatlack
Cc: andrew.jones, wei.w.wang, kvm, linux-kernel, Vipin Sharma
Pin vCPUs to a host physical CPUs (pCPUs) in dirty_log_perf_test and
optionally pin the main application thread to a physical cpu if
provided. All tests based on perf_test_util framework can take advantage
of it if needed.
While at it, I changed atoi() to atoi_paranoid(), atoi_positive,
atoi_non_negative() in other tests, sorted command line options
alphabetically in dirty_log_perf_test, and added break between -e and -g
which was missed in original commit when -e was introduced.
v6:
- Updated the shortlog of Patch 5.
- Changed formatting of help text of -c in dirty_log_perf_test
v5: https://lore.kernel.org/lkml/20221010220538.1154054-1-vipinsh@google.com/
- Added atoi_postive() and atoi_non_negative() APIs for string parsing.
- Using sched_getaffinity() to verify if a pCPU is allowed or not.
- Changed Suggested-by to add only person came up with original idea of
pinning.
- Updated strings and commit messages.
v4: https://lore.kernel.org/lkml/20221006171133.372359-1-vipinsh@google.com/
- Moved boolean to check vCPUs pinning from perf_test_vcpu_args to
perf_test_args.
- Changed assert statements to make error more descriptive.
- Modified break statement between 'e' and 'g' option in v3 by not copying
dirty_log_manual_caps = 0 to 'e'.
v3: https://lore.kernel.org/lkml/20220826184500.1940077-1-vipinsh@google.com
- Moved atoi_paranoid() to test_util.c and replaced all atoi() usage
with atoi_paranoid()
- Sorted command line options alphabetically.
- Instead of creating a vcpu thread on a specific pcpu the thread will
migrate to the provided pcpu after its creation.
- Decoupled -e and -g option.
v2: https://lore.kernel.org/lkml/20220819210737.763135-1-vipinsh@google.com/
- Removed -d option.
- One cpu list passed as option, cpus for vcpus, followed by
application thread cpu.
- Added paranoid cousin of atoi().
v1: https://lore.kernel.org/lkml/20220817152956.4056410-1-vipinsh@google.com
Vipin Sharma (5):
KVM: selftests: Add missing break between -e and -g option in
dirty_log_perf_test
KVM: selftests: Put command line options in alphabetical order in
dirty_log_perf_test
KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi()
KVM: selftests: Add atoi_positive() and atoi_non_negative() for input
validation
KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs
.../selftests/kvm/aarch64/arch_timer.c | 25 ++-----
.../testing/selftests/kvm/aarch64/vgic_irq.c | 6 +-
.../selftests/kvm/access_tracking_perf_test.c | 2 +-
.../selftests/kvm/demand_paging_test.c | 4 +-
.../selftests/kvm/dirty_log_perf_test.c | 61 +++++++++++------
.../selftests/kvm/include/perf_test_util.h | 6 ++
.../testing/selftests/kvm/include/test_util.h | 4 ++
.../selftests/kvm/kvm_page_table_test.c | 4 +-
.../selftests/kvm/lib/perf_test_util.c | 65 ++++++++++++++++++-
tools/testing/selftests/kvm/lib/test_util.c | 35 ++++++++++
.../selftests/kvm/max_guest_memory_test.c | 7 +-
.../kvm/memslot_modification_stress_test.c | 6 +-
.../testing/selftests/kvm/memslot_perf_test.c | 24 ++-----
.../selftests/kvm/set_memory_region_test.c | 2 +-
.../selftests/kvm/x86_64/nx_huge_pages_test.c | 4 +-
15 files changed, 177 insertions(+), 78 deletions(-)
--
2.38.0.135.g90850a2211-goog
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v6 1/5] KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test 2022-10-21 21:18 [PATCH v6 0/5] dirty_log_perf_test vCPU pinning Vipin Sharma @ 2022-10-21 21:18 ` Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 2/5] KVM: selftests: Put command line options in alphabetical order " Vipin Sharma ` (3 subsequent siblings) 4 siblings, 0 replies; 20+ messages in thread From: Vipin Sharma @ 2022-10-21 21:18 UTC (permalink / raw) To: seanjc, pbonzini, dmatlack Cc: andrew.jones, wei.w.wang, kvm, linux-kernel, Vipin Sharma Passing -e option (Run VCPUs while dirty logging is being disabled) in dirty_log_perf_test also unintentionally enables -g (Do not enable KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2). Add break between two switch case logic. Fixes: cfe12e64b065 ("KVM: selftests: Add an option to run vCPUs while disabling dirty logging") Signed-off-by: Vipin Sharma <vipinsh@google.com> --- tools/testing/selftests/kvm/dirty_log_perf_test.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index f99e39a672d3..56e08da3a87f 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -411,6 +411,7 @@ int main(int argc, char *argv[]) case 'e': /* 'e' is for evil. */ run_vcpus_while_disabling_dirty_logging = true; + break; case 'g': dirty_log_manual_caps = 0; break; -- 2.38.0.135.g90850a2211-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v6 2/5] KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test 2022-10-21 21:18 [PATCH v6 0/5] dirty_log_perf_test vCPU pinning Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 1/5] KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test Vipin Sharma @ 2022-10-21 21:18 ` Vipin Sharma 2022-10-26 2:09 ` Wang, Wei W 2022-10-21 21:18 ` [PATCH v6 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() Vipin Sharma ` (2 subsequent siblings) 4 siblings, 1 reply; 20+ messages in thread From: Vipin Sharma @ 2022-10-21 21:18 UTC (permalink / raw) To: seanjc, pbonzini, dmatlack Cc: andrew.jones, wei.w.wang, kvm, linux-kernel, Vipin Sharma There are 13 command line options and they are not in any order. Put them in alphabetical order to make it easy to add new options. No functional change intended. Signed-off-by: Vipin Sharma <vipinsh@google.com> --- .../selftests/kvm/dirty_log_perf_test.c | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index 56e08da3a87f..5bb6954b2358 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -406,50 +406,52 @@ int main(int argc, char *argv[]) guest_modes_append_default(); - while ((opt = getopt(argc, argv, "eghi:p:m:nb:f:v:os:x:")) != -1) { + while ((opt = getopt(argc, argv, "b:ef:ghi:m:nop:s:v:x:")) != -1) { switch (opt) { + case 'b': + guest_percpu_mem_size = parse_size(optarg); + break; case 'e': /* 'e' is for evil. */ run_vcpus_while_disabling_dirty_logging = true; break; + case 'f': + p.wr_fract = atoi(optarg); + TEST_ASSERT(p.wr_fract >= 1, + "Write fraction cannot be less than one"); + break; case 'g': dirty_log_manual_caps = 0; break; + case 'h': + help(argv[0]); + break; case 'i': p.iterations = atoi(optarg); break; - case 'p': - p.phys_offset = strtoull(optarg, NULL, 0); - break; case 'm': guest_modes_cmdline(optarg); break; case 'n': perf_test_args.nested = true; break; - case 'b': - guest_percpu_mem_size = parse_size(optarg); + case 'o': + p.partition_vcpu_memory_access = false; break; - case 'f': - p.wr_fract = atoi(optarg); - TEST_ASSERT(p.wr_fract >= 1, - "Write fraction cannot be less than one"); + case 'p': + p.phys_offset = strtoull(optarg, NULL, 0); + break; + case 's': + p.backing_src = parse_backing_src_type(optarg); break; case 'v': nr_vcpus = atoi(optarg); TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); break; - case 'o': - p.partition_vcpu_memory_access = false; - break; - case 's': - p.backing_src = parse_backing_src_type(optarg); - break; case 'x': p.slots = atoi(optarg); break; - case 'h': default: help(argv[0]); break; -- 2.38.0.135.g90850a2211-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* RE: [PATCH v6 2/5] KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test 2022-10-21 21:18 ` [PATCH v6 2/5] KVM: selftests: Put command line options in alphabetical order " Vipin Sharma @ 2022-10-26 2:09 ` Wang, Wei W 2022-10-26 15:04 ` Sean Christopherson 0 siblings, 1 reply; 20+ messages in thread From: Wang, Wei W @ 2022-10-26 2:09 UTC (permalink / raw) To: Vipin Sharma, Christopherson,, Sean, pbonzini, dmatlack Cc: andrew.jones, kvm, linux-kernel On Saturday, October 22, 2022 5:18 AM, Vipin Sharma wrote: > There are 13 command line options and they are not in any order. Put them in > alphabetical order to make it easy to add new options. > > No functional change intended. > > Signed-off-by: Vipin Sharma <vipinsh@google.com> > --- > .../selftests/kvm/dirty_log_perf_test.c | 36 ++++++++++--------- > 1 file changed, 19 insertions(+), 17 deletions(-) > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c > b/tools/testing/selftests/kvm/dirty_log_perf_test.c > index 56e08da3a87f..5bb6954b2358 100644 > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > @@ -406,50 +406,52 @@ int main(int argc, char *argv[]) > > guest_modes_append_default(); > > - while ((opt = getopt(argc, argv, "eghi:p:m:nb:f:v:os:x:")) != -1) { > + while ((opt = getopt(argc, argv, "b:ef:ghi:m:nop:s:v:x:")) != -1) { > switch (opt) { > + case 'b': > + guest_percpu_mem_size = parse_size(optarg); > + break; "break" wasn't there. This is kind of a functional change (i.e. bug fixing), and would be better to be moved to patch 1. Other parts look good to me. Reviewed-by: Wei Wang <wei.w.wang@intel.com> > case 'e': > /* 'e' is for evil. */ > run_vcpus_while_disabling_dirty_logging = true; > break; > + case 'f': > + p.wr_fract = atoi(optarg); > + TEST_ASSERT(p.wr_fract >= 1, > + "Write fraction cannot be less than one"); > + break; > case 'g': > dirty_log_manual_caps = 0; > break; > + case 'h': > + help(argv[0]); > + break; > case 'i': > p.iterations = atoi(optarg); > break; > - case 'p': > - p.phys_offset = strtoull(optarg, NULL, 0); > - break; > case 'm': > guest_modes_cmdline(optarg); > break; > case 'n': > perf_test_args.nested = true; > break; > - case 'b': > - guest_percpu_mem_size = parse_size(optarg); > + case 'o': > + p.partition_vcpu_memory_access = false; > break; > - case 'f': > - p.wr_fract = atoi(optarg); > - TEST_ASSERT(p.wr_fract >= 1, > - "Write fraction cannot be less than one"); > + case 'p': > + p.phys_offset = strtoull(optarg, NULL, 0); > + break; > + case 's': > + p.backing_src = parse_backing_src_type(optarg); > break; > case 'v': > nr_vcpus = atoi(optarg); > TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, > "Invalid number of vcpus, must be between 1 and %d", > max_vcpus); > break; > - case 'o': > - p.partition_vcpu_memory_access = false; > - break; > - case 's': > - p.backing_src = parse_backing_src_type(optarg); > - break; > case 'x': > p.slots = atoi(optarg); > break; > - case 'h': > default: > help(argv[0]); > break; > -- > 2.38.0.135.g90850a2211-goog ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/5] KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test 2022-10-26 2:09 ` Wang, Wei W @ 2022-10-26 15:04 ` Sean Christopherson 2022-10-26 17:45 ` Vipin Sharma 0 siblings, 1 reply; 20+ messages in thread From: Sean Christopherson @ 2022-10-26 15:04 UTC (permalink / raw) To: Wang, Wei W Cc: Vipin Sharma, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Wed, Oct 26, 2022, Wang, Wei W wrote: > On Saturday, October 22, 2022 5:18 AM, Vipin Sharma wrote: > > There are 13 command line options and they are not in any order. Put them in > > alphabetical order to make it easy to add new options. > > > > No functional change intended. > > > > Signed-off-by: Vipin Sharma <vipinsh@google.com> > > --- > > .../selftests/kvm/dirty_log_perf_test.c | 36 ++++++++++--------- > > 1 file changed, 19 insertions(+), 17 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c > > b/tools/testing/selftests/kvm/dirty_log_perf_test.c > > index 56e08da3a87f..5bb6954b2358 100644 > > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > > @@ -406,50 +406,52 @@ int main(int argc, char *argv[]) > > > > guest_modes_append_default(); > > > > - while ((opt = getopt(argc, argv, "eghi:p:m:nb:f:v:os:x:")) != -1) { > > + while ((opt = getopt(argc, argv, "b:ef:ghi:m:nop:s:v:x:")) != -1) { > > switch (opt) { > > + case 'b': > > + guest_percpu_mem_size = parse_size(optarg); > > + break; > > "break" wasn't there. > This is kind of a functional change (i.e. bug fixing), and would be better to be > moved to patch 1. Ya, good catch. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 2/5] KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test 2022-10-26 15:04 ` Sean Christopherson @ 2022-10-26 17:45 ` Vipin Sharma 0 siblings, 0 replies; 20+ messages in thread From: Vipin Sharma @ 2022-10-26 17:45 UTC (permalink / raw) To: Sean Christopherson Cc: Wang, Wei W, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Wed, Oct 26, 2022 at 8:04 AM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Oct 26, 2022, Wang, Wei W wrote: > > On Saturday, October 22, 2022 5:18 AM, Vipin Sharma wrote: > > > There are 13 command line options and they are not in any order. Put them in > > > alphabetical order to make it easy to add new options. > > > > > > No functional change intended. > > > > > > Signed-off-by: Vipin Sharma <vipinsh@google.com> > > > --- > > > .../selftests/kvm/dirty_log_perf_test.c | 36 ++++++++++--------- > > > 1 file changed, 19 insertions(+), 17 deletions(-) > > > > > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c > > > b/tools/testing/selftests/kvm/dirty_log_perf_test.c > > > index 56e08da3a87f..5bb6954b2358 100644 > > > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > > > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > > > @@ -406,50 +406,52 @@ int main(int argc, char *argv[]) > > > > > > guest_modes_append_default(); > > > > > > - while ((opt = getopt(argc, argv, "eghi:p:m:nb:f:v:os:x:")) != -1) { > > > + while ((opt = getopt(argc, argv, "b:ef:ghi:m:nop:s:v:x:")) != -1) { > > > switch (opt) { > > > + case 'b': > > > + guest_percpu_mem_size = parse_size(optarg); > > > + break; > > > > "break" wasn't there. > > This is kind of a functional change (i.e. bug fixing), and would be better to be > > moved to patch 1. > > Ya, good catch. diff is not very good in showing intuitive diff. "break" was there, it just added "case 'o'" before it and didn't move break. - case 'b': - guest_percpu_mem_size = parse_size(optarg); + case 'o': + p.partition_vcpu_memory_access = false; break; ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v6 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() 2022-10-21 21:18 [PATCH v6 0/5] dirty_log_perf_test vCPU pinning Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 1/5] KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 2/5] KVM: selftests: Put command line options in alphabetical order " Vipin Sharma @ 2022-10-21 21:18 ` Vipin Sharma 2022-10-26 2:16 ` Wang, Wei W 2022-10-21 21:18 ` [PATCH v6 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs Vipin Sharma 4 siblings, 1 reply; 20+ messages in thread From: Vipin Sharma @ 2022-10-21 21:18 UTC (permalink / raw) To: seanjc, pbonzini, dmatlack Cc: andrew.jones, wei.w.wang, kvm, linux-kernel, Vipin Sharma atoi() doesn't detect errors. There is no way to know that a 0 return is correct conversion or due to an error. Introduce atoi_paranoid() to detect errors and provide correct conversion. Replace all atoi() calls with atoi_paranoid(). Signed-off-by: Vipin Sharma <vipinsh@google.com> Suggested-by: David Matlack <dmatlack@google.com> Suggested-by: Sean Christopherson <seanjc@google.com> --- .../selftests/kvm/aarch64/arch_timer.c | 8 ++++---- .../testing/selftests/kvm/aarch64/vgic_irq.c | 6 +++--- .../selftests/kvm/access_tracking_perf_test.c | 2 +- .../selftests/kvm/demand_paging_test.c | 2 +- .../selftests/kvm/dirty_log_perf_test.c | 8 ++++---- .../testing/selftests/kvm/include/test_util.h | 2 ++ .../selftests/kvm/kvm_page_table_test.c | 2 +- tools/testing/selftests/kvm/lib/test_util.c | 19 +++++++++++++++++++ .../selftests/kvm/max_guest_memory_test.c | 6 +++--- .../kvm/memslot_modification_stress_test.c | 4 ++-- .../testing/selftests/kvm/memslot_perf_test.c | 10 +++++----- .../selftests/kvm/set_memory_region_test.c | 2 +- .../selftests/kvm/x86_64/nx_huge_pages_test.c | 4 ++-- 13 files changed, 48 insertions(+), 27 deletions(-) diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c index 574eb73f0e90..251e7ff04883 100644 --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c @@ -414,7 +414,7 @@ static bool parse_args(int argc, char *argv[]) while ((opt = getopt(argc, argv, "hn:i:p:m:")) != -1) { switch (opt) { case 'n': - test_args.nr_vcpus = atoi(optarg); + test_args.nr_vcpus = atoi_paranoid(optarg); if (test_args.nr_vcpus <= 0) { pr_info("Positive value needed for -n\n"); goto err; @@ -425,21 +425,21 @@ static bool parse_args(int argc, char *argv[]) } break; case 'i': - test_args.nr_iter = atoi(optarg); + test_args.nr_iter = atoi_paranoid(optarg); if (test_args.nr_iter <= 0) { pr_info("Positive value needed for -i\n"); goto err; } break; case 'p': - test_args.timer_period_ms = atoi(optarg); + test_args.timer_period_ms = atoi_paranoid(optarg); if (test_args.timer_period_ms <= 0) { pr_info("Positive value needed for -p\n"); goto err; } break; case 'm': - test_args.migration_freq_ms = atoi(optarg); + test_args.migration_freq_ms = atoi_paranoid(optarg); if (test_args.migration_freq_ms < 0) { pr_info("0 or positive value needed for -m\n"); goto err; diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c index 17417220a083..ae90b718070a 100644 --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c @@ -824,16 +824,16 @@ int main(int argc, char **argv) while ((opt = getopt(argc, argv, "hn:e:l:")) != -1) { switch (opt) { case 'n': - nr_irqs = atoi(optarg); + nr_irqs = atoi_paranoid(optarg); if (nr_irqs > 1024 || nr_irqs % 32) help(argv[0]); break; case 'e': - eoi_split = (bool)atoi(optarg); + eoi_split = (bool)atoi_paranoid(optarg); default_args = false; break; case 'l': - level_sensitive = (bool)atoi(optarg); + level_sensitive = (bool)atoi_paranoid(optarg); default_args = false; break; case 'h': diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c index 76c583a07ea2..c6bcc5301e2c 100644 --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c @@ -368,7 +368,7 @@ int main(int argc, char *argv[]) params.vcpu_memory_bytes = parse_size(optarg); break; case 'v': - params.nr_vcpus = atoi(optarg); + params.nr_vcpus = atoi_paranoid(optarg); break; case 'o': overlap_memory_access = true; diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index 779ae54f89c4..82597fb04146 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -427,7 +427,7 @@ int main(int argc, char *argv[]) p.src_type = parse_backing_src_type(optarg); break; case 'v': - nr_vcpus = atoi(optarg); + nr_vcpus = atoi_paranoid(optarg); TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); break; diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index 5bb6954b2358..ecda802b78ff 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -416,7 +416,7 @@ int main(int argc, char *argv[]) run_vcpus_while_disabling_dirty_logging = true; break; case 'f': - p.wr_fract = atoi(optarg); + p.wr_fract = atoi_paranoid(optarg); TEST_ASSERT(p.wr_fract >= 1, "Write fraction cannot be less than one"); break; @@ -427,7 +427,7 @@ int main(int argc, char *argv[]) help(argv[0]); break; case 'i': - p.iterations = atoi(optarg); + p.iterations = atoi_paranoid(optarg); break; case 'm': guest_modes_cmdline(optarg); @@ -445,12 +445,12 @@ int main(int argc, char *argv[]) p.backing_src = parse_backing_src_type(optarg); break; case 'v': - nr_vcpus = atoi(optarg); + nr_vcpus = atoi_paranoid(optarg); TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); break; case 'x': - p.slots = atoi(optarg); + p.slots = atoi_paranoid(optarg); break; default: help(argv[0]); diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h index befc754ce9b3..feae42863759 100644 --- a/tools/testing/selftests/kvm/include/test_util.h +++ b/tools/testing/selftests/kvm/include/test_util.h @@ -152,4 +152,6 @@ static inline void *align_ptr_up(void *x, size_t size) return (void *)align_up((unsigned long)x, size); } +int atoi_paranoid(const char *num_str); + #endif /* SELFTEST_KVM_TEST_UTIL_H */ diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c index f42c6ac6d71d..ea7feb69bb88 100644 --- a/tools/testing/selftests/kvm/kvm_page_table_test.c +++ b/tools/testing/selftests/kvm/kvm_page_table_test.c @@ -461,7 +461,7 @@ int main(int argc, char *argv[]) p.test_mem_size = parse_size(optarg); break; case 'v': - nr_vcpus = atoi(optarg); + nr_vcpus = atoi_paranoid(optarg); TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); break; diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c index 6d23878bbfe1..ec0f070a6f21 100644 --- a/tools/testing/selftests/kvm/lib/test_util.c +++ b/tools/testing/selftests/kvm/lib/test_util.c @@ -334,3 +334,22 @@ long get_run_delay(void) return val[1]; } + +int atoi_paranoid(const char *num_str) +{ + char *end_ptr; + long num; + + errno = 0; + num = strtol(num_str, &end_ptr, 10); + TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str); + TEST_ASSERT(num_str != end_ptr, + "strtol(\"%s\") didn't find a valid integer.\n", num_str); + TEST_ASSERT(*end_ptr == '\0', + "strtol(\"%s\") failed to parse trailing characters \"%s\".\n", + num_str, end_ptr); + TEST_ASSERT(num >= INT_MIN && num <= INT_MAX, + "%ld not in range of [%d, %d]", num, INT_MIN, INT_MAX); + + return num; +} diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c index 9a6e4f3ad6b5..1595b73dc09a 100644 --- a/tools/testing/selftests/kvm/max_guest_memory_test.c +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c @@ -193,15 +193,15 @@ int main(int argc, char *argv[]) while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) { switch (opt) { case 'c': - nr_vcpus = atoi(optarg); + nr_vcpus = atoi_paranoid(optarg); TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0"); break; case 'm': - max_mem = atoi(optarg) * size_1gb; + max_mem = atoi_paranoid(optarg) * size_1gb; TEST_ASSERT(max_mem > 0, "memory size must be >0"); break; case 's': - slot_size = atoi(optarg) * size_1gb; + slot_size = atoi_paranoid(optarg) * size_1gb; TEST_ASSERT(slot_size > 0, "slot size must be >0"); break; case 'H': diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c index 6ee7e1dde404..865276993ffb 100644 --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c @@ -166,7 +166,7 @@ int main(int argc, char *argv[]) guest_percpu_mem_size = parse_size(optarg); break; case 'v': - nr_vcpus = atoi(optarg); + nr_vcpus = atoi_paranoid(optarg); TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); @@ -175,7 +175,7 @@ int main(int argc, char *argv[]) p.partition_vcpu_memory_access = false; break; case 'i': - p.nr_memslot_modifications = atoi(optarg); + p.nr_memslot_modifications = atoi_paranoid(optarg); break; case 'h': default: diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c index 44995446d942..4bae9e3f5ca1 100644 --- a/tools/testing/selftests/kvm/memslot_perf_test.c +++ b/tools/testing/selftests/kvm/memslot_perf_test.c @@ -885,21 +885,21 @@ static bool parse_args(int argc, char *argv[], map_unmap_verify = true; break; case 's': - targs->nslots = atoi(optarg); + targs->nslots = atoi_paranoid(optarg); if (targs->nslots <= 0 && targs->nslots != -1) { pr_info("Slot count cap has to be positive or -1 for no cap\n"); return false; } break; case 'f': - targs->tfirst = atoi(optarg); + targs->tfirst = atoi_paranoid(optarg); if (targs->tfirst < 0) { pr_info("First test to run has to be non-negative\n"); return false; } break; case 'e': - targs->tlast = atoi(optarg); + targs->tlast = atoi_paranoid(optarg); if (targs->tlast < 0 || targs->tlast >= NTESTS) { pr_info("Last test to run has to be non-negative and less than %zu\n", NTESTS); @@ -907,14 +907,14 @@ static bool parse_args(int argc, char *argv[], } break; case 'l': - targs->seconds = atoi(optarg); + targs->seconds = atoi_paranoid(optarg); if (targs->seconds < 0) { pr_info("Test length in seconds has to be non-negative\n"); return false; } break; case 'r': - targs->runs = atoi(optarg); + targs->runs = atoi_paranoid(optarg); if (targs->runs <= 0) { pr_info("Runs per test has to be positive\n"); return false; diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c index 0d55f508d595..c366949c8362 100644 --- a/tools/testing/selftests/kvm/set_memory_region_test.c +++ b/tools/testing/selftests/kvm/set_memory_region_test.c @@ -407,7 +407,7 @@ int main(int argc, char *argv[]) #ifdef __x86_64__ if (argc > 1) - loops = atoi(argv[1]); + loops = atoi_paranoid(argv[1]); else loops = 10; diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c index 59ffe7fd354f..354b6902849c 100644 --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c @@ -241,10 +241,10 @@ int main(int argc, char **argv) while ((opt = getopt(argc, argv, "hp:t:r")) != -1) { switch (opt) { case 'p': - reclaim_period_ms = atoi(optarg); + reclaim_period_ms = atoi_paranoid(optarg); break; case 't': - token = atoi(optarg); + token = atoi_paranoid(optarg); break; case 'r': reboot_permissions = true; -- 2.38.0.135.g90850a2211-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* RE: [PATCH v6 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() 2022-10-21 21:18 ` [PATCH v6 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() Vipin Sharma @ 2022-10-26 2:16 ` Wang, Wei W 2022-10-26 15:10 ` Sean Christopherson 0 siblings, 1 reply; 20+ messages in thread From: Wang, Wei W @ 2022-10-26 2:16 UTC (permalink / raw) To: Vipin Sharma, Christopherson,, Sean, pbonzini, dmatlack Cc: andrew.jones, kvm, linux-kernel On Saturday, October 22, 2022 5:18 AM, Vipin Sharma wrote: > +int atoi_paranoid(const char *num_str) > +{ > + char *end_ptr; > + long num; > + > + errno = 0; > + num = strtol(num_str, &end_ptr, 10); Why not use strtoull here? Negative numbers will result in a huge "unsigned long long" number, and this will be captured by your TEST_ASSERT(num >= INT_MIN) below. Then we don't need patch 4, I think. > + TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str); > + TEST_ASSERT(num_str != end_ptr, > + "strtol(\"%s\") didn't find a valid integer.\n", num_str); > + TEST_ASSERT(*end_ptr == '\0', > + "strtol(\"%s\") failed to parse trailing characters \"%s\".\n", > + num_str, end_ptr); > + TEST_ASSERT(num >= INT_MIN && num <= INT_MAX, > + "%ld not in range of [%d, %d]", num, INT_MIN, INT_MAX); > + > + return num; > +} ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() 2022-10-26 2:16 ` Wang, Wei W @ 2022-10-26 15:10 ` Sean Christopherson 0 siblings, 0 replies; 20+ messages in thread From: Sean Christopherson @ 2022-10-26 15:10 UTC (permalink / raw) To: Wang, Wei W Cc: Vipin Sharma, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Wed, Oct 26, 2022, Wang, Wei W wrote: > On Saturday, October 22, 2022 5:18 AM, Vipin Sharma wrote: > > +int atoi_paranoid(const char *num_str) > > +{ > > + char *end_ptr; > > + long num; > > + > > + errno = 0; > > + num = strtol(num_str, &end_ptr, 10); > > Why not use strtoull here? This intended to be a drop in replacement for atoi(), which allows negative numbers. > Negative numbers will result in a huge "unsigned long long" number, > and this will be captured by your TEST_ASSERT(num >= INT_MIN) below. As above, we want to allow negative numbers, e.g. memslot_perf_test.c uses '-1' to indicate "as many slots as possible". It's unlikely a test will As unlikely as > Then we don't need patch 4, I think. Even if this low level helper disallowed negative numbers, patch 4 still has value in that the wrappers make the code self-documenting, i.e. makes it very obvious what input values are allowed. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v6 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation 2022-10-21 21:18 [PATCH v6 0/5] dirty_log_perf_test vCPU pinning Vipin Sharma ` (2 preceding siblings ...) 2022-10-21 21:18 ` [PATCH v6 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() Vipin Sharma @ 2022-10-21 21:18 ` Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs Vipin Sharma 4 siblings, 0 replies; 20+ messages in thread From: Vipin Sharma @ 2022-10-21 21:18 UTC (permalink / raw) To: seanjc, pbonzini, dmatlack Cc: andrew.jones, wei.w.wang, kvm, linux-kernel, Vipin Sharma Many KVM selftests take command line arguments which are supposed to be positive (>0) or non-negative (>=0). Some tests do these validation and some missed adding the check. Add atoi_positive() and atoi_non_negative() to validate inputs in selftests before proceeding to use those values. Signed-off-by: Vipin Sharma <vipinsh@google.com> --- .../selftests/kvm/aarch64/arch_timer.c | 25 ++++--------------- .../testing/selftests/kvm/aarch64/vgic_irq.c | 2 +- .../selftests/kvm/access_tracking_perf_test.c | 2 +- .../selftests/kvm/demand_paging_test.c | 4 +-- .../selftests/kvm/dirty_log_perf_test.c | 12 ++++----- .../testing/selftests/kvm/include/test_util.h | 2 ++ .../selftests/kvm/kvm_page_table_test.c | 4 +-- tools/testing/selftests/kvm/lib/test_util.c | 16 ++++++++++++ .../selftests/kvm/max_guest_memory_test.c | 7 +++--- .../kvm/memslot_modification_stress_test.c | 6 ++--- .../testing/selftests/kvm/memslot_perf_test.c | 22 ++++------------ .../selftests/kvm/set_memory_region_test.c | 2 +- 12 files changed, 46 insertions(+), 58 deletions(-) diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c index 251e7ff04883..24dffcaf7a9f 100644 --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c @@ -414,36 +414,21 @@ static bool parse_args(int argc, char *argv[]) while ((opt = getopt(argc, argv, "hn:i:p:m:")) != -1) { switch (opt) { case 'n': - test_args.nr_vcpus = atoi_paranoid(optarg); - if (test_args.nr_vcpus <= 0) { - pr_info("Positive value needed for -n\n"); - goto err; - } else if (test_args.nr_vcpus > KVM_MAX_VCPUS) { + test_args.nr_vcpus = atoi_positive(optarg); + if (test_args.nr_vcpus > KVM_MAX_VCPUS) { pr_info("Max allowed vCPUs: %u\n", KVM_MAX_VCPUS); goto err; } break; case 'i': - test_args.nr_iter = atoi_paranoid(optarg); - if (test_args.nr_iter <= 0) { - pr_info("Positive value needed for -i\n"); - goto err; - } + test_args.nr_iter = atoi_positive(optarg); break; case 'p': - test_args.timer_period_ms = atoi_paranoid(optarg); - if (test_args.timer_period_ms <= 0) { - pr_info("Positive value needed for -p\n"); - goto err; - } + test_args.timer_period_ms = atoi_positive(optarg); break; case 'm': - test_args.migration_freq_ms = atoi_paranoid(optarg); - if (test_args.migration_freq_ms < 0) { - pr_info("0 or positive value needed for -m\n"); - goto err; - } + test_args.migration_freq_ms = atoi_non_negative(optarg); break; case 'h': default: diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c index ae90b718070a..d7da5f24db35 100644 --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c @@ -824,7 +824,7 @@ int main(int argc, char **argv) while ((opt = getopt(argc, argv, "hn:e:l:")) != -1) { switch (opt) { case 'n': - nr_irqs = atoi_paranoid(optarg); + nr_irqs = atoi_non_negative(optarg); if (nr_irqs > 1024 || nr_irqs % 32) help(argv[0]); break; diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c index c6bcc5301e2c..b30500cc197e 100644 --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c @@ -368,7 +368,7 @@ int main(int argc, char *argv[]) params.vcpu_memory_bytes = parse_size(optarg); break; case 'v': - params.nr_vcpus = atoi_paranoid(optarg); + params.nr_vcpus = atoi_positive(optarg); break; case 'o': overlap_memory_access = true; diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index 82597fb04146..dcdb6964b1dc 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -427,8 +427,8 @@ int main(int argc, char *argv[]) p.src_type = parse_backing_src_type(optarg); break; case 'v': - nr_vcpus = atoi_paranoid(optarg); - TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, + nr_vcpus = atoi_positive(optarg); + TEST_ASSERT(nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); break; case 'o': diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index ecda802b78ff..618598ddd993 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -416,9 +416,7 @@ int main(int argc, char *argv[]) run_vcpus_while_disabling_dirty_logging = true; break; case 'f': - p.wr_fract = atoi_paranoid(optarg); - TEST_ASSERT(p.wr_fract >= 1, - "Write fraction cannot be less than one"); + p.wr_fract = atoi_positive(optarg); break; case 'g': dirty_log_manual_caps = 0; @@ -427,7 +425,7 @@ int main(int argc, char *argv[]) help(argv[0]); break; case 'i': - p.iterations = atoi_paranoid(optarg); + p.iterations = atoi_positive(optarg); break; case 'm': guest_modes_cmdline(optarg); @@ -445,12 +443,12 @@ int main(int argc, char *argv[]) p.backing_src = parse_backing_src_type(optarg); break; case 'v': - nr_vcpus = atoi_paranoid(optarg); - TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, + nr_vcpus = atoi_positive(optarg); + TEST_ASSERT(nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); break; case 'x': - p.slots = atoi_paranoid(optarg); + p.slots = atoi_positive(optarg); break; default: help(argv[0]); diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h index feae42863759..9c7b2c186a48 100644 --- a/tools/testing/selftests/kvm/include/test_util.h +++ b/tools/testing/selftests/kvm/include/test_util.h @@ -153,5 +153,7 @@ static inline void *align_ptr_up(void *x, size_t size) } int atoi_paranoid(const char *num_str); +uint32_t atoi_positive(const char *num_str); +uint32_t atoi_non_negative(const char *num_str); #endif /* SELFTEST_KVM_TEST_UTIL_H */ diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c index ea7feb69bb88..2f62e19976fd 100644 --- a/tools/testing/selftests/kvm/kvm_page_table_test.c +++ b/tools/testing/selftests/kvm/kvm_page_table_test.c @@ -461,8 +461,8 @@ int main(int argc, char *argv[]) p.test_mem_size = parse_size(optarg); break; case 'v': - nr_vcpus = atoi_paranoid(optarg); - TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, + nr_vcpus = atoi_positive(optarg); + TEST_ASSERT(nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); break; case 's': diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c index ec0f070a6f21..210e98a49a83 100644 --- a/tools/testing/selftests/kvm/lib/test_util.c +++ b/tools/testing/selftests/kvm/lib/test_util.c @@ -353,3 +353,19 @@ int atoi_paranoid(const char *num_str) return num; } + +uint32_t atoi_positive(const char *num_str) +{ + int num = atoi_paranoid(num_str); + + TEST_ASSERT(num > 0, "%s is not a positive integer.\n", num_str); + return num; +} + +uint32_t atoi_non_negative(const char *num_str) +{ + int num = atoi_paranoid(num_str); + + TEST_ASSERT(num >= 0, "%s is not a non-negative integer.\n", num_str); + return num; +} diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c index 1595b73dc09a..20015de3b91c 100644 --- a/tools/testing/selftests/kvm/max_guest_memory_test.c +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c @@ -193,15 +193,14 @@ int main(int argc, char *argv[]) while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) { switch (opt) { case 'c': - nr_vcpus = atoi_paranoid(optarg); - TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0"); + nr_vcpus = atoi_positive(optarg); break; case 'm': - max_mem = atoi_paranoid(optarg) * size_1gb; + max_mem = atoi_positive(optarg) * size_1gb; TEST_ASSERT(max_mem > 0, "memory size must be >0"); break; case 's': - slot_size = atoi_paranoid(optarg) * size_1gb; + slot_size = atoi_positive(optarg) * size_1gb; TEST_ASSERT(slot_size > 0, "slot size must be >0"); break; case 'H': diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c index 865276993ffb..7539ee7b6e95 100644 --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c @@ -166,8 +166,8 @@ int main(int argc, char *argv[]) guest_percpu_mem_size = parse_size(optarg); break; case 'v': - nr_vcpus = atoi_paranoid(optarg); - TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus, + nr_vcpus = atoi_positive(optarg); + TEST_ASSERT(nr_vcpus <= max_vcpus, "Invalid number of vcpus, must be between 1 and %d", max_vcpus); break; @@ -175,7 +175,7 @@ int main(int argc, char *argv[]) p.partition_vcpu_memory_access = false; break; case 'i': - p.nr_memslot_modifications = atoi_paranoid(optarg); + p.nr_memslot_modifications = atoi_positive(optarg); break; case 'h': default: diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c index 4bae9e3f5ca1..8e6e2d44d002 100644 --- a/tools/testing/selftests/kvm/memslot_perf_test.c +++ b/tools/testing/selftests/kvm/memslot_perf_test.c @@ -892,33 +892,21 @@ static bool parse_args(int argc, char *argv[], } break; case 'f': - targs->tfirst = atoi_paranoid(optarg); - if (targs->tfirst < 0) { - pr_info("First test to run has to be non-negative\n"); - return false; - } + targs->tfirst = atoi_non_negative(optarg); break; case 'e': - targs->tlast = atoi_paranoid(optarg); - if (targs->tlast < 0 || targs->tlast >= NTESTS) { + targs->tlast = atoi_non_negative(optarg); + if (targs->tlast >= NTESTS) { pr_info("Last test to run has to be non-negative and less than %zu\n", NTESTS); return false; } break; case 'l': - targs->seconds = atoi_paranoid(optarg); - if (targs->seconds < 0) { - pr_info("Test length in seconds has to be non-negative\n"); - return false; - } + targs->seconds = atoi_non_negative(optarg); break; case 'r': - targs->runs = atoi_paranoid(optarg); - if (targs->runs <= 0) { - pr_info("Runs per test has to be positive\n"); - return false; - } + targs->runs = atoi_positive(optarg); break; } } diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c index c366949c8362..e09092110121 100644 --- a/tools/testing/selftests/kvm/set_memory_region_test.c +++ b/tools/testing/selftests/kvm/set_memory_region_test.c @@ -407,7 +407,7 @@ int main(int argc, char *argv[]) #ifdef __x86_64__ if (argc > 1) - loops = atoi_paranoid(argv[1]); + loops = atoi_positive(argv[1]); else loops = 10; -- 2.38.0.135.g90850a2211-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-21 21:18 [PATCH v6 0/5] dirty_log_perf_test vCPU pinning Vipin Sharma ` (3 preceding siblings ...) 2022-10-21 21:18 ` [PATCH v6 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation Vipin Sharma @ 2022-10-21 21:18 ` Vipin Sharma 2022-10-26 2:27 ` Wang, Wei W 4 siblings, 1 reply; 20+ messages in thread From: Vipin Sharma @ 2022-10-21 21:18 UTC (permalink / raw) To: seanjc, pbonzini, dmatlack Cc: andrew.jones, wei.w.wang, kvm, linux-kernel, Vipin Sharma Add a command line option, -c, to pin vCPUs to physical CPUs (pCPUs), i.e. to force vCPUs to run on specific pCPUs. Requirement to implement this feature came in discussion on the patch "Make page tables for eager page splitting NUMA aware" https://lore.kernel.org/lkml/YuhPT2drgqL+osLl@google.com/ This feature is useful as it provides a way to analyze performance based on the vCPUs and dirty log worker locations, like on the different NUMA nodes or on the same NUMA nodes. To keep things simple, implementation is intentionally very limited, either all of the vCPUs will be pinned followed by an optional main thread or nothing will be pinned. Signed-off-by: Vipin Sharma <vipinsh@google.com> Suggested-by: David Matlack <dmatlack@google.com> --- .../selftests/kvm/dirty_log_perf_test.c | 22 ++++++- .../selftests/kvm/include/perf_test_util.h | 6 ++ .../selftests/kvm/lib/perf_test_util.c | 65 ++++++++++++++++++- 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index 618598ddd993..35504b36b126 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -353,7 +353,7 @@ static void help(char *name) puts(""); printf("usage: %s [-h] [-i iterations] [-p offset] [-g] " "[-m mode] [-n] [-b vcpu bytes] [-v vcpus] [-o] [-s mem type]" - "[-x memslots]\n", name); + "[-x memslots] [-c physical cpus to run test on]\n", name); puts(""); printf(" -i: specify iteration counts (default: %"PRIu64")\n", TEST_HOST_LOOP_N); @@ -383,6 +383,17 @@ static void help(char *name) backing_src_help("-s"); printf(" -x: Split the memory region into this number of memslots.\n" " (default: 1)\n"); + printf(" -c: Pin tasks to physical CPUs. Takes a list of comma separated\n" + " values (target pCPU), one for each vCPU, plus an optional\n" + " entry for the main application task (specified via entry\n" + " <nr_vcpus + 1>). If used, entries must be provided for all\n" + " vCPUs, i.e. pinning vCPUs is all or nothing.\n\n" + " E.g. to create 3 vCPUs, pin vCPU0=>pCPU22, vCPU1=>pCPU23,\n" + " vCPU2=>pCPU24, and pin the application task to pCPU50:\n\n" + " ./dirty_log_perf_test -v 3 -c 22,23,24,50\n\n" + " To leave the application task unpinned, drop the final entry:\n\n" + " ./dirty_log_perf_test -v 3 -c 22,23,24\n\n" + " (default: no pinning)\n"); puts(""); exit(0); } @@ -390,6 +401,7 @@ static void help(char *name) int main(int argc, char *argv[]) { int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); + const char *pcpu_list = NULL; struct test_params p = { .iterations = TEST_HOST_LOOP_N, .wr_fract = 1, @@ -406,11 +418,14 @@ int main(int argc, char *argv[]) guest_modes_append_default(); - while ((opt = getopt(argc, argv, "b:ef:ghi:m:nop:s:v:x:")) != -1) { + while ((opt = getopt(argc, argv, "b:c:ef:ghi:m:nop:s:v:x:")) != -1) { switch (opt) { case 'b': guest_percpu_mem_size = parse_size(optarg); break; + case 'c': + pcpu_list = optarg; + break; case 'e': /* 'e' is for evil. */ run_vcpus_while_disabling_dirty_logging = true; @@ -456,6 +471,9 @@ int main(int argc, char *argv[]) } } + if (pcpu_list) + perf_test_setup_pinning(pcpu_list, nr_vcpus); + TEST_ASSERT(p.iterations >= 2, "The test should have at least two iterations"); pr_info("Test iterations: %"PRIu64"\n", p.iterations); diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h index eaa88df0555a..ccfe3b9dc6bd 100644 --- a/tools/testing/selftests/kvm/include/perf_test_util.h +++ b/tools/testing/selftests/kvm/include/perf_test_util.h @@ -27,6 +27,8 @@ struct perf_test_vcpu_args { /* Only used by the host userspace part of the vCPU thread */ struct kvm_vcpu *vcpu; int vcpu_idx; + /* The pCPU to which this vCPU is pinned. Only valid if pin_vcpus is true. */ + uint32_t pcpu; }; struct perf_test_args { @@ -39,6 +41,8 @@ struct perf_test_args { /* Run vCPUs in L2 instead of L1, if the architecture supports it. */ bool nested; + /* True if all vCPUs are pinned to pCPUs */ + bool pin_vcpus; struct perf_test_vcpu_args vcpu_args[KVM_MAX_VCPUS]; }; @@ -60,4 +64,6 @@ void perf_test_guest_code(uint32_t vcpu_id); uint64_t perf_test_nested_pages(int nr_vcpus); void perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_vcpu *vcpus[]); +void perf_test_setup_pinning(const char *pcpus_string, int nr_vcpus); + #endif /* SELFTEST_KVM_PERF_TEST_UTIL_H */ diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c index 9618b37c66f7..520d1f896d61 100644 --- a/tools/testing/selftests/kvm/lib/perf_test_util.c +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c @@ -2,7 +2,10 @@ /* * Copyright (C) 2020, Google LLC. */ +#define _GNU_SOURCE + #include <inttypes.h> +#include <sched.h> #include "kvm_util.h" #include "perf_test_util.h" @@ -240,10 +243,27 @@ void __weak perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_v exit(KSFT_SKIP); } +static void pin_this_task_to_pcpu(uint32_t pcpu) +{ + cpu_set_t mask; + int r; + + CPU_ZERO(&mask); + CPU_SET(pcpu, &mask); + r = sched_setaffinity(0, sizeof(mask), &mask); + TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); +} + static void *vcpu_thread_main(void *data) { + struct perf_test_vcpu_args *vcpu_args; struct vcpu_thread *vcpu = data; + vcpu_args = &perf_test_args.vcpu_args[vcpu->vcpu_idx]; + + if (perf_test_args.pin_vcpus) + pin_this_task_to_pcpu(vcpu_args->pcpu); + WRITE_ONCE(vcpu->running, true); /* @@ -255,7 +275,7 @@ static void *vcpu_thread_main(void *data) while (!READ_ONCE(all_vcpu_threads_running)) ; - vcpu_thread_fn(&perf_test_args.vcpu_args[vcpu->vcpu_idx]); + vcpu_thread_fn(vcpu_args); return NULL; } @@ -292,3 +312,46 @@ void perf_test_join_vcpu_threads(int nr_vcpus) for (i = 0; i < nr_vcpus; i++) pthread_join(vcpu_threads[i].thread, NULL); } + +static uint32_t parse_pcpu(const char *cpu_str, const cpu_set_t *allowed_mask) +{ + uint32_t pcpu = atoi_non_negative(cpu_str); + + TEST_ASSERT(CPU_ISSET(pcpu, allowed_mask), + "Not allowed to run on pCPU '%d', check cgroups?\n", pcpu); + return pcpu; +} + +void perf_test_setup_pinning(const char *pcpus_string, int nr_vcpus) +{ + cpu_set_t allowed_mask; + char *cpu, *cpu_list; + char delim[2] = ","; + int i, r; + + cpu_list = strdup(pcpus_string); + TEST_ASSERT(cpu_list, "strdup() allocation failed.\n"); + + r = sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask); + TEST_ASSERT(!r, "sched_getaffinity() failed"); + + cpu = strtok(cpu_list, delim); + + /* 1. Get all pcpus for vcpus. */ + for (i = 0; i < nr_vcpus; i++) { + TEST_ASSERT(cpu, "pCPU not provided for vCPU '%d'\n", i); + perf_test_args.vcpu_args[i].pcpu = parse_pcpu(cpu, &allowed_mask); + cpu = strtok(NULL, delim); + } + + perf_test_args.pin_vcpus = true; + + /* 2. Check if the main worker needs to be pinned. */ + if (cpu) { + pin_this_task_to_pcpu(parse_pcpu(cpu, &allowed_mask)); + cpu = strtok(NULL, delim); + } + + TEST_ASSERT(!cpu, "pCPU list contains trailing garbage characters '%s'", cpu); + free(cpu_list); +} -- 2.38.0.135.g90850a2211-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* RE: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-21 21:18 ` [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs Vipin Sharma @ 2022-10-26 2:27 ` Wang, Wei W 2022-10-26 15:44 ` Sean Christopherson 0 siblings, 1 reply; 20+ messages in thread From: Wang, Wei W @ 2022-10-26 2:27 UTC (permalink / raw) To: Vipin Sharma, Christopherson,, Sean, pbonzini, dmatlack Cc: andrew.jones, kvm, linux-kernel On Saturday, October 22, 2022 5:18 AM, Vipin Sharma wrote: > +static void pin_this_task_to_pcpu(uint32_t pcpu) { > + cpu_set_t mask; > + int r; > + > + CPU_ZERO(&mask); > + CPU_SET(pcpu, &mask); > + r = sched_setaffinity(0, sizeof(mask), &mask); > + TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); > +} > + > static void *vcpu_thread_main(void *data) { > + struct perf_test_vcpu_args *vcpu_args; > struct vcpu_thread *vcpu = data; > > + vcpu_args = &perf_test_args.vcpu_args[vcpu->vcpu_idx]; > + > + if (perf_test_args.pin_vcpus) > + pin_this_task_to_pcpu(vcpu_args->pcpu); > + I think it would be better to do the thread pinning at the time when the thread is created by providing a pthread_attr_t attr, e.g. : pthread_attr_t attr; CPU_SET(vcpu->pcpu, &cpu_set); pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpu_set); pthread_create(thread, attr,...); Also, pinning a vCPU thread to a pCPU is a general operation which other users would need. I think we could make it more general and put it to kvm_util, e.g. adding it to the helper function that I'm trying to create + * Create a vcpu thread with user provided attribute and the name in + * "vcpu-##id" format. + */ +void __vcpu_thread_create(struct kvm_vcpu *vcpu, const pthread_attr_t *attr, + void *(*start_routine)(void *), uint32_t private_data_size) (https://lore.kernel.org/kvm/20221024113445.1022147-1-wei.w.wang@intel.com/T/#m0ceed820278a9deb199871ee6da7d6ec54d065f4) ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-26 2:27 ` Wang, Wei W @ 2022-10-26 15:44 ` Sean Christopherson 2022-10-26 18:17 ` Vipin Sharma 2022-10-27 12:03 ` Wang, Wei W 0 siblings, 2 replies; 20+ messages in thread From: Sean Christopherson @ 2022-10-26 15:44 UTC (permalink / raw) To: Wang, Wei W Cc: Vipin Sharma, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Wed, Oct 26, 2022, Wang, Wei W wrote: > On Saturday, October 22, 2022 5:18 AM, Vipin Sharma wrote: > > +static void pin_this_task_to_pcpu(uint32_t pcpu) { > > + cpu_set_t mask; > > + int r; > > + > > + CPU_ZERO(&mask); > > + CPU_SET(pcpu, &mask); > > + r = sched_setaffinity(0, sizeof(mask), &mask); > > + TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); > > +} > > + > > static void *vcpu_thread_main(void *data) { > > + struct perf_test_vcpu_args *vcpu_args; > > struct vcpu_thread *vcpu = data; > > > > + vcpu_args = &perf_test_args.vcpu_args[vcpu->vcpu_idx]; > > + > > + if (perf_test_args.pin_vcpus) > > + pin_this_task_to_pcpu(vcpu_args->pcpu); > > + > > I think it would be better to do the thread pinning at the time when the > thread is created by providing a pthread_attr_t attr, e.g. : > > pthread_attr_t attr; > > CPU_SET(vcpu->pcpu, &cpu_set); > pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpu_set); > pthread_create(thread, attr,...); > > Also, pinning a vCPU thread to a pCPU is a general operation > which other users would need. I think we could make it more general and > put it to kvm_util. We could, but it taking advantage of the pinning functionality would require plumbing a command line option for every test, or alternatively adding partial command line parsing with a "hidden" global struct to kvm_selftest_init(), though handling error checking for a truly generic case would be a mess. Either way, extending pinning to other tests would require non-trivial effort, and can be done on top of this series. That said, it's also trival to extract the pinning helpers to common code, and I can't think of any reason not to do that straightaway. Vipin, any objection to squashing the below diff with patch 5? > e.g. adding it to the helper function that I'm trying to create If we go this route in the future, we'd need to add a worker trampoline as the pinning needs to happen in the worker task itself to guarantee that the pinning takes effect before the worker does anything useful. That should be very doable. I do like the idea of extending __vcpu_thread_create(), but we can do that once __vcpu_thread_create() lands to avoid further delaying this series. --- .../selftests/kvm/dirty_log_perf_test.c | 7 ++- .../selftests/kvm/include/kvm_util_base.h | 4 ++ .../selftests/kvm/include/perf_test_util.h | 8 +-- tools/testing/selftests/kvm/lib/kvm_util.c | 54 ++++++++++++++++++ .../selftests/kvm/lib/perf_test_util.c | 57 +------------------ 5 files changed, 68 insertions(+), 62 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index 35504b36b126..a82fc51d57ca 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -471,8 +471,11 @@ int main(int argc, char *argv[]) } } - if (pcpu_list) - perf_test_setup_pinning(pcpu_list, nr_vcpus); + if (pcpu_list) { + kvm_parse_vcpu_pinning(pcpu_list, perf_test_args.vcpu_to_pcpu, + nr_vcpus); + perf_test_args.pin_vcpus = true; + } TEST_ASSERT(p.iterations >= 2, "The test should have at least two iterations"); diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index e42a09cd24a0..3bf2333ef95d 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -688,6 +688,10 @@ static inline struct kvm_vm *vm_create_with_one_vcpu(struct kvm_vcpu **vcpu, struct kvm_vcpu *vm_recreate_with_one_vcpu(struct kvm_vm *vm); +void kvm_pin_this_task_to_pcpu(uint32_t pcpu); +void kvm_parse_vcpu_pinning(const char *pcpus_string, uint32_t vcpu_to_pcpu[], + int nr_vcpus); + unsigned long vm_compute_max_gfn(struct kvm_vm *vm); unsigned int vm_calc_num_guest_pages(enum vm_guest_mode mode, size_t size); unsigned int vm_num_host_pages(enum vm_guest_mode mode, unsigned int num_guest_pages); diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h index ccfe3b9dc6bd..85320e0640fc 100644 --- a/tools/testing/selftests/kvm/include/perf_test_util.h +++ b/tools/testing/selftests/kvm/include/perf_test_util.h @@ -27,8 +27,6 @@ struct perf_test_vcpu_args { /* Only used by the host userspace part of the vCPU thread */ struct kvm_vcpu *vcpu; int vcpu_idx; - /* The pCPU to which this vCPU is pinned. Only valid if pin_vcpus is true. */ - uint32_t pcpu; }; struct perf_test_args { @@ -43,8 +41,12 @@ struct perf_test_args { bool nested; /* True if all vCPUs are pinned to pCPUs */ bool pin_vcpus; + /* The vCPU=>pCPU pinning map. Only valid if pin_vcpus is true. */ + uint32_t vcpu_to_pcpu[KVM_MAX_VCPUS]; struct perf_test_vcpu_args vcpu_args[KVM_MAX_VCPUS]; + + }; extern struct perf_test_args perf_test_args; @@ -64,6 +66,4 @@ void perf_test_guest_code(uint32_t vcpu_id); uint64_t perf_test_nested_pages(int nr_vcpus); void perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_vcpu *vcpus[]); -void perf_test_setup_pinning(const char *pcpus_string, int nr_vcpus); - #endif /* SELFTEST_KVM_PERF_TEST_UTIL_H */ diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index f1cb1627161f..8292af9d7660 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -11,6 +11,7 @@ #include "processor.h" #include <assert.h> +#include <sched.h> #include <sys/mman.h> #include <sys/types.h> #include <sys/stat.h> @@ -443,6 +444,59 @@ struct kvm_vcpu *vm_recreate_with_one_vcpu(struct kvm_vm *vm) return vm_vcpu_recreate(vm, 0); } +void kvm_pin_this_task_to_pcpu(uint32_t pcpu) +{ + cpu_set_t mask; + int r; + + CPU_ZERO(&mask); + CPU_SET(pcpu, &mask); + r = sched_setaffinity(0, sizeof(mask), &mask); + TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); +} + +static uint32_t parse_pcpu(const char *cpu_str, const cpu_set_t *allowed_mask) +{ + uint32_t pcpu = atoi_non_negative(cpu_str); + + TEST_ASSERT(CPU_ISSET(pcpu, allowed_mask), + "Not allowed to run on pCPU '%d', check cgroups?\n", pcpu); + return pcpu; +} + +void kvm_parse_vcpu_pinning(const char *pcpus_string, uint32_t vcpu_to_pcpu[], + int nr_vcpus) +{ + cpu_set_t allowed_mask; + char *cpu, *cpu_list; + char delim[2] = ","; + int i, r; + + cpu_list = strdup(pcpus_string); + TEST_ASSERT(cpu_list, "strdup() allocation failed.\n"); + + r = sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask); + TEST_ASSERT(!r, "sched_getaffinity() failed"); + + cpu = strtok(cpu_list, delim); + + /* 1. Get all pcpus for vcpus. */ + for (i = 0; i < nr_vcpus; i++) { + TEST_ASSERT(cpu, "pCPU not provided for vCPU '%d'\n", i); + vcpu_to_pcpu[i] = parse_pcpu(cpu, &allowed_mask); + cpu = strtok(NULL, delim); + } + + /* 2. Check if the main worker needs to be pinned. */ + if (cpu) { + kvm_pin_this_task_to_pcpu(parse_pcpu(cpu, &allowed_mask)); + cpu = strtok(NULL, delim); + } + + TEST_ASSERT(!cpu, "pCPU list contains trailing garbage characters '%s'", cpu); + free(cpu_list); +} + /* * Userspace Memory Region Find * diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c index 520d1f896d61..1d133007d7de 100644 --- a/tools/testing/selftests/kvm/lib/perf_test_util.c +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c @@ -5,7 +5,6 @@ #define _GNU_SOURCE #include <inttypes.h> -#include <sched.h> #include "kvm_util.h" #include "perf_test_util.h" @@ -243,17 +242,6 @@ void __weak perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_v exit(KSFT_SKIP); } -static void pin_this_task_to_pcpu(uint32_t pcpu) -{ - cpu_set_t mask; - int r; - - CPU_ZERO(&mask); - CPU_SET(pcpu, &mask); - r = sched_setaffinity(0, sizeof(mask), &mask); - TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); -} - static void *vcpu_thread_main(void *data) { struct perf_test_vcpu_args *vcpu_args; @@ -262,7 +250,7 @@ static void *vcpu_thread_main(void *data) vcpu_args = &perf_test_args.vcpu_args[vcpu->vcpu_idx]; if (perf_test_args.pin_vcpus) - pin_this_task_to_pcpu(vcpu_args->pcpu); + kvm_pin_this_task_to_pcpu(perf_test_args.vcpu_to_pcpu[vcpu->vcpu_idx]); WRITE_ONCE(vcpu->running, true); @@ -312,46 +300,3 @@ void perf_test_join_vcpu_threads(int nr_vcpus) for (i = 0; i < nr_vcpus; i++) pthread_join(vcpu_threads[i].thread, NULL); } - -static uint32_t parse_pcpu(const char *cpu_str, const cpu_set_t *allowed_mask) -{ - uint32_t pcpu = atoi_non_negative(cpu_str); - - TEST_ASSERT(CPU_ISSET(pcpu, allowed_mask), - "Not allowed to run on pCPU '%d', check cgroups?\n", pcpu); - return pcpu; -} - -void perf_test_setup_pinning(const char *pcpus_string, int nr_vcpus) -{ - cpu_set_t allowed_mask; - char *cpu, *cpu_list; - char delim[2] = ","; - int i, r; - - cpu_list = strdup(pcpus_string); - TEST_ASSERT(cpu_list, "strdup() allocation failed.\n"); - - r = sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask); - TEST_ASSERT(!r, "sched_getaffinity() failed"); - - cpu = strtok(cpu_list, delim); - - /* 1. Get all pcpus for vcpus. */ - for (i = 0; i < nr_vcpus; i++) { - TEST_ASSERT(cpu, "pCPU not provided for vCPU '%d'\n", i); - perf_test_args.vcpu_args[i].pcpu = parse_pcpu(cpu, &allowed_mask); - cpu = strtok(NULL, delim); - } - - perf_test_args.pin_vcpus = true; - - /* 2. Check if the main worker needs to be pinned. */ - if (cpu) { - pin_this_task_to_pcpu(parse_pcpu(cpu, &allowed_mask)); - cpu = strtok(NULL, delim); - } - - TEST_ASSERT(!cpu, "pCPU list contains trailing garbage characters '%s'", cpu); - free(cpu_list); -} base-commit: 076ac4ca97225d6b8698a9b066153b556e97be7c -- ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-26 15:44 ` Sean Christopherson @ 2022-10-26 18:17 ` Vipin Sharma 2022-10-27 12:03 ` Wang, Wei W 1 sibling, 0 replies; 20+ messages in thread From: Vipin Sharma @ 2022-10-26 18:17 UTC (permalink / raw) To: Sean Christopherson Cc: Wang, Wei W, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Wed, Oct 26, 2022 at 8:44 AM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Oct 26, 2022, Wang, Wei W wrote: > > On Saturday, October 22, 2022 5:18 AM, Vipin Sharma wrote: > > > +static void pin_this_task_to_pcpu(uint32_t pcpu) { > > > + cpu_set_t mask; > > > + int r; > > > + > > > + CPU_ZERO(&mask); > > > + CPU_SET(pcpu, &mask); > > > + r = sched_setaffinity(0, sizeof(mask), &mask); > > > + TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); > > > +} > > > + > > > static void *vcpu_thread_main(void *data) { > > > + struct perf_test_vcpu_args *vcpu_args; > > > struct vcpu_thread *vcpu = data; > > > > > > + vcpu_args = &perf_test_args.vcpu_args[vcpu->vcpu_idx]; > > > + > > > + if (perf_test_args.pin_vcpus) > > > + pin_this_task_to_pcpu(vcpu_args->pcpu); > > > + > > > > I think it would be better to do the thread pinning at the time when the > > thread is created by providing a pthread_attr_t attr, e.g. : > > > > pthread_attr_t attr; > > > > CPU_SET(vcpu->pcpu, &cpu_set); > > pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpu_set); > > pthread_create(thread, attr,...); > > > > Also, pinning a vCPU thread to a pCPU is a general operation > > which other users would need. I think we could make it more general and > > put it to kvm_util. > > We could, but it taking advantage of the pinning functionality would require > plumbing a command line option for every test, or alternatively adding partial > command line parsing with a "hidden" global struct to kvm_selftest_init(), though > handling error checking for a truly generic case would be a mess. Either way, > extending pinning to other tests would require non-trivial effort, and can be > done on top of this series. > > That said, it's also trival to extract the pinning helpers to common code, and I > can't think of any reason not to do that straightaway. > > Vipin, any objection to squashing the below diff with patch 5? > Looks fine to me, I will send v7 with this change. > > e.g. adding it to the helper function that I'm trying to create > > If we go this route in the future, we'd need to add a worker trampoline as the > pinning needs to happen in the worker task itself to guarantee that the pinning > takes effect before the worker does anything useful. That should be very doable. > > I do like the idea of extending __vcpu_thread_create(), but we can do that once > __vcpu_thread_create() lands to avoid further delaying this series. > > --- > .../selftests/kvm/dirty_log_perf_test.c | 7 ++- > .../selftests/kvm/include/kvm_util_base.h | 4 ++ > .../selftests/kvm/include/perf_test_util.h | 8 +-- > tools/testing/selftests/kvm/lib/kvm_util.c | 54 ++++++++++++++++++ > .../selftests/kvm/lib/perf_test_util.c | 57 +------------------ > 5 files changed, 68 insertions(+), 62 deletions(-) > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c > index 35504b36b126..a82fc51d57ca 100644 > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > @@ -471,8 +471,11 @@ int main(int argc, char *argv[]) > } > } > > - if (pcpu_list) > - perf_test_setup_pinning(pcpu_list, nr_vcpus); > + if (pcpu_list) { > + kvm_parse_vcpu_pinning(pcpu_list, perf_test_args.vcpu_to_pcpu, > + nr_vcpus); > + perf_test_args.pin_vcpus = true; > + } > > TEST_ASSERT(p.iterations >= 2, "The test should have at least two iterations"); > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index e42a09cd24a0..3bf2333ef95d 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -688,6 +688,10 @@ static inline struct kvm_vm *vm_create_with_one_vcpu(struct kvm_vcpu **vcpu, > > struct kvm_vcpu *vm_recreate_with_one_vcpu(struct kvm_vm *vm); > > +void kvm_pin_this_task_to_pcpu(uint32_t pcpu); > +void kvm_parse_vcpu_pinning(const char *pcpus_string, uint32_t vcpu_to_pcpu[], > + int nr_vcpus); > + > unsigned long vm_compute_max_gfn(struct kvm_vm *vm); > unsigned int vm_calc_num_guest_pages(enum vm_guest_mode mode, size_t size); > unsigned int vm_num_host_pages(enum vm_guest_mode mode, unsigned int num_guest_pages); > diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h > index ccfe3b9dc6bd..85320e0640fc 100644 > --- a/tools/testing/selftests/kvm/include/perf_test_util.h > +++ b/tools/testing/selftests/kvm/include/perf_test_util.h > @@ -27,8 +27,6 @@ struct perf_test_vcpu_args { > /* Only used by the host userspace part of the vCPU thread */ > struct kvm_vcpu *vcpu; > int vcpu_idx; > - /* The pCPU to which this vCPU is pinned. Only valid if pin_vcpus is true. */ > - uint32_t pcpu; > }; > > struct perf_test_args { > @@ -43,8 +41,12 @@ struct perf_test_args { > bool nested; > /* True if all vCPUs are pinned to pCPUs */ > bool pin_vcpus; > + /* The vCPU=>pCPU pinning map. Only valid if pin_vcpus is true. */ > + uint32_t vcpu_to_pcpu[KVM_MAX_VCPUS]; > > struct perf_test_vcpu_args vcpu_args[KVM_MAX_VCPUS]; > + > + > }; > > extern struct perf_test_args perf_test_args; > @@ -64,6 +66,4 @@ void perf_test_guest_code(uint32_t vcpu_id); > uint64_t perf_test_nested_pages(int nr_vcpus); > void perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_vcpu *vcpus[]); > > -void perf_test_setup_pinning(const char *pcpus_string, int nr_vcpus); > - > #endif /* SELFTEST_KVM_PERF_TEST_UTIL_H */ > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index f1cb1627161f..8292af9d7660 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -11,6 +11,7 @@ > #include "processor.h" > > #include <assert.h> > +#include <sched.h> > #include <sys/mman.h> > #include <sys/types.h> > #include <sys/stat.h> > @@ -443,6 +444,59 @@ struct kvm_vcpu *vm_recreate_with_one_vcpu(struct kvm_vm *vm) > return vm_vcpu_recreate(vm, 0); > } > > +void kvm_pin_this_task_to_pcpu(uint32_t pcpu) > +{ > + cpu_set_t mask; > + int r; > + > + CPU_ZERO(&mask); > + CPU_SET(pcpu, &mask); > + r = sched_setaffinity(0, sizeof(mask), &mask); > + TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); > +} > + > +static uint32_t parse_pcpu(const char *cpu_str, const cpu_set_t *allowed_mask) > +{ > + uint32_t pcpu = atoi_non_negative(cpu_str); > + > + TEST_ASSERT(CPU_ISSET(pcpu, allowed_mask), > + "Not allowed to run on pCPU '%d', check cgroups?\n", pcpu); > + return pcpu; > +} > + > +void kvm_parse_vcpu_pinning(const char *pcpus_string, uint32_t vcpu_to_pcpu[], > + int nr_vcpus) > +{ > + cpu_set_t allowed_mask; > + char *cpu, *cpu_list; > + char delim[2] = ","; > + int i, r; > + > + cpu_list = strdup(pcpus_string); > + TEST_ASSERT(cpu_list, "strdup() allocation failed.\n"); > + > + r = sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask); > + TEST_ASSERT(!r, "sched_getaffinity() failed"); > + > + cpu = strtok(cpu_list, delim); > + > + /* 1. Get all pcpus for vcpus. */ > + for (i = 0; i < nr_vcpus; i++) { > + TEST_ASSERT(cpu, "pCPU not provided for vCPU '%d'\n", i); > + vcpu_to_pcpu[i] = parse_pcpu(cpu, &allowed_mask); > + cpu = strtok(NULL, delim); > + } > + > + /* 2. Check if the main worker needs to be pinned. */ > + if (cpu) { > + kvm_pin_this_task_to_pcpu(parse_pcpu(cpu, &allowed_mask)); > + cpu = strtok(NULL, delim); > + } > + > + TEST_ASSERT(!cpu, "pCPU list contains trailing garbage characters '%s'", cpu); > + free(cpu_list); > +} > + > /* > * Userspace Memory Region Find > * > diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c > index 520d1f896d61..1d133007d7de 100644 > --- a/tools/testing/selftests/kvm/lib/perf_test_util.c > +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c > @@ -5,7 +5,6 @@ > #define _GNU_SOURCE > > #include <inttypes.h> > -#include <sched.h> > > #include "kvm_util.h" > #include "perf_test_util.h" > @@ -243,17 +242,6 @@ void __weak perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_v > exit(KSFT_SKIP); > } > > -static void pin_this_task_to_pcpu(uint32_t pcpu) > -{ > - cpu_set_t mask; > - int r; > - > - CPU_ZERO(&mask); > - CPU_SET(pcpu, &mask); > - r = sched_setaffinity(0, sizeof(mask), &mask); > - TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); > -} > - > static void *vcpu_thread_main(void *data) > { > struct perf_test_vcpu_args *vcpu_args; > @@ -262,7 +250,7 @@ static void *vcpu_thread_main(void *data) > vcpu_args = &perf_test_args.vcpu_args[vcpu->vcpu_idx]; > > if (perf_test_args.pin_vcpus) > - pin_this_task_to_pcpu(vcpu_args->pcpu); > + kvm_pin_this_task_to_pcpu(perf_test_args.vcpu_to_pcpu[vcpu->vcpu_idx]); > > WRITE_ONCE(vcpu->running, true); > > @@ -312,46 +300,3 @@ void perf_test_join_vcpu_threads(int nr_vcpus) > for (i = 0; i < nr_vcpus; i++) > pthread_join(vcpu_threads[i].thread, NULL); > } > - > -static uint32_t parse_pcpu(const char *cpu_str, const cpu_set_t *allowed_mask) > -{ > - uint32_t pcpu = atoi_non_negative(cpu_str); > - > - TEST_ASSERT(CPU_ISSET(pcpu, allowed_mask), > - "Not allowed to run on pCPU '%d', check cgroups?\n", pcpu); > - return pcpu; > -} > - > -void perf_test_setup_pinning(const char *pcpus_string, int nr_vcpus) > -{ > - cpu_set_t allowed_mask; > - char *cpu, *cpu_list; > - char delim[2] = ","; > - int i, r; > - > - cpu_list = strdup(pcpus_string); > - TEST_ASSERT(cpu_list, "strdup() allocation failed.\n"); > - > - r = sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask); > - TEST_ASSERT(!r, "sched_getaffinity() failed"); > - > - cpu = strtok(cpu_list, delim); > - > - /* 1. Get all pcpus for vcpus. */ > - for (i = 0; i < nr_vcpus; i++) { > - TEST_ASSERT(cpu, "pCPU not provided for vCPU '%d'\n", i); > - perf_test_args.vcpu_args[i].pcpu = parse_pcpu(cpu, &allowed_mask); > - cpu = strtok(NULL, delim); > - } > - > - perf_test_args.pin_vcpus = true; > - > - /* 2. Check if the main worker needs to be pinned. */ > - if (cpu) { > - pin_this_task_to_pcpu(parse_pcpu(cpu, &allowed_mask)); > - cpu = strtok(NULL, delim); > - } > - > - TEST_ASSERT(!cpu, "pCPU list contains trailing garbage characters '%s'", cpu); > - free(cpu_list); > -} > > base-commit: 076ac4ca97225d6b8698a9b066153b556e97be7c > -- > ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-26 15:44 ` Sean Christopherson 2022-10-26 18:17 ` Vipin Sharma @ 2022-10-27 12:03 ` Wang, Wei W 2022-10-27 15:56 ` Sean Christopherson 1 sibling, 1 reply; 20+ messages in thread From: Wang, Wei W @ 2022-10-27 12:03 UTC (permalink / raw) To: Christopherson,, Sean Cc: Vipin Sharma, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Wednesday, October 26, 2022 11:44 PM, Sean Christopherson wrote: > > I think it would be better to do the thread pinning at the time when > > the thread is created by providing a pthread_attr_t attr, e.g. : > > > > pthread_attr_t attr; > > > > CPU_SET(vcpu->pcpu, &cpu_set); > > pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpu_set); > > pthread_create(thread, attr,...); > > > > Also, pinning a vCPU thread to a pCPU is a general operation which > > other users would need. I think we could make it more general and put > > it to kvm_util. > > We could, but it taking advantage of the pinning functionality would require > plumbing a command line option for every test, I think we could make this "pinning" be optional (no need to force everyone to use it). > or alternatively adding partial > command line parsing with a "hidden" global struct to kvm_selftest_init(), > though handling error checking for a truly generic case would be a mess. Either > way, extending pinning to other tests would require non-trivial effort, and can be > done on top of this series. > > That said, it's also trival to extract the pinning helpers to common code, and I > can't think of any reason not to do that straightaway. > > Vipin, any objection to squashing the below diff with patch 5? > > > e.g. adding it to the helper function that I'm trying to create > > If we go this route in the future, we'd need to add a worker trampoline as the > pinning needs to happen in the worker task itself to guarantee that the pinning > takes effect before the worker does anything useful. That should be very > doable. The alternative way is the one I shared before, using this: /* Thread created with attribute ATTR will be limited to run only on the processors represented in CPUSET. */ extern int pthread_attr_setaffinity_np (pthread_attr_t *__attr, size_t __cpusetsize, const cpu_set_t *__cpuset) Basically, the thread is created on the pCPU as user specified. I think this is better than "creating the thread on an arbitrary pCPU and then pinning it to the user specified pCPU in the thread's start routine". > > I do like the idea of extending __vcpu_thread_create(), but we can do that once > __vcpu_thread_create() lands to avoid further delaying this series. Sounds good. I can move some of those to vcpu_thread_create() once it's ready later. > struct perf_test_args { > @@ -43,8 +41,12 @@ struct perf_test_args { > bool nested; > /* True if all vCPUs are pinned to pCPUs */ > bool pin_vcpus; > + /* The vCPU=>pCPU pinning map. Only valid if pin_vcpus is true. */ > + uint32_t vcpu_to_pcpu[KVM_MAX_VCPUS]; How about putting the pcpu id to "struct kvm_vcpu"? (please see below code posed to shows how that works). This is helpful when we later make this more generic, as kvm_vcpu is used by everyone. Probably we also don't need "bool pin_vcpus". We could initialize pcpu_id to -1 to indicate that the vcpu doesn't need pinning (this is also what I meant above optional for other users). Put the whole changes together (tested and worked fine), FYI: diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c index b30500cc197e..2829c98078d0 100644 --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c @@ -304,7 +304,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) int nr_vcpus = params->nr_vcpus; vm = perf_test_create_vm(mode, nr_vcpus, params->vcpu_memory_bytes, 1, - params->backing_src, !overlap_memory_access); + params->backing_src, !overlap_memory_access, NULL); perf_test_start_vcpu_threads(nr_vcpus, vcpu_thread_main); diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index dcdb6964b1dc..e19c3ce32c62 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -286,7 +286,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) int r, i; vm = perf_test_create_vm(mode, nr_vcpus, guest_percpu_mem_size, 1, - p->src_type, p->partition_vcpu_memory_access); + p->src_type, p->partition_vcpu_memory_access, NULL); demand_paging_size = get_backing_src_pagesz(p->src_type); diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index 35504b36b126..cbe7de28e094 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -132,6 +132,7 @@ struct test_params { bool partition_vcpu_memory_access; enum vm_mem_backing_src_type backing_src; int slots; + char *pcpu_list; }; static void toggle_dirty_logging(struct kvm_vm *vm, int slots, bool enable) @@ -223,7 +224,8 @@ static void run_test(enum vm_guest_mode mode, void *arg) vm = perf_test_create_vm(mode, nr_vcpus, guest_percpu_mem_size, p->slots, p->backing_src, - p->partition_vcpu_memory_access); + p->partition_vcpu_memory_access, + p->pcpu_list); perf_test_set_wr_fract(vm, p->wr_fract); @@ -401,13 +403,13 @@ static void help(char *name) int main(int argc, char *argv[]) { int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); - const char *pcpu_list = NULL; struct test_params p = { .iterations = TEST_HOST_LOOP_N, .wr_fract = 1, .partition_vcpu_memory_access = true, .backing_src = DEFAULT_VM_MEM_SRC, .slots = 1, + .pcpu_list = NULL, }; int opt; @@ -424,7 +426,7 @@ int main(int argc, char *argv[]) guest_percpu_mem_size = parse_size(optarg); break; case 'c': - pcpu_list = optarg; + p.pcpu_list = optarg; break; case 'e': /* 'e' is for evil. */ @@ -471,9 +473,6 @@ int main(int argc, char *argv[]) } } - if (pcpu_list) - perf_test_setup_pinning(pcpu_list, nr_vcpus); - TEST_ASSERT(p.iterations >= 2, "The test should have at least two iterations"); pr_info("Test iterations: %"PRIu64"\n", p.iterations); diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index c9286811a4cb..d403b374bae5 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -7,7 +7,11 @@ #ifndef SELFTEST_KVM_UTIL_H #define SELFTEST_KVM_UTIL_H +#include <pthread.h> + #include "kvm_util_base.h" #include "ucall_common.h" +void kvm_parse_vcpu_pinning(struct kvm_vcpu **vcpu, int nr_vcpus, const char *pcpus_string); + #endif /* SELFTEST_KVM_UTIL_H */ diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index e42a09cd24a0..79867a478a81 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -47,6 +47,7 @@ struct userspace_mem_region { struct kvm_vcpu { struct list_head list; uint32_t id; + int pcpu_id; int fd; struct kvm_vm *vm; struct kvm_run *run; diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h index ccfe3b9dc6bd..81428022bdb5 100644 --- a/tools/testing/selftests/kvm/include/perf_test_util.h +++ b/tools/testing/selftests/kvm/include/perf_test_util.h @@ -52,7 +52,8 @@ extern struct perf_test_args perf_test_args; struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int nr_vcpus, uint64_t vcpu_memory_bytes, int slots, enum vm_mem_backing_src_type backing_src, - bool partition_vcpu_memory_access); + bool partition_vcpu_memory_access, + char *pcpu_list); void perf_test_destroy_vm(struct kvm_vm *vm); void perf_test_set_wr_fract(struct kvm_vm *vm, int wr_fract); diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index f1cb1627161f..8acee6d4ccbe 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1114,6 +1114,7 @@ struct kvm_vcpu *__vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id) vcpu->vm = vm; vcpu->id = vcpu_id; + vcpu->pcpu_id = -1; vcpu->fd = __vm_ioctl(vm, KVM_CREATE_VCPU, (void *)(unsigned long)vcpu_id); TEST_ASSERT(vcpu->fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VCPU, vcpu->fd)); @@ -2021,3 +2022,58 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data, break; } } + +void kvm_pin_this_task_to_pcpu(uint32_t pcpu) +{ + cpu_set_t mask; + int r; + + CPU_ZERO(&mask); + CPU_SET(pcpu, &mask); + r = sched_setaffinity(0, sizeof(mask), &mask); + TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu); +} + +static uint32_t parse_pcpu(const char *cpu_str, const cpu_set_t *allowed_mask) +{ + uint32_t pcpu = atoi_non_negative(cpu_str); + + TEST_ASSERT(CPU_ISSET(pcpu, allowed_mask), + "Not allowed to run on pCPU '%d', check cgroups?\n", pcpu); + return pcpu; +} + +void kvm_parse_vcpu_pinning(struct kvm_vcpu **vcpu, int nr_vcpus, const char *pcpus_string) +{ + cpu_set_t allowed_mask; + char *cpu, *cpu_list; + char delim[2] = ","; + int i, r; + + if (!pcpus_string) + return; + + cpu_list = strdup(pcpus_string); + TEST_ASSERT(cpu_list, "strdup() allocation failed.\n"); + + r = sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask); + TEST_ASSERT(!r, "sched_getaffinity() failed"); + + cpu = strtok(cpu_list, delim); + + /* 1. Get all pcpus for vcpus. */ + for (i = 0; i < nr_vcpus; i++) { + TEST_ASSERT(cpu, "pCPU not provided for vCPU '%d'\n", i); + vcpu[i]->pcpu_id = parse_pcpu(cpu, &allowed_mask); + cpu = strtok(NULL, delim); + } + + /* 2. Check if the main worker needs to be pinned. */ + if (cpu) { + kvm_pin_this_task_to_pcpu(parse_pcpu(cpu, &allowed_mask)); + cpu = strtok(NULL, delim); + } + + TEST_ASSERT(!cpu, "pCPU list contains trailing garbage characters '%s'", cpu); + free(cpu_list); +} diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c index 520d1f896d61..95166c5a77f7 100644 --- a/tools/testing/selftests/kvm/lib/perf_test_util.c +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c @@ -112,7 +112,8 @@ void perf_test_setup_vcpus(struct kvm_vm *vm, int nr_vcpus, struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int nr_vcpus, uint64_t vcpu_memory_bytes, int slots, enum vm_mem_backing_src_type backing_src, - bool partition_vcpu_memory_access) + bool partition_vcpu_memory_access, + char *pcpu_list) { struct perf_test_args *pta = &perf_test_args; struct kvm_vm *vm; @@ -157,7 +158,7 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int nr_vcpus, */ vm = __vm_create_with_vcpus(mode, nr_vcpus, slot0_pages + guest_num_pages, perf_test_guest_code, vcpus); - + kvm_parse_vcpu_pinning(vcpus, nr_vcpus, pcpu_list); pta->vm = vm; /* Put the test region at the top guest physical memory. */ @@ -284,17 +285,29 @@ void perf_test_start_vcpu_threads(int nr_vcpus, void (*vcpu_fn)(struct perf_test_vcpu_args *)) { int i; + pthread_attr_t attr, *attr_p; + cpu_set_t cpuset; vcpu_thread_fn = vcpu_fn; WRITE_ONCE(all_vcpu_threads_running, false); for (i = 0; i < nr_vcpus; i++) { struct vcpu_thread *vcpu = &vcpu_threads[i]; + attr_p = NULL; vcpu->vcpu_idx = i; WRITE_ONCE(vcpu->running, false); - pthread_create(&vcpu->thread, NULL, vcpu_thread_main, vcpu); + if (vcpus[i]->pcpu_id != -1) { + CPU_ZERO(&cpuset); + CPU_SET(vcpus[i]->pcpu_id, &cpuset); + pthread_attr_init(&attr); + pthread_attr_setaffinity_np(&attr, + sizeof(cpu_set_t), &cpuset); + attr_p = &attr; + } + + pthread_create(&vcpu->thread, attr_p, vcpu_thread_main, vcpu); } for (i = 0; i < nr_vcpus; i++) { diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c index 9968800ec2ec..5dbe09537b2d 100644 --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c @@ -99,7 +99,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) vm = perf_test_create_vm(mode, nr_vcpus, guest_percpu_mem_size, 1, VM_MEM_SRC_ANONYMOUS, - p->partition_vcpu_memory_access); + p->partition_vcpu_memory_access, NULL); pr_info("Finished creating vCPUs\n"); ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-27 12:03 ` Wang, Wei W @ 2022-10-27 15:56 ` Sean Christopherson 2022-10-27 20:02 ` Vipin Sharma 0 siblings, 1 reply; 20+ messages in thread From: Sean Christopherson @ 2022-10-27 15:56 UTC (permalink / raw) To: Wang, Wei W Cc: Vipin Sharma, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Thu, Oct 27, 2022, Wang, Wei W wrote: > On Wednesday, October 26, 2022 11:44 PM, Sean Christopherson wrote: > > > I think it would be better to do the thread pinning at the time when > > > the thread is created by providing a pthread_attr_t attr, e.g. : > > > > > > pthread_attr_t attr; > > > > > > CPU_SET(vcpu->pcpu, &cpu_set); > > > pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpu_set); > > > pthread_create(thread, attr,...); > > > > > > Also, pinning a vCPU thread to a pCPU is a general operation which > > > other users would need. I think we could make it more general and put > > > it to kvm_util. > > > > We could, but it taking advantage of the pinning functionality would require > > plumbing a command line option for every test, > > I think we could make this "pinning" be optional (no need to force everyone > to use it). Heh, it's definitely optional. > > If we go this route in the future, we'd need to add a worker trampoline as the > > pinning needs to happen in the worker task itself to guarantee that the pinning > > takes effect before the worker does anything useful. That should be very > > doable. > > The alternative way is the one I shared before, using this: > > /* Thread created with attribute ATTR will be limited to run only on > the processors represented in CPUSET. */ > extern int pthread_attr_setaffinity_np (pthread_attr_t *__attr, > size_t __cpusetsize, > const cpu_set_t *__cpuset) > > Basically, the thread is created on the pCPU as user specified. > I think this is better than "creating the thread on an arbitrary pCPU > and then pinning it to the user specified pCPU in the thread's start routine". Ah, yeah, that's better. > > I do like the idea of extending __vcpu_thread_create(), but we can do that once > > __vcpu_thread_create() lands to avoid further delaying this series. > > Sounds good. I can move some of those to vcpu_thread_create() once it's ready later. > > > struct perf_test_args { > > @@ -43,8 +41,12 @@ struct perf_test_args { > > bool nested; > > /* True if all vCPUs are pinned to pCPUs */ > > bool pin_vcpus; > > + /* The vCPU=>pCPU pinning map. Only valid if pin_vcpus is true. */ > > + uint32_t vcpu_to_pcpu[KVM_MAX_VCPUS]; > > How about putting the pcpu id to "struct kvm_vcpu"? (please see below code > posed to shows how that works). This is helpful when we later make this more generic, > as kvm_vcpu is used by everyone. I don't think "pcpu" belongs in kvm_vcpu, even in the long run. The vast, vast majority of tests will never care about pinning, which means that vcpu->pcpu can't be used for anything except the actual pinning. And for pinning, the "pcpu" doesn't need to be persistent information, i.e. doesn't need to live in kvm_vcpu. > Probably we also don't need "bool pin_vcpus". Yeah, but for selftests shaving bytes is not exactly top priority, and having a dedicated flag avoids the need for magic numbers. If Vipin had used -1, I'd probably be fine with that, but I'm also totally fine using a dedicated flag too. > We could initialize pcpu_id to -1 to indicate that the vcpu doesn't need > pinning (this is also what I meant above optional for other users). > > Put the whole changes together (tested and worked fine), FYI: The big downside of this is forcing all callers of perf_test_create_vm() to pass in NULL. I really want to move away from this pattern as it makes what should be simple code rather difficult to read due to having a bunch of "dead" params dangling off the end of function calls. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-27 15:56 ` Sean Christopherson @ 2022-10-27 20:02 ` Vipin Sharma 2022-10-27 20:23 ` Sean Christopherson 2022-10-28 2:11 ` Wang, Wei W 0 siblings, 2 replies; 20+ messages in thread From: Vipin Sharma @ 2022-10-27 20:02 UTC (permalink / raw) To: Sean Christopherson Cc: Wang, Wei W, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Thu, Oct 27, 2022 at 8:56 AM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Oct 27, 2022, Wang, Wei W wrote: > > On Wednesday, October 26, 2022 11:44 PM, Sean Christopherson wrote: > > > If we go this route in the future, we'd need to add a worker trampoline as the > > > pinning needs to happen in the worker task itself to guarantee that the pinning > > > takes effect before the worker does anything useful. That should be very > > > doable. > > > > The alternative way is the one I shared before, using this: > > > > /* Thread created with attribute ATTR will be limited to run only on > > the processors represented in CPUSET. */ > > extern int pthread_attr_setaffinity_np (pthread_attr_t *__attr, > > size_t __cpusetsize, > > const cpu_set_t *__cpuset) > > > > Basically, the thread is created on the pCPU as user specified. > > I think this is better than "creating the thread on an arbitrary pCPU > > and then pinning it to the user specified pCPU in the thread's start routine". > > Ah, yeah, that's better. > pthread_create() will internally call sched_setaffinity() syscall after creation of a thread on a random CPU. So, from the performance side there is not much difference between the two approaches. However, we will still need pin_this_task_to_pcpu()/sched_affinity() to move the main thread to a specific pCPU, therefore, I am thinking of keeping the current approach unless there is a strong objection to it. > > Probably we also don't need "bool pin_vcpus". > > Yeah, but for selftests shaving bytes is not exactly top priority, and having a > dedicated flag avoids the need for magic numbers. If Vipin had used -1, I'd > probably be fine with that, but I'm also totally fine using a dedicated flag too. > Same, it is not performance critical in this case to add a magical -1. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-27 20:02 ` Vipin Sharma @ 2022-10-27 20:23 ` Sean Christopherson 2022-10-28 2:11 ` Wang, Wei W 1 sibling, 0 replies; 20+ messages in thread From: Sean Christopherson @ 2022-10-27 20:23 UTC (permalink / raw) To: Vipin Sharma Cc: Wang, Wei W, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Thu, Oct 27, 2022, Vipin Sharma wrote: > On Thu, Oct 27, 2022 at 8:56 AM Sean Christopherson <seanjc@google.com> wrote: > > > > On Thu, Oct 27, 2022, Wang, Wei W wrote: > > > On Wednesday, October 26, 2022 11:44 PM, Sean Christopherson wrote: > > > > If we go this route in the future, we'd need to add a worker trampoline as the > > > > pinning needs to happen in the worker task itself to guarantee that the pinning > > > > takes effect before the worker does anything useful. That should be very > > > > doable. > > > > > > The alternative way is the one I shared before, using this: > > > > > > /* Thread created with attribute ATTR will be limited to run only on > > > the processors represented in CPUSET. */ > > > extern int pthread_attr_setaffinity_np (pthread_attr_t *__attr, > > > size_t __cpusetsize, > > > const cpu_set_t *__cpuset) > > > > > > Basically, the thread is created on the pCPU as user specified. > > > I think this is better than "creating the thread on an arbitrary pCPU > > > and then pinning it to the user specified pCPU in the thread's start routine". > > > > Ah, yeah, that's better. > > > > pthread_create() will internally call sched_setaffinity() syscall > after creation of a thread on a random CPU. So, from the performance > side there is not much difference between the two approaches. > > However, we will still need pin_this_task_to_pcpu()/sched_affinity() > to move the main thread to a specific pCPU, therefore, Heh, that's a good point too. > I am thinking of keeping the current approach unless there is a strong objection > to it. No objection here, I don't see an obvious way to make that helper going away. ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-27 20:02 ` Vipin Sharma 2022-10-27 20:23 ` Sean Christopherson @ 2022-10-28 2:11 ` Wang, Wei W 2022-10-28 17:37 ` Vipin Sharma 1 sibling, 1 reply; 20+ messages in thread From: Wang, Wei W @ 2022-10-28 2:11 UTC (permalink / raw) To: Vipin Sharma, Christopherson,, Sean Cc: pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Friday, October 28, 2022 4:03 AM, Vipin Sharma wrote: > pthread_create() will internally call sched_setaffinity() syscall after creation of a > thread on a random CPU. So, from the performance side there is not much > difference between the two approaches. The main difference I see is that the vcpu could be created on one NUMA node by default initially and then gets pinned to another NUMA node. > > However, we will still need pin_this_task_to_pcpu()/sched_affinity() > to move the main thread to a specific pCPU, therefore, I am thinking of keeping > the current approach unless there is a strong objection to it. I also don’t have strong objections, and it's up to you for now. I will re-visit this later after the code consolidation patchset is landed and see how this could be better consolidated from all user's perspective. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs 2022-10-28 2:11 ` Wang, Wei W @ 2022-10-28 17:37 ` Vipin Sharma 0 siblings, 0 replies; 20+ messages in thread From: Vipin Sharma @ 2022-10-28 17:37 UTC (permalink / raw) To: Wang, Wei W Cc: Christopherson,, Sean, pbonzini, dmatlack, andrew.jones, kvm, linux-kernel On Thu, Oct 27, 2022 at 7:11 PM Wang, Wei W <wei.w.wang@intel.com> wrote: > > On Friday, October 28, 2022 4:03 AM, Vipin Sharma wrote: > > pthread_create() will internally call sched_setaffinity() syscall after creation of a > > thread on a random CPU. So, from the performance side there is not much > > difference between the two approaches. > > The main difference I see is that the vcpu could be created on one NUMA node by > default initially and then gets pinned to another NUMA node. > pthread_create(..., &attr,...) calls clone and then sched_setaffinity(). This is not different than calling pthread_create(...,NULL,...) and then explicitly calling sched_setaffinity() by a user. vCPU creation on one NUMA node and then getting pinned to another NUMA node is equally probable in both approaches. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-10-28 17:37 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-21 21:18 [PATCH v6 0/5] dirty_log_perf_test vCPU pinning Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 1/5] KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 2/5] KVM: selftests: Put command line options in alphabetical order " Vipin Sharma 2022-10-26 2:09 ` Wang, Wei W 2022-10-26 15:04 ` Sean Christopherson 2022-10-26 17:45 ` Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() Vipin Sharma 2022-10-26 2:16 ` Wang, Wei W 2022-10-26 15:10 ` Sean Christopherson 2022-10-21 21:18 ` [PATCH v6 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation Vipin Sharma 2022-10-21 21:18 ` [PATCH v6 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs Vipin Sharma 2022-10-26 2:27 ` Wang, Wei W 2022-10-26 15:44 ` Sean Christopherson 2022-10-26 18:17 ` Vipin Sharma 2022-10-27 12:03 ` Wang, Wei W 2022-10-27 15:56 ` Sean Christopherson 2022-10-27 20:02 ` Vipin Sharma 2022-10-27 20:23 ` Sean Christopherson 2022-10-28 2:11 ` Wang, Wei W 2022-10-28 17:37 ` Vipin Sharma
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.