[PATCH v3 0/4] dirty_log_perf_test cpu pinning and some goodies

[PATCH v3 0/4] dirty_log_perf_test cpu pinning and some goodies

[Vipin Sharma]

Pin vcpus to a host physical cpus 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() in other tests, sorted
command line options alphabetically, and made switch case logic of -e
option less error prone to code changes by adding break and decoupling
it from -g.

v3:
- 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 (4):
  KVM: selftests: Explicitly set variables based on options 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: Run dirty_log_perf_test on specific cpus

 .../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       | 65 +++++++++++++------
 .../selftests/kvm/include/perf_test_util.h    |  4 ++
 .../testing/selftests/kvm/include/test_util.h |  2 +
 .../selftests/kvm/kvm_page_table_test.c       |  2 +-
 .../selftests/kvm/lib/perf_test_util.c        | 62 +++++++++++++++++-
 tools/testing/selftests/kvm/lib/test_util.c   | 14 ++++
 .../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 +-
 15 files changed, 148 insertions(+), 45 deletions(-)

-- 
2.37.2.672.g94769d06f0-goog

[PATCH v3 1/4] KVM: selftests: Explicitly set variables based on options in dirty_log_perf_test

[Vipin Sharma]

Variable set via -g are also indirectly set by -e option by omitting
break statement. Set them explicitly so that movement of switch-case
statements does not unintentionally break features.

No functional change intended.

Signed-off-by: Vipin Sharma <vipinsh@google.com>
---
 tools/testing/selftests/kvm/dirty_log_perf_test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index f99e39a672d3..a03db7f9f4c0 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -411,6 +411,8 @@ int main(int argc, char *argv[])
 		case 'e':
 			/* 'e' is for evil. */
 			run_vcpus_while_disabling_dirty_logging = true;
+			dirty_log_manual_caps = 0;
+			break;
 		case 'g':
 			dirty_log_manual_caps = 0;
 			break;
-- 
2.37.2.672.g94769d06f0-goog

[PATCH v3 2/4] KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test

[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 a03db7f9f4c0..acf8b80c91d1 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -406,51 +406,53 @@ 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;
 			dirty_log_manual_caps = 0;
 			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.37.2.672.g94769d06f0-goog

[PATCH v3 3/4] KVM: selftests: Add atoi_paranoid to catch errors missed by atoi

[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>
---
 tools/testing/selftests/kvm/aarch64/arch_timer.c   |  8 ++++----
 tools/testing/selftests/kvm/aarch64/vgic_irq.c     |  6 +++---
 .../selftests/kvm/access_tracking_perf_test.c      |  2 +-
 tools/testing/selftests/kvm/demand_paging_test.c   |  2 +-
 tools/testing/selftests/kvm/dirty_log_perf_test.c  |  8 ++++----
 tools/testing/selftests/kvm/include/test_util.h    |  2 ++
 tools/testing/selftests/kvm/kvm_page_table_test.c  |  2 +-
 tools/testing/selftests/kvm/lib/test_util.c        | 14 ++++++++++++++
 .../testing/selftests/kvm/max_guest_memory_test.c  |  6 +++---
 .../kvm/memslot_modification_stress_test.c         |  4 ++--
 tools/testing/selftests/kvm/memslot_perf_test.c    | 10 +++++-----
 .../testing/selftests/kvm/set_memory_region_test.c |  2 +-
 .../selftests/kvm/x86_64/nx_huge_pages_test.c      |  4 ++--
 13 files changed, 43 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 1c2749b1481a..99b16302d94d 100644
--- a/tools/testing/selftests/kvm/access_tracking_perf_test.c
+++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c
@@ -361,7 +361,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 acf8b80c91d1..1346f6b5a9bd 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -417,7 +417,7 @@ int main(int argc, char *argv[])
 			dirty_log_manual_caps = 0;
 			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;
@@ -428,7 +428,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);
@@ -446,12 +446,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 5c5a88180b6c..56776f431733 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -150,4 +150,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..1e560c30a696 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -334,3 +334,17 @@ long get_run_delay(void)
 
 	return val[1];
 }
+
+int atoi_paranoid(const char *num_str)
+{
+	int num;
+	char *end_ptr;
+
+	errno = 0;
+	num = (int)strtol(num_str, &end_ptr, 10);
+	TEST_ASSERT(errno == 0, "Conversion error: %d\n", errno);
+	TEST_ASSERT(num_str != end_ptr && *end_ptr == '\0',
+		    "Invalid number string.\n");
+
+	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 cc6421716400..5e18d716782b 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
@@ -233,10 +233,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.37.2.672.g94769d06f0-goog

[PATCH v3 4/4] KVM: selftests: Run dirty_log_perf_test on specific cpus

[Vipin Sharma]

Add command line options, -c,  to run the vcpus and optionally the main
process on the specific cpus on a host machine. This 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.

Link: https://lore.kernel.org/lkml/20220801151928.270380-1-vipinsh@google.com
Signed-off-by: Vipin Sharma <vipinsh@google.com>
Suggested-by: David Matlack <dmatlack@google.com>
Suggested-by: Sean Christopherson <seanjc@google.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .../selftests/kvm/dirty_log_perf_test.c       | 23 ++++++-
 .../selftests/kvm/include/perf_test_util.h    |  4 ++
 .../selftests/kvm/lib/perf_test_util.c        | 62 ++++++++++++++++++-
 3 files changed, 86 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 1346f6b5a9bd..9514b5f28b67 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,18 @@ 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: Comma separated values of the physical CPUs, which will run\n"
+	       "     the vCPUs, optionally, followed by the main application thread cpu.\n"
+	       "     Number of values must be at least the number of vCPUs.\n"
+	       "     The very next number is used to pin main application thread.\n\n"
+	       "     Example: ./dirty_log_perf_test -v 3 -c 22,23,24,50\n"
+	       "     This means that the vcpu 0 will run on the physical cpu 22,\n"
+	       "     vcpu 1 on the physical cpu 23, vcpu 2 on the physical cpu 24\n"
+	       "     and the main thread will run on cpu 50.\n\n"
+	       "     Example: ./dirty_log_perf_test -v 3 -c 22,23,24\n"
+	       "     Same as the previous example except now main application\n"
+	       "     thread can run on any physical cpu\n\n"
+	       "     (default: No cpu mapping)\n");
 	puts("");
 	exit(0);
 }
@@ -398,6 +410,7 @@ int main(int argc, char *argv[])
 		.slots = 1,
 	};
 	int opt;
+	const char *pcpu_list = NULL;
 
 	dirty_log_manual_caps =
 		kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2);
@@ -406,11 +419,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;
@@ -459,6 +475,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..d02619f153a2 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;
+	bool pin_pcpu;
+	int pcpu;
 };
 
 struct perf_test_args {
@@ -60,4 +62,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[]);
 
+int 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..7a1e8223e7c7 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,9 +243,26 @@ void __weak perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_v
 	exit(KSFT_SKIP);
 }
 
+static void pin_me_to_pcpu(int pcpu)
+{
+	cpu_set_t cpuset;
+	int err;
+
+	CPU_ZERO(&cpuset);
+	CPU_SET(pcpu, &cpuset);
+	errno = 0;
+	err = sched_setaffinity(0, sizeof(cpu_set_t), &cpuset);
+	TEST_ASSERT(err == 0, "sched_setaffinity errored out: %d\n", errno);
+}
+
 static void *vcpu_thread_main(void *data)
 {
 	struct vcpu_thread *vcpu = data;
+	int idx = vcpu->vcpu_idx;
+	struct perf_test_vcpu_args *vcpu_args = &perf_test_args.vcpu_args[idx];
+
+	if (vcpu_args->pin_pcpu)
+		pin_me_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,43 @@ void perf_test_join_vcpu_threads(int nr_vcpus)
 	for (i = 0; i < nr_vcpus; i++)
 		pthread_join(vcpu_threads[i].thread, NULL);
 }
+
+int perf_test_setup_pinning(const char *pcpus_string, int nr_vcpus)
+{
+	char delim[2] = ",";
+	char *cpu, *cpu_list;
+	int i = 0, pcpu_num;
+
+	cpu_list = strdup(pcpus_string);
+	TEST_ASSERT(cpu_list, "strdup() allocation failed.\n");
+
+	cpu = strtok(cpu_list, delim);
+
+	// 1. Get all pcpus for vcpus
+	while (cpu && i < nr_vcpus) {
+		pcpu_num = atoi_paranoid(cpu);
+		TEST_ASSERT(pcpu_num >= 0, "Invalid cpu number: %d\n", pcpu_num);
+
+		perf_test_args.vcpu_args[i].pin_pcpu = true;
+		perf_test_args.vcpu_args[i++].pcpu = pcpu_num;
+
+		cpu = strtok(NULL, delim);
+	}
+
+	TEST_ASSERT(i == nr_vcpus,
+		    "Number of pcpus (%d) not sufficient for the number of vcpus (%d).",
+		    i, nr_vcpus);
+
+	// 2. Check if main worker is provided
+	if (cpu) {
+		pcpu_num = atoi_paranoid(cpu);
+		TEST_ASSERT(pcpu_num >= 0, "Invalid cpu number: %d\n", pcpu_num);
+
+		pin_me_to_pcpu(pcpu_num);
+
+		cpu = strtok(NULL, delim);
+	}
+
+	free(cpu_list);
+	return i;
+}
-- 
2.37.2.672.g94769d06f0-goog

Re: [PATCH v3 1/4] KVM: selftests: Explicitly set variables based on options in dirty_log_perf_test

[Vipin Sharma]

On Fri, Aug 26, 2022 at 11:45 AM Vipin Sharma <vipinsh@google.com> wrote:
>
> Variable set via -g are also indirectly set by -e option by omitting
> break statement. Set them explicitly so that movement of switch-case
> statements does not unintentionally break features.
>
> No functional change intended.
>
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> ---
>  tools/testing/selftests/kvm/dirty_log_perf_test.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> index f99e39a672d3..a03db7f9f4c0 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> @@ -411,6 +411,8 @@ int main(int argc, char *argv[])
>                 case 'e':
>                         /* 'e' is for evil. */
>                         run_vcpus_while_disabling_dirty_logging = true;
> +                       dirty_log_manual_caps = 0;
> +                       break;

@Sean, I hope you intentionally didn't write a break between -e and -g
when you created the patch and it is not just a missed thing :)


>                 case 'g':
>                         dirty_log_manual_caps = 0;
>                         break;
> --
> 2.37.2.672.g94769d06f0-goog
>

Re: [PATCH v3 2/4] KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test

[Andrew Jones]

On Fri, Aug 26, 2022 at 11:44:58AM -0700, 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.

Arguably it's actually easiest to insert into an unsorted list, but
kvm selftests loves alphabetical order (I'm looking at you Makefile
and .gitignore). Uh oh, I did just look at those files and they're
full of violations! Oh well... Let's see how long these command
lines options stay ordered :-)

Thanks,
drew

> 
> 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 a03db7f9f4c0..acf8b80c91d1 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> @@ -406,51 +406,53 @@ 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;
>  			dirty_log_manual_caps = 0;
>  			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.37.2.672.g94769d06f0-goog
> 

Re: [PATCH v3 0/4] dirty_log_perf_test cpu pinning and some goodies

[Vipin Sharma]

On Fri, Aug 26, 2022 at 11:45 AM Vipin Sharma <vipinsh@google.com> wrote:
>
> Pin vcpus to a host physical cpus 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() in other tests, sorted
> command line options alphabetically, and made switch case logic of -e
> option less error prone to code changes by adding break and decoupling
> it from -g.
>
> v3:
> - 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 (4):
>   KVM: selftests: Explicitly set variables based on options 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: Run dirty_log_perf_test on specific cpus
>
>  .../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       | 65 +++++++++++++------
>  .../selftests/kvm/include/perf_test_util.h    |  4 ++
>  .../testing/selftests/kvm/include/test_util.h |  2 +
>  .../selftests/kvm/kvm_page_table_test.c       |  2 +-
>  .../selftests/kvm/lib/perf_test_util.c        | 62 +++++++++++++++++-
>  tools/testing/selftests/kvm/lib/test_util.c   | 14 ++++
>  .../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 +-
>  15 files changed, 148 insertions(+), 45 deletions(-)
>
> --
> 2.37.2.672.g94769d06f0-goog
>

Knock Knock!!

Re: [PATCH v3 1/4] KVM: selftests: Explicitly set variables based on options in dirty_log_perf_test

[David Matlack]

On Fri, Aug 26, 2022 at 02:12:49PM -0700, Vipin Sharma wrote:
> On Fri, Aug 26, 2022 at 11:45 AM Vipin Sharma <vipinsh@google.com> wrote:
> >
> > Variable set via -g are also indirectly set by -e option by omitting
> > break statement. Set them explicitly so that movement of switch-case
> > statements does not unintentionally break features.
> >
> > No functional change intended.
> >
> > Signed-off-by: Vipin Sharma <vipinsh@google.com>
> > ---
> >  tools/testing/selftests/kvm/dirty_log_perf_test.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> > index f99e39a672d3..a03db7f9f4c0 100644
> > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> > @@ -411,6 +411,8 @@ int main(int argc, char *argv[])
> >                 case 'e':
> >                         /* 'e' is for evil. */
> >                         run_vcpus_while_disabling_dirty_logging = true;
> > +                       dirty_log_manual_caps = 0;
> > +                       break;
> 
> @Sean, I hope you intentionally didn't write a break between -e and -g
> when you created the patch and it is not just a missed thing :)

I'm pretty sure the missing break here is by accident. If it was on
purpose I would expect Sean (especially) to have called it out the
subtle change in the commit message and probably add a comment here.
Also I can't think of any reason why
run_vcpus_while_disabling_dirty_logging=true would require
dirty_log_manual_caps=0.

Can you change this patch to just add a break and add a fixes tag to the
commit message?

Fixes: cfe12e64b065 ("KVM: selftests: Add an option to run vCPUs while disabling dirty logging")


> 
> 
> >                 case 'g':
> >                         dirty_log_manual_caps = 0;
> >                         break;
> > --
> > 2.37.2.672.g94769d06f0-goog
> >

Re: [PATCH v3 3/4] KVM: selftests: Add atoi_paranoid to catch errors missed by atoi

[David Matlack]

On Fri, Aug 26, 2022 at 11:44:59AM -0700, Vipin Sharma wrote:
> 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.

Please use "()" after all function names.

> 
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> Suggested-by: David Matlack <dmatlack@google.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> ---
>  tools/testing/selftests/kvm/aarch64/arch_timer.c   |  8 ++++----
>  tools/testing/selftests/kvm/aarch64/vgic_irq.c     |  6 +++---
>  .../selftests/kvm/access_tracking_perf_test.c      |  2 +-
>  tools/testing/selftests/kvm/demand_paging_test.c   |  2 +-
>  tools/testing/selftests/kvm/dirty_log_perf_test.c  |  8 ++++----
>  tools/testing/selftests/kvm/include/test_util.h    |  2 ++
>  tools/testing/selftests/kvm/kvm_page_table_test.c  |  2 +-
>  tools/testing/selftests/kvm/lib/test_util.c        | 14 ++++++++++++++
>  .../testing/selftests/kvm/max_guest_memory_test.c  |  6 +++---
>  .../kvm/memslot_modification_stress_test.c         |  4 ++--
>  tools/testing/selftests/kvm/memslot_perf_test.c    | 10 +++++-----
>  .../testing/selftests/kvm/set_memory_region_test.c |  2 +-
>  .../selftests/kvm/x86_64/nx_huge_pages_test.c      |  4 ++--
>  13 files changed, 43 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 1c2749b1481a..99b16302d94d 100644
> --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c
> +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c
> @@ -361,7 +361,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 acf8b80c91d1..1346f6b5a9bd 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> @@ -417,7 +417,7 @@ int main(int argc, char *argv[])
>  			dirty_log_manual_caps = 0;
>  			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;
> @@ -428,7 +428,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);
> @@ -446,12 +446,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 5c5a88180b6c..56776f431733 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -150,4 +150,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..1e560c30a696 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -334,3 +334,17 @@ long get_run_delay(void)
>  
>  	return val[1];
>  }
> +
> +int atoi_paranoid(const char *num_str)
> +{
> +	int num;
> +	char *end_ptr;
> +
> +	errno = 0;
> +	num = (int)strtol(num_str, &end_ptr, 10);
> +	TEST_ASSERT(errno == 0, "Conversion error: %d\n", errno);

"Conversion error: " is a bit vague. Also, TEST_ASSERT() already logs
errno and strerror(errno). It would probably be more useful here to log
the input string that caused the conversion error.

How about this?

        TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str);

> +	TEST_ASSERT(num_str != end_ptr && *end_ptr == '\0',
> +		    "Invalid number string.\n");

"Invalid number string." is also a bit vague. How about this?

        TEST_ASSERT(num_str != end_ptr && *end_ptr == '\0',
                    "strtol(\"%s\") failed to parse trailing characters \"%s\"",
                    num_str, end_ptr);

> +
> +	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 cc6421716400..5e18d716782b 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
> @@ -233,10 +233,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.37.2.672.g94769d06f0-goog
> 

Re: [PATCH v3 4/4] KVM: selftests: Run dirty_log_perf_test on specific cpus

[David Matlack]

On Fri, Aug 26, 2022 at 11:45:00AM -0700, Vipin Sharma wrote:
> Add command line options, -c,  to run the vcpus and optionally the main
> process on the specific cpus on a host machine. This 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.
> 
> Link: https://lore.kernel.org/lkml/20220801151928.270380-1-vipinsh@google.com
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> Suggested-by: David Matlack <dmatlack@google.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .../selftests/kvm/dirty_log_perf_test.c       | 23 ++++++-
>  .../selftests/kvm/include/perf_test_util.h    |  4 ++
>  .../selftests/kvm/lib/perf_test_util.c        | 62 ++++++++++++++++++-
>  3 files changed, 86 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 1346f6b5a9bd..9514b5f28b67 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,18 @@ 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: Comma separated values of the physical CPUs, which will run\n"
> +	       "     the vCPUs, optionally, followed by the main application thread cpu.\n"
> +	       "     Number of values must be at least the number of vCPUs.\n"
> +	       "     The very next number is used to pin main application thread.\n\n"
> +	       "     Example: ./dirty_log_perf_test -v 3 -c 22,23,24,50\n"
> +	       "     This means that the vcpu 0 will run on the physical cpu 22,\n"
> +	       "     vcpu 1 on the physical cpu 23, vcpu 2 on the physical cpu 24\n"
> +	       "     and the main thread will run on cpu 50.\n\n"
> +	       "     Example: ./dirty_log_perf_test -v 3 -c 22,23,24\n"
> +	       "     Same as the previous example except now main application\n"
> +	       "     thread can run on any physical cpu\n\n"
> +	       "     (default: No cpu mapping)\n");
>  	puts("");
>  	exit(0);
>  }
> @@ -398,6 +410,7 @@ int main(int argc, char *argv[])
>  		.slots = 1,
>  	};
>  	int opt;
> +	const char *pcpu_list = NULL;
>  
>  	dirty_log_manual_caps =
>  		kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2);
> @@ -406,11 +419,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;
> @@ -459,6 +475,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..d02619f153a2 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;
> +	bool pin_pcpu;
> +	int pcpu;
>  };
>  
>  struct perf_test_args {
> @@ -60,4 +62,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[]);
>  
> +int 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..7a1e8223e7c7 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,9 +243,26 @@ void __weak perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_v
>  	exit(KSFT_SKIP);
>  }
>  
> +static void pin_me_to_pcpu(int pcpu)
> +{
> +	cpu_set_t cpuset;
> +	int err;
> +
> +	CPU_ZERO(&cpuset);
> +	CPU_SET(pcpu, &cpuset);
> +	errno = 0;

No need to set errno explicitly here since sched_setaffinity() is
defined to set errno if it returns -1.

> +	err = sched_setaffinity(0, sizeof(cpu_set_t), &cpuset);
> +	TEST_ASSERT(err == 0, "sched_setaffinity errored out: %d\n", errno);

TEST_ASSERT() already prints errno. It would be more useful to print
@pcpu to help the user debug the failure.

Also, use "()" after function names.

> +}
> +
>  static void *vcpu_thread_main(void *data)
>  {
>  	struct vcpu_thread *vcpu = data;
> +	int idx = vcpu->vcpu_idx;
> +	struct perf_test_vcpu_args *vcpu_args = &perf_test_args.vcpu_args[idx];
> +
> +	if (vcpu_args->pin_pcpu)
> +		pin_me_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,43 @@ void perf_test_join_vcpu_threads(int nr_vcpus)
>  	for (i = 0; i < nr_vcpus; i++)
>  		pthread_join(vcpu_threads[i].thread, NULL);
>  }
> +
> +int perf_test_setup_pinning(const char *pcpus_string, int nr_vcpus)
> +{
> +	char delim[2] = ",";
> +	char *cpu, *cpu_list;
> +	int i = 0, pcpu_num;
> +
> +	cpu_list = strdup(pcpus_string);
> +	TEST_ASSERT(cpu_list, "strdup() allocation failed.\n");
> +
> +	cpu = strtok(cpu_list, delim);
> +
> +	// 1. Get all pcpus for vcpus
> +	while (cpu && i < nr_vcpus) {
> +		pcpu_num = atoi_paranoid(cpu);
> +		TEST_ASSERT(pcpu_num >= 0, "Invalid cpu number: %d\n", pcpu_num);
> +
> +		perf_test_args.vcpu_args[i].pin_pcpu = true;

Since pinning vCPU is all or nothing, this can be a single bool instead
of per-vCPUs.

        /* True if vCPUs are pinned to pCPUs. */
        perf_test_args.pin_vcpus

        /* The pCPU to which this vCPU is pinned. Only valid if pin_vcpus is true. */
        perf_test_args.vcpus_args[i].pcpu

> +		perf_test_args.vcpu_args[i++].pcpu = pcpu_num;
> +
> +		cpu = strtok(NULL, delim);
> +	}
> +
> +	TEST_ASSERT(i == nr_vcpus,
> +		    "Number of pcpus (%d) not sufficient for the number of vcpus (%d).",
> +		    i, nr_vcpus);
> +
> +	// 2. Check if main worker is provided
> +	if (cpu) {
> +		pcpu_num = atoi_paranoid(cpu);
> +		TEST_ASSERT(pcpu_num >= 0, "Invalid cpu number: %d\n", pcpu_num);

nite: Create a helper function for this since it's repeated twice.

> +
> +		pin_me_to_pcpu(pcpu_num);
> +
> +		cpu = strtok(NULL, delim);

Delete?

> +	}
> +
> +	free(cpu_list);
> +	return i;

The return value is unused. Drop it until we have a usecase for it.

> +}
> -- 
> 2.37.2.672.g94769d06f0-goog
>