Re: [PATCH 16/20] KVM: selftests: Ensure guest writes min number of pages in dirty_log_test

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Ensure the vCPU fully completes at least one write in each dirty_log_test
> iteration, as failure to dirty any pages complicates verification and
> forces the test to be overly conservative about possible values.  E.g.
> verification needs to allow the last dirty page from a previous iteration
> to have *any* value, because the vCPU could get stuck for multiple
> iterations, which is unlikely but can happen in heavily overloaded and/or
> nested virtualization setups.
> 
> Somewhat arbitrarily set the minimum to 0x100/256; high enough to be
> interesting, but not so high as to lead to pointlessly long runtimes.
> 
> Reported-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  tools/testing/selftests/kvm/dirty_log_test.c | 30 ++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 500257b712e3..8eb51597f762 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -37,6 +37,12 @@
>  /* Interval for each host loop (ms) */
>  #define TEST_HOST_LOOP_INTERVAL		10UL
>  
> +/*
> + * Ensure the vCPU is able to perform a reasonable number of writes in each
> + * iteration to provide a lower bound on coverage.
> + */
> +#define TEST_MIN_WRITES_PER_ITERATION	0x100
> +
>  /* Dirty bitmaps are always little endian, so we need to swap on big endian */
>  #if defined(__s390x__)
>  # define BITOP_LE_SWIZZLE	((BITS_PER_LONG-1) & ~0x7)
> @@ -72,6 +78,7 @@ static uint64_t host_page_size;
>  static uint64_t guest_page_size;
>  static uint64_t guest_num_pages;
>  static uint64_t iteration;
> +static uint64_t nr_writes;
>  static bool vcpu_stop;
>  
>  /*
> @@ -107,6 +114,7 @@ static void guest_code(void)
>  	for (i = 0; i < guest_num_pages; i++) {
>  		addr = guest_test_virt_mem + i * guest_page_size;
>  		vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration));
> +		nr_writes++;
>  	}
>  #endif
>  
> @@ -118,6 +126,7 @@ static void guest_code(void)
>  			addr = align_down(addr, host_page_size);
>  
>  			vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration));
> +			nr_writes++;
>  		}
>  
>  		GUEST_SYNC(1);
> @@ -665,6 +674,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>  	host_dirty_count = 0;
>  	host_clear_count = 0;
>  	WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
> +	WRITE_ONCE(nr_writes, 0);
> +	sync_global_to_guest(vm, nr_writes);
>  
>  	/*
>  	 * Ensure the previous iteration didn't leave a dangling semaphore, i.e.
> @@ -683,10 +694,22 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>  
>  		dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
>  
> -		/* Give the vcpu thread some time to dirty some pages */
> -		for (i = 0; i < p->interval; i++) {
> +		/*
> +		 * Let the vCPU run beyond the configured interval until it has
> +		 * performed the minimum number of writes.  This verifies the
> +		 * guest is making forward progress, e.g. isn't stuck because
> +		 * of a KVM bug, and puts a firm floor on test coverage.
> +		 */
> +		for (i = 0; i < p->interval || nr_writes < TEST_MIN_WRITES_PER_ITERATION; i++) {
> +			/*
> +			 * Sleep in 1ms chunks to keep the interval math simple
> +			 * and so that the test doesn't run too far beyond the
> +			 * specified interval.
> +			 */
>  			usleep(1000);
>  
> +			sync_global_from_guest(vm, nr_writes);
> +
>  			/*
>  			 * Reap dirty pages while the guest is running so that
>  			 * dirty ring full events are resolved, i.e. so that a
> @@ -760,6 +783,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>  			WRITE_ONCE(host_quit, true);
>  		sync_global_to_guest(vm, iteration);
>  
> +		WRITE_ONCE(nr_writes, 0);
> +		sync_global_to_guest(vm, nr_writes);
> +
>  		WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
>  
>  		sem_post(&sem_vcpu_cont);

This makes sense.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky