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

[Sean Christopherson <seanjc@google.com>]

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