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.
v5:
- 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/[email protected]/
- 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/[email protected]
- 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/[email protected]/
- 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/[email protected]
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: Run 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 | 62 ++++++++++++------
.../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, 178 insertions(+), 78 deletions(-)
--
2.38.0.rc1.362.ged0d419d3c-goog
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 <[email protected]>
---
.../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.rc1.362.ged0d419d3c-goog
On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> 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.
Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
For example, with below changes:
diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/se lftests/kvm/lib/perf_test_util.c
index 9618b37c66f7..aac58d1acb3c 100644
--- a/tools/testing/selftests/kvm/lib/perf_test_util.c
+++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
@@ -264,6 +264,7 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
void (*vcpu_fn)(struct perf_test_vcpu_args *))
{
int i;
+ char vcpu_name[5];
vcpu_thread_fn = vcpu_fn;
WRITE_ONCE(all_vcpu_threads_running, false);
@@ -275,6 +276,8 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
WRITE_ONCE(vcpu->running, false);
pthread_create(&vcpu->thread, NULL, vcpu_thread_main, vcpu);
+ sprintf(vcpu_name, "%s%d", "vcpu", i);
+ pthread_setname_np(vcpu->thread, vcpu_name);
}
and with top we can get
PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
4464 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.97 dirty_log_perf_
4467 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu0
4469 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu2
4470 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.94 vcpu3
4468 root 20 0 4248684 4.0g 1628 R 99.7 26.2 0:50.93 vcpu1
On Friday, October 14, 2022 11:12 PM, Wang, Wei W wrote:
> On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> > 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.
>
> Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
>
> For example, with below changes:
> diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/se
> lftests/kvm/lib/perf_test_util.c
> index 9618b37c66f7..aac58d1acb3c 100644
> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> @@ -264,6 +264,7 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> void (*vcpu_fn)(struct
> perf_test_vcpu_args *)) {
> int i;
> + char vcpu_name[5];
Typo, should be "vcpu_name[6]" here.
On Fri, Oct 14, 2022, Wang, Wei W wrote:
> On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> > 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.
>
> Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
IIUC, you're suggesting the test give tasks meaningful names so that the user can
do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before
they do any meaningful work. I don't see how that can be accomplished with taskset
without some form of hook in the test to effectively pause the test until the user
(or some run script) is ready to continue.
Pinning aside, naming the threads is a great idea! That would definitely help
debug, e.g. if one vCPU gets stuck or is lagging behind.
>
> For example, with below changes:
> diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/se lftests/kvm/lib/perf_test_util.c
> index 9618b37c66f7..aac58d1acb3c 100644
> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> @@ -264,6 +264,7 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> void (*vcpu_fn)(struct perf_test_vcpu_args *))
> {
> int i;
> + char vcpu_name[5];
>
> vcpu_thread_fn = vcpu_fn;
> WRITE_ONCE(all_vcpu_threads_running, false);
> @@ -275,6 +276,8 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> WRITE_ONCE(vcpu->running, false);
>
> pthread_create(&vcpu->thread, NULL, vcpu_thread_main, vcpu);
> + sprintf(vcpu_name, "%s%d", "vcpu", i);
> + pthread_setname_np(vcpu->thread, vcpu_name);
> }
>
> and with top we can get
> PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
> 4464 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.97 dirty_log_perf_
> 4467 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu0
> 4469 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu2
> 4470 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.94 vcpu3
> 4468 root 20 0 4248684 4.0g 1628 R 99.7 26.2 0:50.93 vcpu1
On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <[email protected]> wrote:
>
> On Fri, Oct 14, 2022, Wang, Wei W wrote:
> > On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> > > 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.
> >
> > Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
>
> IIUC, you're suggesting the test give tasks meaningful names so that the user can
> do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before
> they do any meaningful work. I don't see how that can be accomplished with taskset
> without some form of hook in the test to effectively pause the test until the user
> (or some run script) is ready to continue.
A taskset approach would also be more difficult to incorporate into
automated runs of dirty_log_perf_test.
>
> Pinning aside, naming the threads is a great idea! That would definitely help
> debug, e.g. if one vCPU gets stuck or is lagging behind.
+1
>
> >
> > For example, with below changes:
> > diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/se lftests/kvm/lib/perf_test_util.c
> > index 9618b37c66f7..aac58d1acb3c 100644
> > --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> > +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> > @@ -264,6 +264,7 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> > void (*vcpu_fn)(struct perf_test_vcpu_args *))
> > {
> > int i;
> > + char vcpu_name[5];
> >
> > vcpu_thread_fn = vcpu_fn;
> > WRITE_ONCE(all_vcpu_threads_running, false);
> > @@ -275,6 +276,8 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> > WRITE_ONCE(vcpu->running, false);
> >
> > pthread_create(&vcpu->thread, NULL, vcpu_thread_main, vcpu);
> > + sprintf(vcpu_name, "%s%d", "vcpu", i);
> > + pthread_setname_np(vcpu->thread, vcpu_name);
> > }
> >
> > and with top we can get
> > PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
> > 4464 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.97 dirty_log_perf_
> > 4467 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu0
> > 4469 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu2
> > 4470 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.94 vcpu3
> > 4468 root 20 0 4248684 4.0g 1628 R 99.7 26.2 0:50.93 vcpu1
On Fri, Oct 14, 2022 at 9:55 AM David Matlack <[email protected]> wrote:
>
> On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <[email protected]> wrote:
> >
> > On Fri, Oct 14, 2022, Wang, Wei W wrote:
> > > On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> > > > 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.
> > >
> > > Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
> >
> > IIUC, you're suggesting the test give tasks meaningful names so that the user can
> > do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before
> > they do any meaningful work. I don't see how that can be accomplished with taskset
> > without some form of hook in the test to effectively pause the test until the user
> > (or some run script) is ready to continue.
>
> A taskset approach would also be more difficult to incorporate into
> automated runs of dirty_log_perf_test.
>
> >
> > Pinning aside, naming the threads is a great idea! That would definitely help
> > debug, e.g. if one vCPU gets stuck or is lagging behind.
>
> +1
I also like the idea.
Sean:
Do you want a v6 with the naming patch or you will be fine taking v5,
if there are no changes needed in v5, and I can send a separate patch
for naming?
On Fri, Oct 14, 2022, Vipin Sharma wrote:
> On Fri, Oct 14, 2022 at 9:55 AM David Matlack <[email protected]> wrote:
> >
> > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <[email protected]> wrote:
> > >
> > > On Fri, Oct 14, 2022, Wang, Wei W wrote:
> > > > Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
> > >
> > > IIUC, you're suggesting the test give tasks meaningful names so that the user can
> > > do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before
> > > they do any meaningful work. I don't see how that can be accomplished with taskset
> > > without some form of hook in the test to effectively pause the test until the user
> > > (or some run script) is ready to continue.
> >
> > A taskset approach would also be more difficult to incorporate into
> > automated runs of dirty_log_perf_test.
> >
> > >
> > > Pinning aside, naming the threads is a great idea! That would definitely help
> > > debug, e.g. if one vCPU gets stuck or is lagging behind.
> >
> > +1
>
> I also like the idea.
>
> Sean:
> Do you want a v6 with the naming patch or you will be fine taking v5,
> if there are no changes needed in v5, and I can send a separate patch
> for naming?
Definitely separate, this is an orthogonal change and I don't think there will be
any conflict. If there is a conflict, it will be trivial to resolve. But since
Wei provided a more or less complete patch, let's let Wei post a formal patch
(unless he doesn't want to).
On Fri, Oct 14, 2022 at 12:03 PM Sean Christopherson <[email protected]> wrote:
>
> On Fri, Oct 14, 2022, Vipin Sharma wrote:
> > On Fri, Oct 14, 2022 at 9:55 AM David Matlack <[email protected]> wrote:
> > >
> > > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <[email protected]> wrote:
> > > >
> > > > On Fri, Oct 14, 2022, Wang, Wei W wrote:
> > > > > Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
> > > >
> > > > IIUC, you're suggesting the test give tasks meaningful names so that the user can
> > > > do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before
> > > > they do any meaningful work. I don't see how that can be accomplished with taskset
> > > > without some form of hook in the test to effectively pause the test until the user
> > > > (or some run script) is ready to continue.
> > >
> > > A taskset approach would also be more difficult to incorporate into
> > > automated runs of dirty_log_perf_test.
> > >
> > > >
> > > > Pinning aside, naming the threads is a great idea! That would definitely help
> > > > debug, e.g. if one vCPU gets stuck or is lagging behind.
> > >
> > > +1
> >
> > I also like the idea.
> >
> > Sean:
> > Do you want a v6 with the naming patch or you will be fine taking v5,
> > if there are no changes needed in v5, and I can send a separate patch
> > for naming?
>
> Definitely separate, this is an orthogonal change and I don't think there will be
> any conflict. If there is a conflict, it will be trivial to resolve. But since
> Wei provided a more or less complete patch, let's let Wei post a formal patch
> (unless he doesn't want to).
Sounds good!
On Saturday, October 15, 2022 3:03 AM, Sean Christopherson wrote:
> On Fri, Oct 14, 2022, Vipin Sharma wrote:
> > On Fri, Oct 14, 2022 at 9:55 AM David Matlack <[email protected]>
> wrote:
> > >
> > > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <[email protected]>
> wrote:
> > > >
> > > > On Fri, Oct 14, 2022, Wang, Wei W wrote:
> > > > > Just curious why not re-using the existing tools (e.g. taskset) to do the
> pinning?
> > > >
> > > > IIUC, you're suggesting the test give tasks meaningful names so
> > > > that the user can do taskset on the appropriate tasks? The goal
> > > > is to ensure vCPUs are pinned before they do any meaningful work.
> > > > I don't see how that can be accomplished with taskset without some
> > > > form of hook in the test to effectively pause the test until the user (or
> some run script) is ready to continue.
> > >
> > > A taskset approach would also be more difficult to incorporate into
> > > automated runs of dirty_log_perf_test.
> > >
> > > >
> > > > Pinning aside, naming the threads is a great idea! That would
> > > > definitely help debug, e.g. if one vCPU gets stuck or is lagging behind.
> > >
> > > +1
> >
> > I also like the idea.
> >
> > Sean:
> > Do you want a v6 with the naming patch or you will be fine taking v5,
> > if there are no changes needed in v5, and I can send a separate patch
> > for naming?
>
> Definitely separate, this is an orthogonal change and I don't think there will be
> any conflict. If there is a conflict, it will be trivial to resolve. But since Wei
> provided a more or less complete patch, let's let Wei post a formal patch
> (unless he doesn't want to).
Yeah, I'm glad to take care of this. There are other places (e.g. hardware_disable_test.c)
that seem good to have this as well, I'll cover them in one patchset.
On Mon, Oct 10, 2022, Vipin Sharma wrote:
> 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.
...
> 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: Run dirty_log_perf_test on specific CPUs
Minor nits on patch 5, but otherwise looks good. Might be worth sending a v6
just so there's no confusion.