Re: [RFC PATCH v2 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 2 potential issue(s) to consider:
- [Medium] The return value of `kvm_vm_reset_dirty_ring` is checked for `< 0` using an unsigned integer, which silently masks ioctl failures.
- [Medium] Incompatible and misleading performance metric aggregation for dirty-ring vs. dirty-bitmap.
--

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

KVM: selftests: dirty_log_perf_test: Add dirty-ring support

This commit adds dirty-ring support to dirty_log_perf_test, allowing it to
be used alongside dirty-bitmap for performance comparisons.

> diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> index 69b38791440e7..9bb14340bff5a 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
[ ... ]
> @@ -29,9 +30,41 @@ 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)
> +{
> +	struct timespec start;
> +	struct kvm_dirty_gfn *dirty_gfns = vcpu_map_dirty_ring(vcpu);
> +	u32 ret, idx = *ring_idx;

[ ... ]

> +	*ring_idx = idx;
> +
> +	ret = kvm_vm_reset_dirty_ring(vcpu->vm);
> +
> +	TEST_ASSERT(ret >= 0, "Error (%d) found when cleaning dirty-ring\n", ret);

[Severity: Medium]
Will this TEST_ASSERT ever fail? The 'ret' variable is declared as a u32 above,
so the condition 'ret >= 0' will always be true, masking negative error codes
if kvm_vm_reset_dirty_ring() fails.

> +
> +	*ts = timespec_add(*ts, timespec_elapsed(start));
> +}

[ ... ]

> @@ -195,6 +247,21 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>  		pr_info("Iteration %d dirty memory time: %ld.%.9lds\n",
>  			iteration, ts_diff.tv_sec, ts_diff.tv_nsec);
>  
> +		if (dirty_ring_size) {
> +			struct timespec iteration_sum = (struct timespec){0};
> +
> +			for (i = 0; i < nr_vcpus; i++)
> +				iteration_sum = timespec_add(iteration_sum,
> +							     vcpu_dirty_ring_collect[i]);
> +
> +			pr_info("Iteration %d clear dirty ring time: %ld.%.9lds\n",
> +				iteration, iteration_sum.tv_sec, iteration_sum.tv_nsec);
> +
> +			clear_dirty_log_total = timespec_add(clear_dirty_log_total,
> +							     iteration_sum);

[Severity: Medium]
Does summing concurrent vCPU times here produce an accurate comparison against
the single-threaded wall-clock metric used for the dirty-bitmap?

Since vcpu_dirty_ring_collect[] measures time collected concurrently across
vCPUs, linearly adding them inflates 'clear dirty ring time' compared to
the 'clear dirty log time' which seems to just measure the elapsed time
on the main thread.

> +			continue;
> +		}

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