[PATCH v7 0/3] KVM: selftests: randomize memory access of dirty_log_perf_test

[PATCH v7 0/3] KVM: selftests: randomize memory access of dirty_log_perf_test

[Colton Lewis]

Add the ability to randomize parts of dirty_log_perf_test,
specifically the order pages are accessed and whether pages are read
or written.

v7:

Encapsulate the random state inside a struct. Add detail to names of
those functions. Change interface so random function returns the next
random number rather than using an out parameter.

Rebased to kvm/queue to ensure freshness.

Colton Lewis (3):
  KVM: selftests: implement random number generation for guest code
  KVM: selftests: randomize which pages are written vs read
  KVM: selftests: randomize page access order

 .../selftests/kvm/access_tracking_perf_test.c |  2 +-
 .../selftests/kvm/dirty_log_perf_test.c       | 58 ++++++++++++++-----
 .../selftests/kvm/include/perf_test_util.h    |  8 ++-
 .../testing/selftests/kvm/include/test_util.h |  7 +++
 .../selftests/kvm/lib/perf_test_util.c        | 32 ++++++++--
 tools/testing/selftests/kvm/lib/test_util.c   | 17 ++++++
 6 files changed, 100 insertions(+), 24 deletions(-)

--
2.38.0.413.g74048e4d9e-goog

[PATCH v7 1/3] KVM: selftests: implement random number generation for guest code

[Colton Lewis]

Implement random number generation for guest code to randomize parts
of the test, making it less predictable and a more accurate reflection
of reality.

Create a -r argument to specify a random seed. If no argument is
provided, the seed defaults to 0. The random seed is set with
perf_test_set_random_seed() and must be set before guest_code runs to
apply.

The random number generator chosen is the Park-Miller Linear
Congruential Generator, a fancy name for a basic and well-understood
random number generator entirely sufficient for this purpose. Each
vCPU calculates its own seed by adding its index to the seed provided.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
Reviewed-by: Ricardo Koller <ricarkol@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
 .../testing/selftests/kvm/dirty_log_perf_test.c | 12 ++++++++++--
 .../selftests/kvm/include/perf_test_util.h      |  2 ++
 tools/testing/selftests/kvm/include/test_util.h |  7 +++++++
 .../testing/selftests/kvm/lib/perf_test_util.c  |  7 +++++++
 tools/testing/selftests/kvm/lib/test_util.c     | 17 +++++++++++++++++
 5 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index f99e39a672d3..c97a5e455699 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -132,6 +132,7 @@ struct test_params {
 	bool partition_vcpu_memory_access;
 	enum vm_mem_backing_src_type backing_src;
 	int slots;
+	uint32_t random_seed;
 };
 
 static void toggle_dirty_logging(struct kvm_vm *vm, int slots, bool enable)
@@ -225,6 +226,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 				 p->slots, p->backing_src,
 				 p->partition_vcpu_memory_access);
 
+	/* If no argument provided, random seed will be 1. */
+	pr_info("Random seed: %u\n", p->random_seed);
+	perf_test_set_random_seed(vm, p->random_seed ? p->random_seed : 1);
 	perf_test_set_wr_fract(vm, p->wr_fract);
 
 	guest_num_pages = (nr_vcpus * guest_percpu_mem_size) >> vm->page_shift;
@@ -352,7 +356,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]"
+	       "[-m mode] [-n] [-b vcpu bytes] [-v vcpus] [-o] [-r random seed ] [-s mem type]"
 	       "[-x memslots]\n", name);
 	puts("");
 	printf(" -i: specify iteration counts (default: %"PRIu64")\n",
@@ -380,6 +384,7 @@ static void help(char *name)
 	printf(" -v: specify the number of vCPUs to run.\n");
 	printf(" -o: Overlap guest memory accesses instead of partitioning\n"
 	       "     them into a separate region of memory for each vCPU.\n");
+	printf(" -r: specify the starting random seed.\n");
 	backing_src_help("-s");
 	printf(" -x: Split the memory region into this number of memslots.\n"
 	       "     (default: 1)\n");
@@ -406,7 +411,7 @@ 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, "eghi:p:m:nb:f:v:or:s:x:")) != -1) {
 		switch (opt) {
 		case 'e':
 			/* 'e' is for evil. */
@@ -442,6 +447,9 @@ int main(int argc, char *argv[])
 		case 'o':
 			p.partition_vcpu_memory_access = false;
 			break;
+		case 'r':
+			p.random_seed = atoi(optarg);
+			break;
 		case 's':
 			p.backing_src = parse_backing_src_type(optarg);
 			break;
diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h
index eaa88df0555a..f1050fd42d10 100644
--- a/tools/testing/selftests/kvm/include/perf_test_util.h
+++ b/tools/testing/selftests/kvm/include/perf_test_util.h
@@ -35,6 +35,7 @@ struct perf_test_args {
 	uint64_t gpa;
 	uint64_t size;
 	uint64_t guest_page_size;
+	uint32_t random_seed;
 	int wr_fract;
 
 	/* Run vCPUs in L2 instead of L1, if the architecture supports it. */
@@ -52,6 +53,7 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int nr_vcpus,
 void perf_test_destroy_vm(struct kvm_vm *vm);
 
 void perf_test_set_wr_fract(struct kvm_vm *vm, int wr_fract);
+void perf_test_set_random_seed(struct kvm_vm *vm, uint32_t random_seed);
 
 void perf_test_start_vcpu_threads(int vcpus, void (*vcpu_fn)(struct perf_test_vcpu_args *));
 void perf_test_join_vcpu_threads(int vcpus);
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index befc754ce9b3..9e4f36a1a8b0 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -152,4 +152,11 @@ static inline void *align_ptr_up(void *x, size_t size)
 	return (void *)align_up((unsigned long)x, size);
 }
 
+struct guest_random_state {
+	uint32_t seed;
+};
+
+struct guest_random_state new_guest_random_state(uint32_t seed);
+uint32_t guest_random_u32(struct guest_random_state *state);
+
 #endif /* SELFTEST_KVM_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..5f0eebb626b5 100644
--- a/tools/testing/selftests/kvm/lib/perf_test_util.c
+++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
@@ -49,6 +49,7 @@ void perf_test_guest_code(uint32_t vcpu_idx)
 	uint64_t gva;
 	uint64_t pages;
 	int i;
+	struct guest_random_state rand_state = new_guest_random_state(pta->random_seed + vcpu_idx);
 
 	gva = vcpu_args->gva;
 	pages = vcpu_args->pages;
@@ -229,6 +230,12 @@ void perf_test_set_wr_fract(struct kvm_vm *vm, int wr_fract)
 	sync_global_to_guest(vm, perf_test_args);
 }
 
+void perf_test_set_random_seed(struct kvm_vm *vm, uint32_t random_seed)
+{
+	perf_test_args.random_seed = random_seed;
+	sync_global_to_guest(vm, perf_test_args.random_seed);
+}
+
 uint64_t __weak perf_test_nested_pages(int nr_vcpus)
 {
 	return 0;
diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index 6d23878bbfe1..c4d2749fb2c3 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -17,6 +17,23 @@
 
 #include "test_util.h"
 
+/*
+ * Random number generator that is usable from guest code. This is the
+ * Park-Miller LCG using standard constants.
+ */
+
+struct guest_random_state new_guest_random_state(uint32_t seed)
+{
+	struct guest_random_state s = {.seed = seed};
+	return s;
+}
+
+uint32_t guest_random_u32(struct guest_random_state *state)
+{
+	state->seed = (uint64_t)state->seed * 48271 % ((uint32_t)(1 << 31) - 1);
+	return state->seed;
+}
+
 /*
  * Parses "[0-9]+[kmgt]?".
  */
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v7 2/3] KVM: selftests: randomize which pages are written vs read

[Colton Lewis]

Randomize which pages are written vs read using the random number
generator.

Change the variable wr_fract and associated function calls to
write_percent that now operates as a percentage from 0 to 100 where X
means each page has an X% chance of being written. Change the -f
argument to -w to reflect the new variable semantics. Keep the same
default of 100% writes.

Population always uses 100% writes to ensure all memory is actually
populated and not just mapped to the zero page. The prevents expensive
copy-on-write faults from occurring during the dirty memory iterations
below, which would pollute the performance results.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
Reviewed-by: Ricardo Koller <ricarkol@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
 .../selftests/kvm/access_tracking_perf_test.c |  2 +-
 .../selftests/kvm/dirty_log_perf_test.c       | 38 ++++++++++++-------
 .../selftests/kvm/include/perf_test_util.h    |  4 +-
 .../selftests/kvm/lib/perf_test_util.c        | 10 ++---
 4 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c
index 76c583a07ea2..3e16d5bd7856 100644
--- a/tools/testing/selftests/kvm/access_tracking_perf_test.c
+++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c
@@ -279,7 +279,7 @@ static void run_iteration(struct kvm_vm *vm, int nr_vcpus, const char *descripti
 static void access_memory(struct kvm_vm *vm, int nr_vcpus,
 			  enum access_type access, const char *description)
 {
-	perf_test_set_wr_fract(vm, (access == ACCESS_READ) ? INT_MAX : 1);
+	perf_test_set_write_percent(vm, (access == ACCESS_READ) ? 0 : 100);
 	iteration_work = ITERATION_ACCESS_MEMORY;
 	run_iteration(vm, nr_vcpus, description);
 }
diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index c97a5e455699..0d0240041acf 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -128,10 +128,10 @@ static void vcpu_worker(struct perf_test_vcpu_args *vcpu_args)
 struct test_params {
 	unsigned long iterations;
 	uint64_t phys_offset;
-	int wr_fract;
 	bool partition_vcpu_memory_access;
 	enum vm_mem_backing_src_type backing_src;
 	int slots;
+	uint32_t write_percent;
 	uint32_t random_seed;
 };
 
@@ -229,7 +229,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 	/* If no argument provided, random seed will be 1. */
 	pr_info("Random seed: %u\n", p->random_seed);
 	perf_test_set_random_seed(vm, p->random_seed ? p->random_seed : 1);
-	perf_test_set_wr_fract(vm, p->wr_fract);
+	perf_test_set_write_percent(vm, p->write_percent);
 
 	guest_num_pages = (nr_vcpus * guest_percpu_mem_size) >> vm->page_shift;
 	guest_num_pages = vm_adjust_num_guest_pages(mode, guest_num_pages);
@@ -252,6 +252,14 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 	for (i = 0; i < nr_vcpus; i++)
 		vcpu_last_completed_iteration[i] = -1;
 
+	/*
+	 * Use 100% writes during the population phase to ensure all
+	 * memory is actually populated and not just mapped to the zero
+	 * page. The prevents expensive copy-on-write faults from
+	 * occurring during the dirty memory iterations below, which
+	 * would pollute the performance results.
+	 */
+	perf_test_set_write_percent(vm, 100);
 	perf_test_start_vcpu_threads(nr_vcpus, vcpu_worker);
 
 	/* Allow the vCPUs to populate memory */
@@ -273,6 +281,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 	pr_info("Enabling dirty logging time: %ld.%.9lds\n\n",
 		ts_diff.tv_sec, ts_diff.tv_nsec);
 
+	perf_test_set_write_percent(vm, p->write_percent);
+
 	while (iteration < p->iterations) {
 		/*
 		 * Incrementing the iteration number will start the vCPUs
@@ -357,7 +367,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] [-r random seed ] [-s mem type]"
-	       "[-x memslots]\n", name);
+	       "[-x memslots] [-w percentage]\n", name);
 	puts("");
 	printf(" -i: specify iteration counts (default: %"PRIu64")\n",
 	       TEST_HOST_LOOP_N);
@@ -377,10 +387,6 @@ static void help(char *name)
 	printf(" -b: specify the size of the memory region which should be\n"
 	       "     dirtied by each vCPU. e.g. 10M or 3G.\n"
 	       "     (default: 1G)\n");
-	printf(" -f: specify the fraction of pages which should be written to\n"
-	       "     as opposed to simply read, in the form\n"
-	       "     1/<fraction of pages to write>.\n"
-	       "     (default: 1 i.e. all pages are written to.)\n");
 	printf(" -v: specify the number of vCPUs to run.\n");
 	printf(" -o: Overlap guest memory accesses instead of partitioning\n"
 	       "     them into a separate region of memory for each vCPU.\n");
@@ -388,6 +394,11 @@ 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(" -w: specify the percentage of pages which should be written to\n"
+	       "     as an integer from 0-100 inclusive. This is probabalistic,\n"
+	       "     so -w X means each page has an X%% chance of writing\n"
+	       "     and a (100-X)%% chance of reading.\n"
+	       "     (default: 100 i.e. all pages are written to.)\n");
 	puts("");
 	exit(0);
 }
@@ -397,10 +408,10 @@ int main(int argc, char *argv[])
 	int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS);
 	struct test_params p = {
 		.iterations = TEST_HOST_LOOP_N,
-		.wr_fract = 1,
 		.partition_vcpu_memory_access = true,
 		.backing_src = DEFAULT_VM_MEM_SRC,
 		.slots = 1,
+		.write_percent = 100,
 	};
 	int opt;
 
@@ -411,7 +422,7 @@ int main(int argc, char *argv[])
 
 	guest_modes_append_default();
 
-	while ((opt = getopt(argc, argv, "eghi:p:m:nb:f:v:or:s:x:")) != -1) {
+	while ((opt = getopt(argc, argv, "eghi:p:m:nb:v:or:s:x:w:")) != -1) {
 		switch (opt) {
 		case 'e':
 			/* 'e' is for evil. */
@@ -434,10 +445,11 @@ int main(int argc, char *argv[])
 		case 'b':
 			guest_percpu_mem_size = parse_size(optarg);
 			break;
-		case 'f':
-			p.wr_fract = atoi(optarg);
-			TEST_ASSERT(p.wr_fract >= 1,
-				    "Write fraction cannot be less than one");
+		case 'w':
+			p.write_percent = atoi(optarg);
+			TEST_ASSERT(p.write_percent >= 0
+				    && p.write_percent <= 100,
+				    "Write percentage must be between 0 and 100");
 			break;
 		case 'v':
 			nr_vcpus = atoi(optarg);
diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h
index f1050fd42d10..845165001ec8 100644
--- a/tools/testing/selftests/kvm/include/perf_test_util.h
+++ b/tools/testing/selftests/kvm/include/perf_test_util.h
@@ -36,7 +36,7 @@ struct perf_test_args {
 	uint64_t size;
 	uint64_t guest_page_size;
 	uint32_t random_seed;
-	int wr_fract;
+	uint32_t write_percent;
 
 	/* Run vCPUs in L2 instead of L1, if the architecture supports it. */
 	bool nested;
@@ -52,7 +52,7 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int nr_vcpus,
 				   bool partition_vcpu_memory_access);
 void perf_test_destroy_vm(struct kvm_vm *vm);
 
-void perf_test_set_wr_fract(struct kvm_vm *vm, int wr_fract);
+void perf_test_set_write_percent(struct kvm_vm *vm, uint32_t write_percent);
 void perf_test_set_random_seed(struct kvm_vm *vm, uint32_t random_seed);
 
 void perf_test_start_vcpu_threads(int vcpus, void (*vcpu_fn)(struct perf_test_vcpu_args *));
diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c
index 5f0eebb626b5..97a402f5ed23 100644
--- a/tools/testing/selftests/kvm/lib/perf_test_util.c
+++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
@@ -61,7 +61,7 @@ void perf_test_guest_code(uint32_t vcpu_idx)
 		for (i = 0; i < pages; i++) {
 			uint64_t addr = gva + (i * pta->guest_page_size);
 
-			if (i % pta->wr_fract == 0)
+			if (guest_random_u32(&rand_state) % 100 < pta->write_percent)
 				*(uint64_t *)addr = 0x0123456789ABCDEF;
 			else
 				READ_ONCE(*(uint64_t *)addr);
@@ -122,7 +122,7 @@ struct kvm_vm *perf_test_create_vm(enum vm_guest_mode mode, int nr_vcpus,
 	pr_info("Testing guest mode: %s\n", vm_guest_mode_string(mode));
 
 	/* By default vCPUs will write to memory. */
-	pta->wr_fract = 1;
+	pta->write_percent = 100;
 
 	/*
 	 * Snapshot the non-huge page size.  This is used by the guest code to
@@ -224,10 +224,10 @@ void perf_test_destroy_vm(struct kvm_vm *vm)
 	kvm_vm_free(vm);
 }
 
-void perf_test_set_wr_fract(struct kvm_vm *vm, int wr_fract)
+void perf_test_set_write_percent(struct kvm_vm *vm, uint32_t write_percent)
 {
-	perf_test_args.wr_fract = wr_fract;
-	sync_global_to_guest(vm, perf_test_args);
+	perf_test_args.write_percent = write_percent;
+	sync_global_to_guest(vm, perf_test_args.write_percent);
 }
 
 void perf_test_set_random_seed(struct kvm_vm *vm, uint32_t random_seed)
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v7 3/3] KVM: selftests: randomize page access order

[Colton Lewis]

Create the ability to randomize page access order with the -a
argument. This includes the possibility that the same pages may be hit
multiple times during an iteration or not at all.

Population has random access as false to ensure all pages will be
touched by population and avoid page faults in late dirty memory that
would pollute the test results.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
Reviewed-by: Ricardo Koller <ricarkol@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
 tools/testing/selftests/kvm/dirty_log_perf_test.c | 12 ++++++++++--
 .../selftests/kvm/include/perf_test_util.h        |  2 ++
 tools/testing/selftests/kvm/lib/perf_test_util.c  | 15 ++++++++++++++-
 3 files changed, 26 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 0d0240041acf..e8855bd6f023 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -133,6 +133,7 @@ struct test_params {
 	int slots;
 	uint32_t write_percent;
 	uint32_t random_seed;
+	bool random_access;
 };
 
 static void toggle_dirty_logging(struct kvm_vm *vm, int slots, bool enable)
@@ -260,6 +261,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 	 * would pollute the performance results.
 	 */
 	perf_test_set_write_percent(vm, 100);
+	perf_test_set_random_access(vm, false);
 	perf_test_start_vcpu_threads(nr_vcpus, vcpu_worker);
 
 	/* Allow the vCPUs to populate memory */
@@ -282,6 +284,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 		ts_diff.tv_sec, ts_diff.tv_nsec);
 
 	perf_test_set_write_percent(vm, p->write_percent);
+	perf_test_set_random_access(vm, p->random_access);
 
 	while (iteration < p->iterations) {
 		/*
@@ -365,10 +368,11 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 static void help(char *name)
 {
 	puts("");
-	printf("usage: %s [-h] [-i iterations] [-p offset] [-g] "
+	printf("usage: %s [-h] [-a] [-i iterations] [-p offset] [-g] "
 	       "[-m mode] [-n] [-b vcpu bytes] [-v vcpus] [-o] [-r random seed ] [-s mem type]"
 	       "[-x memslots] [-w percentage]\n", name);
 	puts("");
+	printf(" -a: access memory randomly rather than in order.\n");
 	printf(" -i: specify iteration counts (default: %"PRIu64")\n",
 	       TEST_HOST_LOOP_N);
 	printf(" -g: Do not enable KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2. This\n"
@@ -422,11 +426,15 @@ int main(int argc, char *argv[])
 
 	guest_modes_append_default();
 
-	while ((opt = getopt(argc, argv, "eghi:p:m:nb:v:or:s:x:w:")) != -1) {
+	while ((opt = getopt(argc, argv, "aeghi:p:m:nb:v:or:s:x:w:")) != -1) {
 		switch (opt) {
+		case 'a':
+			p.random_access = true;
+			break;
 		case 'e':
 			/* 'e' is for evil. */
 			run_vcpus_while_disabling_dirty_logging = true;
+			break;
 		case 'g':
 			dirty_log_manual_caps = 0;
 			break;
diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h
index 845165001ec8..3d0b75ea866a 100644
--- a/tools/testing/selftests/kvm/include/perf_test_util.h
+++ b/tools/testing/selftests/kvm/include/perf_test_util.h
@@ -40,6 +40,7 @@ struct perf_test_args {
 
 	/* Run vCPUs in L2 instead of L1, if the architecture supports it. */
 	bool nested;
+	bool random_access;
 
 	struct perf_test_vcpu_args vcpu_args[KVM_MAX_VCPUS];
 };
@@ -54,6 +55,7 @@ void perf_test_destroy_vm(struct kvm_vm *vm);
 
 void perf_test_set_write_percent(struct kvm_vm *vm, uint32_t write_percent);
 void perf_test_set_random_seed(struct kvm_vm *vm, uint32_t random_seed);
+void perf_test_set_random_access(struct kvm_vm *vm, bool random_access);
 
 void perf_test_start_vcpu_threads(int vcpus, void (*vcpu_fn)(struct perf_test_vcpu_args *));
 void perf_test_join_vcpu_threads(int vcpus);
diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c
index 97a402f5ed23..a27405a590ba 100644
--- a/tools/testing/selftests/kvm/lib/perf_test_util.c
+++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
@@ -48,6 +48,8 @@ void perf_test_guest_code(uint32_t vcpu_idx)
 	struct perf_test_vcpu_args *vcpu_args = &pta->vcpu_args[vcpu_idx];
 	uint64_t gva;
 	uint64_t pages;
+	uint64_t addr;
+	uint64_t page;
 	int i;
 	struct guest_random_state rand_state = new_guest_random_state(pta->random_seed + vcpu_idx);
 
@@ -59,7 +61,12 @@ void perf_test_guest_code(uint32_t vcpu_idx)
 
 	while (true) {
 		for (i = 0; i < pages; i++) {
-			uint64_t addr = gva + (i * pta->guest_page_size);
+			if (pta->random_access)
+				page = guest_random_u32(&rand_state) % pages;
+			else
+				page = i;
+
+			addr = gva + (page * pta->guest_page_size);
 
 			if (guest_random_u32(&rand_state) % 100 < pta->write_percent)
 				*(uint64_t *)addr = 0x0123456789ABCDEF;
@@ -236,6 +243,12 @@ void perf_test_set_random_seed(struct kvm_vm *vm, uint32_t random_seed)
 	sync_global_to_guest(vm, perf_test_args.random_seed);
 }
 
+void perf_test_set_random_access(struct kvm_vm *vm, bool random_access)
+{
+	perf_test_args.random_access = random_access;
+	sync_global_to_guest(vm, perf_test_args.random_access);
+}
+
 uint64_t __weak perf_test_nested_pages(int nr_vcpus)
 {
 	return 0;
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH v7 1/3] KVM: selftests: implement random number generation for guest code

[Sean Christopherson]

On Wed, Oct 19, 2022, Colton Lewis wrote:
> Implement random number generation for guest code to randomize parts
> of the test, making it less predictable and a more accurate reflection
> of reality.
> 
> Create a -r argument to specify a random seed. If no argument is
> provided, the seed defaults to 0. The random seed is set with
> perf_test_set_random_seed() and must be set before guest_code runs to
> apply.
> 
> The random number generator chosen is the Park-Miller Linear
> Congruential Generator, a fancy name for a basic and well-understood
> random number generator entirely sufficient for this purpose. Each
> vCPU calculates its own seed by adding its index to the seed provided.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> Reviewed-by: Ricardo Koller <ricarkol@google.com>
> Reviewed-by: David Matlack <dmatlack@google.com>

This patch has changed a fair bit since David and Ricardo gave their reviews,
their Reviewed-by's should be dropped.  Alternatively, if a patch has is on the
fence so to speak, i.e. has changed a little bit but not thaaaaat much, you can
add something in the cover letter, e.g. "David/Ricardo, I kept your reviews, let
me know if that's not ok".  But in this case, I think the code has changed enough
that their reviews should be dropped.

> ---
>  .../testing/selftests/kvm/dirty_log_perf_test.c | 12 ++++++++++--
>  .../selftests/kvm/include/perf_test_util.h      |  2 ++
>  tools/testing/selftests/kvm/include/test_util.h |  7 +++++++
>  .../testing/selftests/kvm/lib/perf_test_util.c  |  7 +++++++
>  tools/testing/selftests/kvm/lib/test_util.c     | 17 +++++++++++++++++
>  5 files changed, 43 insertions(+), 2 deletions(-)

I think it makes sense to introduce "struct guest_random_state" separately from
the usage in perf_test_util and dirty_log_perf_test.  E.g. so that if we need to
revert the perf_test_util changes (extremely unlikely), we can do so without having
to wipe out the pRNG at the same time.  Or so that someone can pull in the pRNG to
their series without having to take a dependency on the other changes.

> diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
> index befc754ce9b3..9e4f36a1a8b0 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -152,4 +152,11 @@ static inline void *align_ptr_up(void *x, size_t size)
>  	return (void *)align_up((unsigned long)x, size);
>  }
>  
> +struct guest_random_state {
> +	uint32_t seed;
> +};
> +
> +struct guest_random_state new_guest_random_state(uint32_t seed);
> +uint32_t guest_random_u32(struct guest_random_state *state);
> +
>  #endif /* SELFTEST_KVM_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..5f0eebb626b5 100644
> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> @@ -49,6 +49,7 @@ void perf_test_guest_code(uint32_t vcpu_idx)
>  	uint64_t gva;
>  	uint64_t pages;
>  	int i;
> +	struct guest_random_state rand_state = new_guest_random_state(pta->random_seed + vcpu_idx);

lib/perf_test_util.c: In function ‘perf_test_guest_code’:
lib/perf_test_util.c:52:35: error: unused variable ‘rand_state’ [-Werror=unused-variable]
   52 |         struct guest_random_state rand_state = new_guest_random_state(pta->random_seed + vcpu_idx);
      |                                   ^~~~~~~~~~

This belongs in the next path.  I'd also prefer to split the declaration from the
initialization as this is an unnecessarily long line, e.g.

	struct perf_test_args *pta = &perf_test_args;
	struct perf_test_vcpu_args *vcpu_args = &pta->vcpu_args[vcpu_idx];
	struct guest_random_state rand_state;
	uint64_t gva;
	uint64_t pages;
	uint64_t addr;
	uint64_t page;
	int i;

	rand_state = new_guest_random_state(pta->random_seed + vcpu_idx);

Re: [PATCH v7 3/3] KVM: selftests: randomize page access order

[Sean Christopherson]

On Wed, Oct 19, 2022, Colton Lewis wrote:
> @@ -422,11 +426,15 @@ int main(int argc, char *argv[])
>  
>  	guest_modes_append_default();
>  
> -	while ((opt = getopt(argc, argv, "eghi:p:m:nb:v:or:s:x:w:")) != -1) {
> +	while ((opt = getopt(argc, argv, "aeghi:p:m:nb:v:or:s:x:w:")) != -1) {
>  		switch (opt) {
> +		case 'a':
> +			p.random_access = true;
> +			break;
>  		case 'e':
>  			/* 'e' is for evil. */
>  			run_vcpus_while_disabling_dirty_logging = true;
> +			break;

Heh, I appreciate the fix for my bug, but this belongs in a separate patch.
Isolating bug fixes isn't exactly critical for selftests, but it does matter.
E.g. Vipin also has an in-flight patch to fix this, and isolating the fix makes
it easier on maintainers to resolve the conflicts between the two series.

[*] https://lore.kernel.org/all/20221021211816.1525201-2-vipinsh@google.com

Re: [PATCH v7 0/3] KVM: selftests: randomize memory access of dirty_log_perf_test

[Sean Christopherson]

On Wed, Oct 19, 2022, Colton Lewis wrote:
> Add the ability to randomize parts of dirty_log_perf_test,
> specifically the order pages are accessed and whether pages are read
> or written.
> 
> v7:
> 
> Encapsulate the random state inside a struct. Add detail to names of
> those functions. Change interface so random function returns the next
> random number rather than using an out parameter.
> 
> Rebased to kvm/queue to ensure freshness.
> 
> Colton Lewis (3):
>   KVM: selftests: implement random number generation for guest code
>   KVM: selftests: randomize which pages are written vs read
>   KVM: selftests: randomize page access order

A few mechanical comments, but otherwise looks good.

Re: [PATCH v7 3/3] KVM: selftests: randomize page access order

[Colton Lewis]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Oct 19, 2022, Colton Lewis wrote:
>> @@ -422,11 +426,15 @@ int main(int argc, char *argv[])

>>   	guest_modes_append_default();

>> -	while ((opt = getopt(argc, argv, "eghi:p:m:nb:v:or:s:x:w:")) != -1) {
>> +	while ((opt = getopt(argc, argv, "aeghi:p:m:nb:v:or:s:x:w:")) != -1) {
>>   		switch (opt) {
>> +		case 'a':
>> +			p.random_access = true;
>> +			break;
>>   		case 'e':
>>   			/* 'e' is for evil. */
>>   			run_vcpus_while_disabling_dirty_logging = true;
>> +			break;

> Heh, I appreciate the fix for my bug, but this belongs in a separate  
> patch.
> Isolating bug fixes isn't exactly critical for selftests, but it does  
> matter.
> E.g. Vipin also has an in-flight patch to fix this, and isolating the fix  
> makes
> it easier on maintainers to resolve the conflicts between the two series.

> [*]  
> https://lore.kernel.org/all/20221021211816.1525201-2-vipinsh@google.com

Unintentional, I had a conflict there and assumed there was a break
there.

Will take it out.

Re: [PATCH v7 1/3] KVM: selftests: implement random number generation for guest code

[Colton Lewis]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Oct 19, 2022, Colton Lewis wrote:
>> Implement random number generation for guest code to randomize parts
>> of the test, making it less predictable and a more accurate reflection
>> of reality.

>> Create a -r argument to specify a random seed. If no argument is
>> provided, the seed defaults to 0. The random seed is set with
>> perf_test_set_random_seed() and must be set before guest_code runs to
>> apply.

>> The random number generator chosen is the Park-Miller Linear
>> Congruential Generator, a fancy name for a basic and well-understood
>> random number generator entirely sufficient for this purpose. Each
>> vCPU calculates its own seed by adding its index to the seed provided.

>> Signed-off-by: Colton Lewis <coltonlewis@google.com>
>> Reviewed-by: Ricardo Koller <ricarkol@google.com>
>> Reviewed-by: David Matlack <dmatlack@google.com>

> This patch has changed a fair bit since David and Ricardo gave their  
> reviews,
> their Reviewed-by's should be dropped.  Alternatively, if a patch has is  
> on the
> fence so to speak, i.e. has changed a little bit but not thaaaaat much,  
> you can
> add something in the cover letter, e.g. "David/Ricardo, I kept your  
> reviews, let
> me know if that's not ok".  But in this case, I think the code has  
> changed enough
> that their reviews should be dropped.


I talked to Ricardo privately and he thought it was ok to leave the
names in this borderline case. IMO, changing this interface doesn't
change anything important of what the patch is doing.

Nevertheless, I'll drop the names and ask them to reconfirm.

>> ---
>>   .../testing/selftests/kvm/dirty_log_perf_test.c | 12 ++++++++++--
>>   .../selftests/kvm/include/perf_test_util.h      |  2 ++
>>   tools/testing/selftests/kvm/include/test_util.h |  7 +++++++
>>   .../testing/selftests/kvm/lib/perf_test_util.c  |  7 +++++++
>>   tools/testing/selftests/kvm/lib/test_util.c     | 17 +++++++++++++++++
>>   5 files changed, 43 insertions(+), 2 deletions(-)

> I think it makes sense to introduce "struct guest_random_state"  
> separately from
> the usage in perf_test_util and dirty_log_perf_test.  E.g. so that if we  
> need to
> revert the perf_test_util changes (extremely unlikely), we can do so  
> without having
> to wipe out the pRNG at the same time.  Or so that someone can pull in  
> the pRNG to
> their series without having to take a dependency on the other changes.


Will do. Was attempting to avoid adding unused code in its own commit
according to
https://www.kernel.org/doc/html/latest/process/5.Posting.html

"Whenever possible, a patch which adds new code should make that code
active immediately."

>> diff --git a/tools/testing/selftests/kvm/include/test_util.h  
>> b/tools/testing/selftests/kvm/include/test_util.h
>> index befc754ce9b3..9e4f36a1a8b0 100644
>> --- a/tools/testing/selftests/kvm/include/test_util.h
>> +++ b/tools/testing/selftests/kvm/include/test_util.h
>> @@ -152,4 +152,11 @@ static inline void *align_ptr_up(void *x, size_t  
>> size)
>>   	return (void *)align_up((unsigned long)x, size);
>>   }

>> +struct guest_random_state {
>> +	uint32_t seed;
>> +};
>> +
>> +struct guest_random_state new_guest_random_state(uint32_t seed);
>> +uint32_t guest_random_u32(struct guest_random_state *state);
>> +
>>   #endif /* SELFTEST_KVM_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..5f0eebb626b5 100644
>> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
>> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
>> @@ -49,6 +49,7 @@ void perf_test_guest_code(uint32_t vcpu_idx)
>>   	uint64_t gva;
>>   	uint64_t pages;
>>   	int i;
>> +	struct guest_random_state rand_state =  
>> new_guest_random_state(pta->random_seed + vcpu_idx);

> lib/perf_test_util.c: In function ‘perf_test_guest_code’:
> lib/perf_test_util.c:52:35: error: unused variable ‘rand_state’  
> [-Werror=unused-variable]
>     52 |         struct guest_random_state rand_state =  
> new_guest_random_state(pta->random_seed + vcpu_idx);
>        |                                   ^~~~~~~~~~

> This belongs in the next path.  I'd also prefer to split the declaration  
> from the
> initialization as this is an unnecessarily long line, e.g.

Understood that this is implied by the previous comment. As for the line
length, checkpatch doesn't complain.

Re: [PATCH v7 1/3] KVM: selftests: implement random number generation for guest code

[Sean Christopherson]

On Thu, Oct 27, 2022, Colton Lewis wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Wed, Oct 19, 2022, Colton Lewis wrote:
> > > Signed-off-by: Colton Lewis <coltonlewis@google.com>
> > > Reviewed-by: Ricardo Koller <ricarkol@google.com>
> > > Reviewed-by: David Matlack <dmatlack@google.com>
> 
> > This patch has changed a fair bit since David and Ricardo gave their
> > reviews, their Reviewed-by's should be dropped.  Alternatively, if a patch
> > has is on the fence so to speak, i.e. has changed a little bit but not
> > thaaaaat much, you can add something in the cover letter, e.g.
> > "David/Ricardo, I kept your reviews, let me know if that's not ok".  But in
> > this case, I think the code has changed enough that their reviews should be
> > dropped.
> 
> 
> I talked to Ricardo privately and he thought it was ok to leave the
> names in this borderline case.

Heh, damned if you do, damned if you don't.  On a more serious note, this is why
I avoid doing off-list reviews except for the most basic sanity checks.  Ask two
people for their opinion and inevitably you'll get two different answers :-)
By asking and responding on-list, there's is (a) a paper trail and (b) a chance
for others to object before the decision is "finalized".

> IMO, changing this interface doesn't change anything important of what the
> patch is doing.

Right, but the code is different.  E.g. hypothetically, if you botched something
while reworking the code to fit the new interace, then it looks as if multiple
people gave a thumbs up to broken code, which can cause a maintainer to not give
the patch as much scrutiny as that might to a patch without reviews.

A recent example that's somewhat similar is commit 4293ddb788c1 ("KVM: x86/mmu:
Remove redundant spte present check in mmu_set_spte"), where a conflict during a
rebase of a relatively simple patch was mishandled and resulted in a buggy commit
with three Reviewed-by tags where none of the reviews were given for the buggy
code.  There's no guarantee the bug would have been caught if the Reviewed-by
tags had been dropped, but a "hey, I dropped your reviews from patch XYZ" likely
would have drawn eyeballs to the patch in question.

> Nevertheless, I'll drop the names and ask them to reconfirm.

FWIW, if you've confirmed offline that someone is ok keeping _their_ review, that's
totally fine, though throwing a comment in the cover letter is probably a good
idea in that case.  That's a decent rule of thumb in general; if a decision was
made off-list, make a note of it on-list to keep others in the loop.

> > I think it makes sense to introduce "struct guest_random_state" separately
> > from the usage in perf_test_util and dirty_log_perf_test.  E.g. so that if
> > we need to revert the perf_test_util changes (extremely unlikely), we can
> > do so without having to wipe out the pRNG at the same time.  Or so that
> > someone can pull in the pRNG to their series without having to take a
> > dependency on the other changes.
> 
> Will do. Was attempting to avoid adding unused code in its own commit
> according to
> https://www.kernel.org/doc/html/latest/process/5.Posting.html
> 
> "Whenever possible, a patch which adds new code should make that code
> active immediately."

Yeah, this is another "rule" that is sometimes subjective, e.g. in this case it
conflicts with the "one change per patch" rule.  Since the RNG code can't be made
completely active without pulling in yet more code (the guest_random_u32() usage
in the next path), the intended benefit of the "use immediately" rule isn't really
achieved, and so forcing the issue is a net negative.

> > > diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c
> > > b/tools/testing/selftests/kvm/lib/perf_test_util.c
> > > index 9618b37c66f7..5f0eebb626b5 100644
> > > --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> > > +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> > > @@ -49,6 +49,7 @@ void perf_test_guest_code(uint32_t vcpu_idx)
> > >   	uint64_t gva;
> > >   	uint64_t pages;
> > >   	int i;
> > > +	struct guest_random_state rand_state =
> > > new_guest_random_state(pta->random_seed + vcpu_idx);
> 
> > lib/perf_test_util.c: In function ‘perf_test_guest_code’:
> > lib/perf_test_util.c:52:35: error: unused variable ‘rand_state’
> > [-Werror=unused-variable]
> >     52 |         struct guest_random_state rand_state =
> > new_guest_random_state(pta->random_seed + vcpu_idx);
> >        |                                   ^~~~~~~~~~
> 
> > This belongs in the next path.  I'd also prefer to split the declaration
> > from the
> > initialization as this is an unnecessarily long line, e.g.
> 
> Understood that this is implied by the previous comment. As for the line
> length, checkpatch doesn't complain.

Checkpatch now only complains if the line length is >100, but there is still an 80
column "soft" limit that is preferred by most of the kernel, KVM included.
Checkpatch was relaxed because too many people were wrapping code in really weird
places "because checkpatch complained", but preferred style is still to say under
the soft limit.

In other words, stay under the soft limit unless the alternative sucks worse :-)