Re: [PATCH 2/3] KVM: selftests: Randomize which pages are written vs read

[David Matlack <dmatlack@google.com>]

On Wed, Aug 10, 2022 at 05:58:29PM +0000, Colton Lewis wrote:
> Randomize which pages are written vs read by using the random number

Same thing here about stating what the patch does first.

> table for each page modulo 100. This changes how the -w argument

s/-f/-w/

Although I would love it if you renamed -f to -w in the code instead.

> works. It is now a percentage from 0 to 100 inclusive that represents
> what percentage of accesses are writes. It keeps the same default of
> 100 percent writes.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
>  tools/testing/selftests/kvm/dirty_log_perf_test.c | 12 +++++++-----

access_tracking_perf_test.c also uses wr_fract and will need to be
updated.

>  tools/testing/selftests/kvm/lib/perf_test_util.c  |  4 ++--
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> index 80a1cbe7fbb0..dcc5d44fc757 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> @@ -381,8 +381,8 @@ static void help(char *name)
>  	       "     (default: 1G)\n");
>  	printf(" -f: specify the fraction of pages which should be written to\n"

s/fraction/percentage/

>  	       "     as opposed to simply read, in the form\n"
> -	       "     1/<fraction of pages to write>.\n"
> -	       "     (default: 1 i.e. all pages are written to.)\n");
> +	       "     [0-100]%% of pages to write.\n"
> +	       "     (default: 100 i.e. all pages are written to.)\n");

Mention that the implementation is probabilistic.

>  	printf(" -v: specify the number of vCPUs to run.\n");
>  	printf(" -o: Overlap guest memory accesses instead of partitioning\n"
>  	       "     them into a separate region of memory for each vCPU.\n");
> @@ -399,7 +399,7 @@ int main(int argc, char *argv[])
>  	int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS);
>  	struct test_params p = {
>  		.iterations = TEST_HOST_LOOP_N,
> -		.wr_fract = 1,
> +		.wr_fract = 100,

Please rename wr_fract to e.g. write_percent to reflect the new
semantics. Same goes for perf_test_set_wr_fract(),
perf_test_args.wr_fract, etc.

>  		.partition_vcpu_memory_access = true,
>  		.backing_src = DEFAULT_VM_MEM_SRC,
>  		.slots = 1,
> @@ -439,8 +439,10 @@ int main(int argc, char *argv[])
>  			break;
>  		case 'f':
>  			p.wr_fract = atoi(optarg);
> -			TEST_ASSERT(p.wr_fract >= 1,
> -				    "Write fraction cannot be less than one");
> +			TEST_ASSERT(p.wr_fract >= 0,
> +				    "Write fraction cannot be less than 0");
> +			TEST_ASSERT(p.wr_fract <= 100,
> +				    "Write fraction cannot be greater than 100");

nit: This could be combined into a single assert pretty easily.

>  			break;
>  		case 'v':
>  			nr_vcpus = atoi(optarg);
> diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c
> index b04e8d2c0f37..3c7b93349fef 100644
> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> @@ -64,7 +64,7 @@ void perf_test_guest_code(uint32_t vcpu_idx)
>  		for (i = 0; i < pages; i++) {
>  			uint64_t addr = gva + (i * pta->guest_page_size);
>  
> -			if (i % pta->wr_fract == 0)
> +			if (random_table[vcpu_idx][i] % 100 < pta->wr_fract)
>  				*(uint64_t *)addr = 0x0123456789ABCDEF;
>  			else
>  				READ_ONCE(*(uint64_t *)addr);
> @@ -168,7 +168,7 @@ 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;
> +	pta->wr_fract = 100;
>  
>  	/*
>  	 * Snapshot the non-huge page size.  This is used by the guest code to
> -- 
> 2.37.1.559.g78731f0fdb-goog
>