On Tue, Aug 30, 2022 at 07:01:57PM +0000, Colton Lewis wrote: > 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. You might want to reconsider the idea of using a random number generator inside the guest. IRC the reasons against it were: quality of the random numbers, and that some random generators use floating-point numbers. I don't think the first one is a big issue. The second one might be an issue if we want to generate non-uniform distributions (e.g., poisson); but not a problem for now. > > > 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.