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

[Sean Christopherson <seanjc@google.com>]

On Tue, Jan 17, 2023, Ricardo Koller wrote:
> On Tue, Nov 15, 2022 at 05:32:57PM +0000, Colton Lewis wrote:
> > @@ -44,6 +47,18 @@ static struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> >  /* Store all samples in a flat array so they can be easily sorted later. */
> >  uint64_t latency_samples[SAMPLE_CAPACITY];
> >  
> > +static uint64_t perf_test_timer_read(void)
> > +{
> > +#if defined(__aarch64__)
> > +	return timer_get_cntct(VIRTUAL);
> > +#elif defined(__x86_64__)
> > +	return rdtsc();
> > +#else
> > +#warn __func__ " is not implemented for this architecture, will return 0"
> > +	return 0;
> > +#endif
> > +}

I would prefer to put the guest-side timer helpers into common code, e.g. as
guest_read_system_counter(), replacing system_counter_offset_test.c's one-off
version.

> >  /*
> >   * Continuously write to the first 8 bytes of each page in the
> >   * specified region.
> > @@ -59,6 +74,10 @@ void perf_test_guest_code(uint32_t vcpu_idx)
> >  	int i;
> >  	struct guest_random_state rand_state =
> >  		new_guest_random_state(pta->random_seed + vcpu_idx);
> > +	uint64_t *latency_samples_offset = latency_samples + SAMPLES_PER_VCPU * vcpu_idx;

"offset" is confusing because the system counter (TSC in x86) has an offset for
the guest-perceived value.  Maybe just "latencies"?

> > +	uint64_t count_before;
> > +	uint64_t count_after;

Maybe s/count/time?  Yeah, it's technically wrong to call it "time", but "count"
is too generic.

> > +	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();

> > +			}
> > +
> > +			maybe_sample = guest_random_u32(&rand_state) % (i + 1);

No need to generate a random number for iterations that always sample.  And I
think it will make the code easier to follow if there is a single write to the
array.  The derivation of the index is what's interesting and different, we should
use code to highlight that.

			/*
			 * Always sample early iterations to ensure at least the
			 * number of requested samples is collected.  Once the
			 * array has been filled, <here is a comment from Colton
			 * briefly explaining the math>.
			 * 
			if (i < SAMPLES_PER_VCPU)
				idx = i;
			else
				idx = guest_random_u32(&rand_state) % (i + 1);

			if (idx < SAMPLES_PER_VCPU)
				latencies[idx] = time_after - time_before;

> > +			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.

> > +				latency_samples_offset[i] = count_after - count_before;
> > +			else if (maybe_sample < SAMPLES_PER_VCPU)
> > +				latency_samples_offset[maybe_sample] = count_after - count_before;
> 
> I would prefer these reservoir sampling details to be in a helper, 
> e.g.,:
> 	reservoir_sample_record(t, i);

Heh, I vote to open code the behavior.  I dislike fancy names that hide relatively
simple logic.  IMO, readers won't care how the modulo math provides even distribution,
just that it does and that the early iterations always samples to ensure ever bucket
is filled.

In this case, I can pretty much guarantee that I'd end up spending more time
digging into what "reservoir" means than I would to understand the basic flow.