Re: [PATCH v1 3/3] KVM: selftests: dirty_log_perf_test: Add dirty-ring support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: selftests: dirty ring collection time excludes lock wait time
--

commit 2c539bfee74e383775f9c7f30033144876ad2f45
Author: Leonardo Bras <leo.bras@arm.com>

KVM: selftests: dirty_log_perf_test: Add dirty-ring support

This commit adds dirty-ring support to the dirty_log_perf_test to enable
performance comparisons between dirty-bitmap and dirty-ring tracking
mechanisms.

> diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> index 8f791ad7b86a8..af0ab654d46e2 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
[ ... ]
> @@ -29,9 +30,53 @@ static bool run_vcpus_while_disabling_dirty_logging;
>  
>  /* Host variables */
>  static u64 dirty_log_manual_caps;
> +static u32 dirty_ring_size;
>  static bool host_quit;
>  static int iteration;
>  static int vcpu_last_completed_iteration[KVM_MAX_VCPUS];
> +static struct timespec vcpu_dirty_ring_collect[KVM_MAX_VCPUS];
> +
> +static void dirty_ring_collect(struct kvm_vcpu *vcpu, u32 *ring_idx,
> +				struct timespec *ts)
> +{
> +	static pthread_mutex_t collect = PTHREAD_MUTEX_INITIALIZER;
> +	struct timespec start;
> +	struct kvm_dirty_gfn *dirty_gfns = vcpu_map_dirty_ring(vcpu);
> +	u32 idx = *ring_idx;
> +	u32 ring_size = vcpu->vm->dirty_ring_size / sizeof(struct kvm_dirty_gfn);
> +	int cleared, count;
> +
> +	pthread_mutex_lock(&collect);
> +
> +	clock_gettime(CLOCK_MONOTONIC, &start);

[Severity: Medium]
Since the timer is initialized after acquiring the lock, the time spent
waiting for the mutex is not recorded in the collection time. When multiple
vCPUs fill their dirty rings concurrently and wait on this lock, will this
lock contention time be incorrectly omitted from collect?

> +
> +	while (true) {
> +		struct kvm_dirty_gfn *cur;
[ ... ]
> @@ -43,24 +88,45 @@ static void vcpu_worker(struct memstress_vcpu_args *vcpu_args)
>  	struct timespec ts_diff;
>  	struct timespec total = (struct timespec){0};
>  	struct timespec avg;
> +	bool use_dirty_ring = !!vcpu->vm->dirty_ring_size;
> +	u32 ring_idx = 0;
>  	int ret;
>  
>  	run = vcpu->run;
>  
>  	while (!READ_ONCE(host_quit)) {
>  		int current_iteration = READ_ONCE(iteration);
> +		struct timespec collect = (struct timespec){0};
>  
>  		clock_gettime(CLOCK_MONOTONIC, &start);
> -		ret = _vcpu_run(vcpu);
> +
> +		do {
> +			ret = _vcpu_run(vcpu);
> +			if (!use_dirty_ring)
> +				break;
> +
> +			dirty_ring_collect(vcpu, &ring_idx, &collect);
> +		} while (!ret && run->exit_reason == KVM_EXIT_DIRTY_RING_FULL);
> +
>  		ts_diff = timespec_elapsed(start);
>  
> +		if (use_dirty_ring) {
> +			ts_diff = timespec_sub(ts_diff, collect);

[Severity: Medium]
Because collect does not include the lock wait time from
dirty_ring_collect(), subtracting it from ts_diff leaves the lock
contention overhead misattributed to the guest execution time. Doesn't this
artificially inflate the reported guest execution time and obscure the true
overhead of dirty ring synchronization?

> +			vcpu_dirty_ring_collect[vcpu_idx] = collect;
> +		}
> +
>  		TEST_ASSERT(ret == 0, "vcpu_run failed: %d", ret);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929113711.2064390-1-leo.bras@arm.com?part=3