Re: [PATCH 2/3] KVM: selftests: Collect memory access latency samples

[Sean Christopherson <seanjc@google.com>]

On Thu, Jan 26, 2023, Colton Lewis wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > Maybe s/count/time?  Yeah, it's technically wrong to call it "time", but
> > "count" is too generic.
> 
> I could say "cycles".

Works for me.

> > > > +	uint32_t maybe_sample;
> > > >
> > > >  	gva = vcpu_args->gva;
> > > >  	pages = vcpu_args->pages;
> > > > @@ -75,10 +94,21 @@ void perf_test_guest_code(uint32_t vcpu_idx)
> > > >
> > > >  			addr = gva + (page * pta->guest_page_size);
> > > >
> > > > -			if (guest_random_u32(&rand_state) % 100 < pta->write_percent)
> > > > +			if (guest_random_u32(&rand_state) % 100 < pta->write_percent) {
> > > > +				count_before = perf_test_timer_read();
> > > >  				*(uint64_t *)addr = 0x0123456789ABCDEF;
> > > > -			else
> > > > +				count_after = perf_test_timer_read();
> > > > +			} else {
> > > > +				count_before = perf_test_timer_read();
> > > >  				READ_ONCE(*(uint64_t *)addr);
> > > > +				count_after = perf_test_timer_read();
> 
> > > "count_before ... ACCESS count_after" could be moved to some macro,
> > > e.g.,:
> > > 	t = MEASURE(READ_ONCE(*(uint64_t *)addr));
> 
> > Even better, capture the read vs. write in a local variable to
> > self-document the
> > use of the RNG, then the motivation for reading the system counter
> > inside the
> > if/else-statements goes away.  That way we don't need to come up with a
> > name
> > that documents what MEASURE() measures.
> 
> > 			write = guest_random_u32(&rand_state) % 100 < args->write_percent;
> 
> > 			time_before = guest_system_counter_read();
> > 			if (write)
> > 				*(uint64_t *)addr = 0x0123456789ABCDEF;
> > 			else
> > 				READ_ONCE(*(uint64_t *)addr);
> > 			time_after = guest_system_counter_read();
> 
> Couldn't timing before and after the if statement produce bad
> measurements? We might be including a branch mispredict in our memory
> access latency and this could happen a lot because it's random so no way
> for the CPU to predict.

Hmm, I was assuming the latency due to a (mispredicted) branch would be in the
noise compared to the latency of a VM-Exit needed to handle the fault.

On the other hand, adding a common macro would be trivial, it's only the naming
that's hard.  What if we keep with the "cycles" theme and do as Ricardo suggested?
E.g. minus backslashes, this doesn't look awful.

#define MEASURE_CYCLES(x)
({
	uint64_t start;

	start = guest_system_counter_read();
	x;
	guest_system_counter_read() - start;
})



> > > > +			if (i < SAMPLES_PER_VCPU)
> 
> > Would it make sense to let the user configure the number of samples?
> > Seems easy enough and would let the user ultimately decide how much memory
> > to burn on samples.
> 
> Theoretically users may wish to tweak the accuracy vs memory use
> tradeoff. Seemed like a shakey value proposition to me because of
> diminishing returns to increased accuracy, but I will include an option
> if you insist.

It's not so much that I expect users to want better accuracy, it's that I dislike
I arbitrary magic numbers.  E.g. why 1000 and not 500 or 2000?  Especially since
the number of accesses _is_ controllable by the user.  Hmm, the other thing to
consider is that, as proposed, the vast majority of the capacity will go unused,
e.g. default is 4, max is 512 (and that should really be tied to KVM's max of 4096).

What if we invert the calculation and define the capacity in bytes, and derive
the number of samples based on capacity / nr_vcpus?  And then push the capacity
as high as possible, e.g. I assume there's a threshold where the test will fail
because of selftests poor page table management.  That way the sampling is as
efficient as possible, and the arbitrary capacity comes from limitations within
the framework, as opposed to a human defined magic number.