Re: [RFC PATCH v4 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] Lock contention wait time is incorrectly attributed to guest execution time instead of dirty ring clearing time.
--

--- Patch [3]: [RFC PATCH v4 3/3] KVM: selftests: dirty_log_perf_test: Add dirty-ring support ---
commit 314e5490e434eab524cfacd4f66a4c83a152147e
Author: Leonardo Bras <leo.bras@arm.com>

KVM: selftests: dirty_log_perf_test: Add dirty-ring support
    
This commit adds dirty-ring tracking support to dirty_log_perf_test.
This allows comparing the performance of the dirty-ring mechanism against
the existing dirty-bitmap implementation.

> diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> index 69b38791440e7..6e80d36cc61a3 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]
Will this clock_gettime() placement exclude the mutex lock contention time
from the collected clearing metric?

By taking the start time after acquiring the mutex, any time spent waiting
for the lock will not be included in the 'collect' time.

[ ... ]
> @@ -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);
> +			vcpu_dirty_ring_collect[vcpu_idx] = collect;
> +		}

[Severity: Medium]
Does subtracting this incomplete 'collect' time strand the lock contention time
inside the guest execution time?

Since the lock contention time is excluded from 'collect' in
dirty_ring_collect(), subtracting it from the total 'ts_diff' leaves the
contention time attributed to the guest execution time rather than the
dirty ring clearing time.

Could this artificially inflate the guest execution metric when multiple
vCPUs contend on the lock?

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