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

[Sean Christopherson <seanjc@google.com>]

On Mon, Sep 12, 2022, Colton Lewis wrote:
> @@ -57,7 +58,17 @@ void perf_test_guest_code(uint32_t vcpu_id)
>  
>  	while (true) {
>  		for (i = 0; i < pages; i++) {
> -			uint64_t addr = gva + (i * pta->guest_page_size);
> +			guest_random(&rand);
> +
> +			if (pta->random_access)
> +				addr = gva + ((rand % pages) * pta->guest_page_size);

Shouldn't this use a 64-bit random number since "pages" is a 64-bit value?  Ha!
And another case where the RNG APIs can help, e.g.

  uint64_t __random_u64(struct ksft_pseudo_rng *rng, uint64_t max);

or maybe avoid naming pain and go straight to:


  uint64_t __random_u64(struct ksft_pseudo_rng *rng, uint64_t min, uint64_t max);

> +			else
> +				addr = gva + (i * pta->guest_page_size);

Since the calculation is the same, only the page index changes, I think it makes
sense to write this as:

			uint64_t idx = i;

			if (pta->random_access)
				idx = __random_u64(rng, 0, pages);

			addr = gva + (idx * pta->guest_page_size);

That way it's easy to introduce other access patterns.

> +
> +			/*
> +			 * Use a new random number here so read/write
> +			 * is not tied to the address used.
> +			 */
>  			guest_random(&rand);

Ya, I'm trippling (quadrupling?) down on my suggestion to improve the APIs.  Users
should not be able to screw up like this, i.e. shouldn't need comments to warn
readers, and adding another call to get a random number shouldn't affect unrelated
code.