David Matlack <dmatlack@xxxxxxxxxx> writes:
On Wed, Aug 17, 2022 at 09:41:44PM +0000, Colton Lewis wrote:
Create for each vcpu an array of random numbers to be used as a source
of randomness when executing guest code. Randomness should be useful
for approaching more realistic conditions in dirty_log_perf_test.
Create a -r argument to specify a random seed. If no argument
is provided, the seed defaults to the current Unix timestamp.
The random arrays are allocated and filled as part of VM creation. The
additional memory used is one 32-bit number per guest VM page to
ensure one number for each iteration of the inner loop of guest_code.
nit: I find it helpful to put this in more practical terms as well. e.g.
The random arrays are allocated and filled as part of VM creation. The
additional memory used is one 32-bit number per guest VM page (so
1MiB of additional memory when using the default 1GiB per-vCPU region
size) to ensure one number for each iteration of the inner loop of
guest_code.
Speaking of, this commit always allocates the random arrays, even if
they are not used. I think that's probably fine but it deserves to be
called out in the commit description with an explanation of why it is
the way it is.
I'll add a concrete example and explain always allocating.
Technically, the random array will always be accessed even if it never
changes the code path when write_percent = 0 or write_percent =
100. Always allocating avoids extra code that adds complexity for no
benefit.
@@ -442,6 +446,9 @@ int main(int argc, char *argv[])
case 'o':
p.partition_vcpu_memory_access = false;
break;
+ case 'r':
+ p.random_seed = atoi(optarg);
Putting the random seed in test_params seems unnecessary. This flag
could just set perf_test_args.randome_seed directly. Doing this would
avoid the need for duplicating the time(NULL) initialization, and allow
you to drop perf_test_set_random_seed().
I understand there is some redundancy in this approach and had
originally done what you suggest. My reason for changing was based on
the consideration that perf_test_util.c is library code that is used for
other tests and could be used for more in the future, so it should
always initialize everything in perf_test_args rather than rely on the
test to do it. This is what is done for wr_fract (renamed write_percent
later in this patch). perf_test_set_random_seed is there for interface
consistency with the other perf_test_set* functions.
But per the point below, moving random generation to VM creation means
we are dependent on the seed being set before then. So I do need a
change here, but I'd rather keep the redundancy and
perf_test_set_random_seed.
+void perf_test_fill_random_arrays(uint32_t nr_vcpus, uint32_t
nr_randoms)
+{
+ struct perf_test_args *pta = &perf_test_args;
+ uint32_t *host_row;
"host_row" is a confusing variable name here since you are no longer
operating on a 2D array. Each vCPU has it's own array.
Agreed.
@@ -120,8 +149,9 @@ 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;
+ /* Set perf_test_args defaults. */
+ pta->wr_fract = 100;
+ pta->random_seed = time(NULL);
Won't this override write the random_seed provided by the test?
Yes. I had thought I accounted for that by setting random_seed later in
run_test, but now realize that has no effect because I moved the
generation of all the random numbers to happen before then.
/*
* Snapshot the non-huge page size. This is used by the guest code to
@@ -211,6 +241,11 @@ struct kvm_vm *perf_test_create_vm(enum
vm_guest_mode mode, int nr_vcpus,
ucall_init(vm, NULL);
+ srandom(perf_test_args.random_seed);
+ pr_info("Random seed: %d\n", perf_test_args.random_seed);
+ perf_test_alloc_random_arrays(nr_vcpus, vcpu_memory_bytes >>
vm->page_shift);
+ perf_test_fill_random_arrays(nr_vcpus, vcpu_memory_bytes >>
vm->page_shift);
I think this is broken if !partition_vcpu_memory_access. nr_randoms
(per-vCPU) should be `nr_vcpus * vcpu_memory_bytes >> vm->page_shift`.
Agree it will break then and should not. But allocating that many more
random numbers may eat too much memory. In a test with 64 vcpus, it would
try to allocate 64x64 times as many random numbers. I'll try it but may
need something different in that case.
Speaking of, this should probably just be done in
perf_test_setup_vcpus(). That way you can just use vcpu_args->pages for
nr_randoms, and you don't have to add another for-each-vcpu loop.
Agreed.
+
/* Export the shared variables to the guest. */
sync_global_to_guest(vm, perf_test_args);
@@ -229,6 +264,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);
sync_global_to_guest() is unnecessary here since the guest does not need
to access perf_test_args.random_seed (and I can't imagine it ever will).
That being said, I think you can drop this function (see earlier
comment).
See earlier response about consistency.
