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

[Leonardo Bras <leo.bras@arm.com>]

On Wed, Jul 29, 2026 at 04:26:05PM +0000, sashiko-bot@kernel.org wrote:
> 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.

Yes, that's supposed to measure the time spent on cleaning the dirty-bits, 
not the time waiting for possible contention, as contention is not 
predictable and would affect the perception of improvements in 
dirty-cleaning.

> 
> [ ... ]
> > @@ -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?

Yes, but that's the tradeoff think works best.
Alternativelly, we could:  
- Get the lock inside the cleaning time, which would cause contention to be 
  accounted as cleaning, which is terrible for getting reproducible 
  results, or
- Exclude the contention time from both cleaning and guest_execution, which 
  would make code more complex, possibly adding a third time log 
  "contention"

I could swich to the second approach if anybody cares about the inflation 
of guest execution times, and don't care if time spent is not accounted.

Thanks!
Leo