Re: [PATCH v2 1/3] KVM: selftests: Create source of randomness for guest code.

[Ricardo Koller <ricarkol@google.com>]

On Tue, Aug 30, 2022 at 07:01:57PM +0000, Colton Lewis wrote:
> David Matlack <dmatlack@google.com> 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.