Re: [RFC PATCH v3 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 'dirty memory time' metric incorrectly includes the time vCPU threads spend waiting for the global `collect` mutex, misattributing it to guest execution time and underreporting dirty ring management overhead.
- [Medium] Lockless synchronization between the vCPU threads and the main thread lacks memory barriers, creating a data race on the `vcpu_dirty_ring_collect` timespec payload.
--

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

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..181f521c9be96 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 timer placement incorrectly measure performance metrics?

Because clock_gettime() is called after acquiring the collect mutex, the
returned ts will not include any time the vCPU thread spends waiting for the
lock.

Later in vcpu_worker():

	ts_diff = timespec_elapsed(start);

	if (use_dirty_ring) {
		ts_diff = timespec_sub(ts_diff, collect);
	}

Since collect excludes lock contention time, subtracting it leaves that
contention overhead in ts_diff. Does this result in misattributing lock
contention to guest execution time?

> +
> +	while (true) {
> +		struct kvm_dirty_gfn *cur;

[ ... ]

> @@ -43,17 +88,33 @@ 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 (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;
> +		}
> +
>  		TEST_ASSERT(ret == 0, "vcpu_run failed: %d", ret);
>  		TEST_ASSERT(get_ucall(vcpu, NULL) == UCALL_SYNC,
>  			    "Invalid guest sync status: exit_reason=%s",
>  			    exit_reason_str(run->exit_reason));
>  
>  		pr_debug("Got sync event from vCPU %d\n", vcpu_idx);
>  		vcpu_last_completed_iteration[vcpu_idx] = current_iteration;

[Severity: Medium]
Are memory barriers needed for this lockless synchronization?

The worker thread updates vcpu_dirty_ring_collect and then sets
vcpu_last_completed_iteration to signal completion. However, this is done
without store-release semantics.

Meanwhile, the main thread in run_test() spins on the completion flag and
then reads the array:

	for (i = 0; i < nr_vcpus; i++) {
		while (READ_ONCE(vcpu_last_completed_iteration[i]) != iteration)
			;
	}
	
	/* ... */
	
	if (dirty_ring_size) {
		for (i = 0; i < nr_vcpus; i++)
			iteration_sum = timespec_add(iteration_sum,
						     vcpu_dirty_ring_collect[i]);
	}

Could weakly-ordered architectures like ARM64 reorder the worker thread's
stores (or the main thread's loads), causing the main thread to read torn or
stale struct timespec data?

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