[PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[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.

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/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: 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

[PATCH v5 1/5] KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test

[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.rc1.362.ged0d419d3c-goog

[PATCH v5 2/5] 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 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.rc1.362.ged0d419d3c-goog

[PATCH v5 3/5] 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>

---
 .../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

[PATCH v5 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation

[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.rc1.362.ged0d419d3c-goog

[PATCH v5 5/5] KVM: selftests: Run dirty_log_perf_test on specific CPUs

[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       | 23 ++++++-
 .../selftests/kvm/include/perf_test_util.h    |  6 ++
 .../selftests/kvm/lib/perf_test_util.c        | 65 ++++++++++++++++++-
 3 files changed, 91 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..eec8bff77767 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: 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"
+	       "     Example: ./dirty_log_perf_test -v 3 -c 22,23,24,50\n"
+	       "     will create 3 vCPUs, and pin vCPU0=>pCPU22, vCPU1=>pCPU23\n"
+	       "     vCPU2=>pCPU24, and pin the application task to pCPU50.\n"
+	       "     To leave the application task unpinned, drop the final\n"
+	       "     entry:\n"
+	       "       ./dirty_log_perf_test -v 3 -c 22,23,24\n\n"
+	       "     (default: no pinning)\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;
@@ -456,6 +472,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.rc1.362.ged0d419d3c-goog

RE: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[Wang, Wei W]

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

RE: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[Wang, Wei W]

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.

Re: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[Sean Christopherson]

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

Re: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[David Matlack]

On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> 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

Re: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[Vipin Sharma]

On Fri, Oct 14, 2022 at 9:55 AM David Matlack <dmatlack@google.com> wrote:
>
> On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> 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?

Re: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[Sean Christopherson]

On Fri, Oct 14, 2022, Vipin Sharma wrote:
> On Fri, Oct 14, 2022 at 9:55 AM David Matlack <dmatlack@google.com> wrote:
> >
> > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> 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).

Re: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[Vipin Sharma]

On Fri, Oct 14, 2022 at 12:03 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Oct 14, 2022, Vipin Sharma wrote:
> > On Fri, Oct 14, 2022 at 9:55 AM David Matlack <dmatlack@google.com> wrote:
> > >
> > > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> 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!

RE: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[Wang, Wei W]

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 <dmatlack@google.com>
> wrote:
> > >
> > > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com>
> 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.

Re: [PATCH v5 5/5] KVM: selftests: Run dirty_log_perf_test on specific CPUs

[Sean Christopherson]

On Mon, Oct 10, 2022, Vipin Sharma wrote:
> @@ -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: 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"
> +	       "     Example: ./dirty_log_perf_test -v 3 -c 22,23,24,50\n"
> +	       "     will create 3 vCPUs, and pin vCPU0=>pCPU22, vCPU1=>pCPU23\n"
> +	       "     vCPU2=>pCPU24, and pin the application task to pCPU50.\n"
> +	       "     To leave the application task unpinned, drop the final\n"
> +	       "     entry:\n"
> +	       "       ./dirty_log_perf_test -v 3 -c 22,23,24\n\n"
> +	       "     (default: no pinning)\n");

Any objection to tweaking the formatting to make the output easier to parse?  And
to match the indentation used by guest_modes_help().  E.g. This

	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");

yields:

 -c: Pin tasks to physical CPUs.  Takes a list of comma separated
     values (target pCPU), one for each vCPU, plus an optional
     entry for the main application task (specified via entry
     <nr_vcpus + 1>).  If used, entries must be provided for all
     vCPUs, i.e. pinning vCPUs is all or nothing.

     E.g. to create 3 vCPUs, pin vCPU0=>pCPU22, vCPU1=>pCPU23
     vCPU2=>pCPU24, and pin the application task to pCPU50:

         ./dirty_log_perf_test -v 3 -c 22,23,24,50

     To leave the application task unpinned, drop the final entry:

         ./dirty_log_perf_test -v 3 -c 22,23,24

     (default: no pinning)


versus:

 -c: Pin tasks to physical CPUs.  Takes a list of comma separated
     values (target pCPU), one for each vCPU, plus an optional
     entry for the main application task (specified via entry
     <nr_vcpus + 1>).  If used, entries must be provided for all
     vCPUs, i.e. pinning vCPUs is all or nothing.

     Example: ./dirty_log_perf_test -v 3 -c 22,23,24,50
     will create 3 vCPUs, and pin vCPU0=>pCPU22, vCPU1=>pCPU23
     vCPU2=>pCPU24, and pin the application task to pCPU50.
     To leave the application task unpinned, drop the final
     entry:
       ./dirty_log_perf_test -v 3 -c 22,23,24

     (default: no pinning)

Re: [PATCH v5 5/5] KVM: selftests: Run dirty_log_perf_test on specific CPUs

[Sean Christopherson]

Almost forgot, the shortlog should be something like:

  KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs

since the test doesn't pin by default, i.e. "Run ... on specific CPUs" is a bit
of a lie.

Re: [PATCH v5 0/5] dirty_log_perf_test vCPU pinning

[Sean Christopherson]

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.