Re: [RFC PATCH 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 4 potential issue(s) to consider:
- [Medium] dirty_ring_collect() computes array indices using the dirty ring size in bytes rather than the number of elements, causing a severe out-of-bounds memory access.
- [Medium] The do-while loop in vcpu_worker() terminates prematurely because it incorrectly checks the _vcpu_run() return value instead of the KVM exit reason, artificially shortening the workload.
- [Medium] Concurrent calls to kvm_vm_reset_dirty_ring() by multiple vCPU threads cause a TEST_ASSERT failure because the VM-wide reset count is compared against a thread-local harvested count.
- [Medium] The elapsed time calculation for clearing the dirty ring incorrectly aggregates parallel CPU time instead of serial wall-clock time, causing an unfair comparison with the dirty-bitmap mechanism.
--

commit 70a79bc8da1cc1134a0b9d174c9a32912039dc85
Author: Leonardo Bras <leo.bras@arm.com>
Subject: KVM: selftests: dirty_log_perf_test: Add dirty-ring support

dirty_log_test supports both dirty-bitmap and dirty-ring as dirty-page
tracking mechanisms, while dirty_log_perf_test only supports dirty-bitmap.
Add support to dirty-ring on dirty_log_perf_test so it can be used to
compare performance between changes in the mechanism.

> diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> index 69b38791440e7..659efa679bc74 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
[ ... ]
> @@ -29,9 +30,43 @@ 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;
> +	u32 ring_size = vcpu->vm->dirty_ring_size;
> +
> +	clock_gettime(CLOCK_MONOTONIC, &start);
> +
> +	while (true) {
> +		struct kvm_dirty_gfn *cur;
> +
> +		cur = &dirty_gfns[idx % ring_size];

[Severity: Medium]
Since the ring size is configured in bytes (e.g., in main() via
dirty_ring_size *= sizeof(struct kvm_dirty_gfn)), does using it here as a
modulo for indexing into an array of structs cause out-of-bounds memory
accesses?

> +		if (smp_load_acquire(&cur->flags) != KVM_DIRTY_GFN_F_DIRTY)
> +			break;
> +
> +		smp_store_release(&cur->flags, KVM_DIRTY_GFN_F_RESET);
> +		idx++;
> +	}
> +
> +	idx -= *ring_idx;
> +	*ring_idx += idx;
> +
> +	ret = kvm_vm_reset_dirty_ring(vcpu->vm);
> +
> +	TEST_ASSERT(ret == idx, "Reset dirty pages (%u) mismatch "
> +		    "with collected (%u)", ret, idx);

[Severity: Medium]
Will this assertion falsely fail under concurrency?

The kvm_vm_reset_dirty_ring() call performs a VM-wide reset of the dirty
rings for all vCPUs.

In a multi-vCPU test, is it possible for one thread's ioctl call to reap
reset entries prepared by other threads, resulting in a return value greater
than its local idx count?

> +
> +	*ts = timespec_add(*ts, timespec_elapsed(start));
> +}
>  
>  static void vcpu_worker(struct memstress_vcpu_args *vcpu_args)
>  {
[ ... ]
> @@ -43,19 +78,36 @@ 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 == KVM_EXIT_DIRTY_RING_FULL);

[Severity: Medium]
Does this loop terminate prematurely because it compares the return value of
_vcpu_run() instead of the KVM exit reason?

_vcpu_run() returns the ioctl status (0 on success), not the exit reason.

This means the condition evaluates to while (0 == KVM_EXIT_DIRTY_RING_FULL)
and will exit after the very first dirty ring fill.

> +
>  		ts_diff = timespec_elapsed(start);
>  
> +		if (use_dirty_ring) {
> +			ts_diff = timespec_sub(ts_diff, collect);
> +			vcpu_dirty_ring_collect[vcpu_idx] = collect;
> +		}
> +
>  		TEST_ASSERT(ret == 0, "vcpu_run failed: %d", ret);
[ ... ]
> @@ -195,6 +250,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);

[Severity: Medium]
Does aggregating the parallel execution times of all vCPU threads into
iteration_sum artificially inflate the reported dirty-ring clearing time?

The dirty-bitmap mechanism measures sequential wall-clock time in the main
thread. By summing parallel CPU times here (which scales linearly with the
number of vCPUs), it seems this will produce misleading performance metrics
when comparing the two mechanisms.

> +
> +			clear_dirty_log_total = timespec_add(clear_dirty_log_total,
> +							     iteration_sum);
> +			continue;
> +		}

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