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

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

On Tue, Sep 29, 2026 at 11:53:46AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] kvm: selftests: dirty ring collection time excludes lock wait time
> --
> 
> commit 2c539bfee74e383775f9c7f30033144876ad2f45
> Author: Leonardo Bras <leo.bras@arm.com>
> 
> KVM: selftests: dirty_log_perf_test: Add dirty-ring support
> 
> This commit adds dirty-ring support to the dirty_log_perf_test to enable
> performance comparisons between dirty-bitmap and dirty-ring tracking
> mechanisms.
> 
> > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> > index 8f791ad7b86a8..af0ab654d46e2 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]
> Since the timer is initialized after acquiring the lock, the time spent
> waiting for the mutex is not recorded in the collection time. When multiple
> vCPUs fill their dirty rings concurrently and wait on this lock, will this
> lock contention time be incorrectly omitted from collect?

Yes, but that was a design choice.

> 
> > +
> > +	while (true) {
> > +		struct kvm_dirty_gfn *cur;
> [ ... ]
> > @@ -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);
> 
> [Severity: Medium]
> Because collect does not include the lock wait time from
> dirty_ring_collect(), subtracting it from ts_diff leaves the lock
> contention overhead misattributed to the guest execution time. Doesn't this
> artificially inflate the reported guest execution time and obscure the true
> overhead of dirty ring synchronization?
> 

But that's what's going to happen in any VMM.
Also, doing otherwise would fakely account for the cleaning part, which is 
what we are trying to measure.

Thanks!
Leo