[PATCH] KVM: selftests: Provide generic way to read system counter

[PATCH] KVM: selftests: Provide generic way to read system counter

[Colton Lewis]

Provide a generic function to read the system counter from the guest
for timing purposes. An increasingly common and important way to
measure guest performance is to measure the amount of time different
actions take in the guest. Provide also a mathematical conversion from
cycles to nanoseconds and a macro for timing individual statements.

Substitute the previous custom implementation of a similar function in
system_counter_offset_test with this new implementation.

Signed-off-by: Colton Lewis <coltonlewis@google.com>
---

These functions were originally part of my patch to introduce latency
measurements into dirty_log_perf_test. [1] Sean Christopherson
suggested lifting these functions into their own patch in generic code
so they can be used by any test. [2] Ricardo Koller suggested the
addition of the MEASURE macro to more easily time individual
statements. [3]

[1] https://lore.kernel.org/kvm/20221115173258.2530923-1-coltonlewis@google.com/
[2] https://lore.kernel.org/kvm/Y8gfOP5CMXK60AtH@google.com/
[3] https://lore.kernel.org/kvm/Y8cIdxp5k8HivVAe@google.com/

 .../testing/selftests/kvm/include/test_util.h |  3 ++
 tools/testing/selftests/kvm/lib/test_util.c   | 37 +++++++++++++++++++
 .../kvm/system_counter_offset_test.c          | 10 +----
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 80d6416f3012..290653b99035 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -84,6 +84,9 @@ struct guest_random_state {
 struct guest_random_state new_guest_random_state(uint32_t seed);
 uint32_t guest_random_u32(struct guest_random_state *state);

+uint64_t guest_cycles_read(void);
+double guest_cycles_to_ns(double cycles);
+
 enum vm_mem_backing_src_type {
 	VM_MEM_SRC_ANONYMOUS,
 	VM_MEM_SRC_ANONYMOUS_THP,
diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index 5c22fa4c2825..6d88132a0131 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -15,6 +15,11 @@
 #include <linux/mman.h>
 #include "linux/kernel.h"

+#if defined(__aarch64__)
+#include "aarch64/arch_timer.h"
+#elif defined(__x86_64__)
+#include "x86_64/processor.h"
+#endif
 #include "test_util.h"

 /*
@@ -34,6 +39,38 @@ uint32_t guest_random_u32(struct guest_random_state *state)
 	return state->seed;
 }

+uint64_t guest_cycles_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
+}
+
+double guest_cycles_to_ns(double cycles)
+{
+#if defined(__aarch64__)
+	return cycles * (1e9 / timer_get_cntfrq());
+#elif defined(__x86_64__)
+	return cycles * (1e9 / (KVM_GET_TSC_KHZ * 1000));
+#else
+#warn __func__ " is not implemented for this architecture, will return 0"
+	return 0.0;
+#endif
+}
+
+#define MEASURE_CYCLES(x)			\
+	({					\
+		uint64_t start;			\
+		start = guest_cycles_read();	\
+		x;				\
+		guest_cycles_read() - start;	\
+	})
+
 /*
  * Parses "[0-9]+[kmgt]?".
  */
diff --git a/tools/testing/selftests/kvm/system_counter_offset_test.c b/tools/testing/selftests/kvm/system_counter_offset_test.c
index 7f5b330b6a1b..39b1249c7404 100644
--- a/tools/testing/selftests/kvm/system_counter_offset_test.c
+++ b/tools/testing/selftests/kvm/system_counter_offset_test.c
@@ -39,14 +39,9 @@ static void setup_system_counter(struct kvm_vcpu *vcpu, struct test_case *test)
 			     &test->tsc_offset);
 }

-static uint64_t guest_read_system_counter(struct test_case *test)
-{
-	return rdtsc();
-}
-
 static uint64_t host_read_guest_system_counter(struct test_case *test)
 {
-	return rdtsc() + test->tsc_offset;
+	return guest_cycles_read() + test->tsc_offset;
 }

 #else /* __x86_64__ */
@@ -63,9 +58,8 @@ static void guest_main(void)
 	int i;

 	for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
-		struct test_case *test = &test_cases[i];

-		GUEST_SYNC_CLOCK(i, guest_read_system_counter(test));
+		GUEST_SYNC_CLOCK(i, guest_cycles_read());
 	}
 }

--
2.39.2.637.g21b0678d19-goog

Re: [PATCH] KVM: selftests: Provide generic way to read system counter

[Oliver Upton]

Hi Colton,

On Tue, Feb 21, 2023 at 11:27:40PM +0000, Colton Lewis wrote:
> Provide a generic function to read the system counter from the guest
> for timing purposes. An increasingly common and important way to
> measure guest performance is to measure the amount of time different
> actions take in the guest. Provide also a mathematical conversion from
> cycles to nanoseconds and a macro for timing individual statements.
> 
> Substitute the previous custom implementation of a similar function in
> system_counter_offset_test with this new implementation.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
> 
> These functions were originally part of my patch to introduce latency
> measurements into dirty_log_perf_test. [1] Sean Christopherson
> suggested lifting these functions into their own patch in generic code
> so they can be used by any test. [2] Ricardo Koller suggested the
> addition of the MEASURE macro to more easily time individual
> statements. [3]
> 
> [1] https://lore.kernel.org/kvm/20221115173258.2530923-1-coltonlewis@google.com/
> [2] https://lore.kernel.org/kvm/Y8gfOP5CMXK60AtH@google.com/
> [3] https://lore.kernel.org/kvm/Y8cIdxp5k8HivVAe@google.com/

This patch doesn't make a great deal of sense outside of [1]. Can you
send this as part of your larger series next time around?

>  .../testing/selftests/kvm/include/test_util.h |  3 ++
>  tools/testing/selftests/kvm/lib/test_util.c   | 37 +++++++++++++++++++
>  .../kvm/system_counter_offset_test.c          | 10 +----
>  3 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
> index 80d6416f3012..290653b99035 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -84,6 +84,9 @@ struct guest_random_state {
>  struct guest_random_state new_guest_random_state(uint32_t seed);
>  uint32_t guest_random_u32(struct guest_random_state *state);
> 
> +uint64_t guest_cycles_read(void);
> +double guest_cycles_to_ns(double cycles);
>
>  enum vm_mem_backing_src_type {
>  	VM_MEM_SRC_ANONYMOUS,
>  	VM_MEM_SRC_ANONYMOUS_THP,
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index 5c22fa4c2825..6d88132a0131 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -15,6 +15,11 @@
>  #include <linux/mman.h>
>  #include "linux/kernel.h"
> 
> +#if defined(__aarch64__)
> +#include "aarch64/arch_timer.h"
> +#elif defined(__x86_64__)
> +#include "x86_64/processor.h"
> +#endif

I believe we place 'include/$(ARCH)/' on the include path, so the
'x86_64' can be dropped.

>  #include "test_util.h"
> 
>  /*
> @@ -34,6 +39,38 @@ uint32_t guest_random_u32(struct guest_random_state *state)
>  	return state->seed;
>  }
> 
> +uint64_t guest_cycles_read(void)

Do we need the 'guest_' prefix in here? At least for x86 and arm64 the
same instructions can be used in host userspace and guest kernel for
accessing the counter.

> +{
> +#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 think it would be cleaner if each architecture just provided its own
implementation of this function instead of ifdeffery here.

If an arch doesn't implement the requisite helper then it will become
painfully obvious at compile time.

> +double guest_cycles_to_ns(double cycles)
> +{
> +#if defined(__aarch64__)
> +	return cycles * (1e9 / timer_get_cntfrq());
> +#elif defined(__x86_64__)
> +	return cycles * (1e9 / (KVM_GET_TSC_KHZ * 1000));

The x86 implementation is wrong. KVM_GET_TSC_KHZ is the ioctl needed
to get at the guest's TSC frequency (from the perpsective of host
userspace).

So at the very least this needs to do an ioctl on the VM fd to work out
the frequency. This is expected to be called in host userspace, right?

> +#else
> +#warn __func__ " is not implemented for this architecture, will return 0"
> +	return 0.0;
> +#endif
> +}
> +
> +#define MEASURE_CYCLES(x)			\
> +	({					\
> +		uint64_t start;			\
> +		start = guest_cycles_read();	\
> +		x;				\
> +		guest_cycles_read() - start;	\
> +	})
> +
>  /*
>   * Parses "[0-9]+[kmgt]?".
>   */
> diff --git a/tools/testing/selftests/kvm/system_counter_offset_test.c b/tools/testing/selftests/kvm/system_counter_offset_test.c
> index 7f5b330b6a1b..39b1249c7404 100644
> --- a/tools/testing/selftests/kvm/system_counter_offset_test.c
> +++ b/tools/testing/selftests/kvm/system_counter_offset_test.c
> @@ -39,14 +39,9 @@ static void setup_system_counter(struct kvm_vcpu *vcpu, struct test_case *test)
>  			     &test->tsc_offset);
>  }
> 
> -static uint64_t guest_read_system_counter(struct test_case *test)
> -{
> -	return rdtsc();
> -}
> -
>  static uint64_t host_read_guest_system_counter(struct test_case *test)
>  {
> -	return rdtsc() + test->tsc_offset;
> +	return guest_cycles_read() + test->tsc_offset;

The 'guest_' part is rather confusing here. What this function is really
trying to do is read the counter from host userspace and apply an offset
to construct the expected value for the test.

It all compiles down to the same thing, but as written is seemingly
wrong.

-- 
Thanks,
Oliver

Re: [PATCH] KVM: selftests: Provide generic way to read system counter

[Colton Lewis]

Oliver Upton <oliver.upton@linux.dev> writes:

>> These functions were originally part of my patch to introduce latency
>> measurements into dirty_log_perf_test. [1] Sean Christopherson
>> suggested lifting these functions into their own patch in generic code
>> so they can be used by any test. [2] Ricardo Koller suggested the
>> addition of the MEASURE macro to more easily time individual
>> statements. [3]

>> [1]  
>> https://lore.kernel.org/kvm/20221115173258.2530923-1-coltonlewis@google.com/
>> [2] https://lore.kernel.org/kvm/Y8gfOP5CMXK60AtH@google.com/
>> [3] https://lore.kernel.org/kvm/Y8cIdxp5k8HivVAe@google.com/

> This patch doesn't make a great deal of sense outside of [1]. Can you
> send this as part of your larger series next time around?

I copied the wrong email link where Sean suggested this should be
generic code in a separate patch. I may have been mistaken in thinking
he meant to upstream it separately.

https://lore.kernel.org/kvm/Y8gjG6gG5UR6T3Yg@google.com/

But yes, I would prefer to keep this as part of the larger series.

>>   #include <linux/mman.h>
>>   #include "linux/kernel.h"

>> +#if defined(__aarch64__)
>> +#include "aarch64/arch_timer.h"
>> +#elif defined(__x86_64__)
>> +#include "x86_64/processor.h"
>> +#endif

> I believe we place 'include/$(ARCH)/' on the include path, so the
> 'x86_64' can be dropped.

Good to know.

>>   #include "test_util.h"

>>   /*
>> @@ -34,6 +39,38 @@ uint32_t guest_random_u32(struct guest_random_state  
>> *state)
>>   	return state->seed;
>>   }

>> +uint64_t guest_cycles_read(void)

> Do we need the 'guest_' prefix in here? At least for x86 and arm64 the
> same instructions can be used in host userspace and guest kernel for
> accessing the counter.

It's intended for guest space, but I take your point it isn't necessary.

>> +{
>> +#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 think it would be cleaner if each architecture just provided its own
> implementation of this function instead of ifdeffery here.

> If an arch doesn't implement the requisite helper then it will become
> painfully obvious at compile time.

Good point. That will be more clear.

>> +double guest_cycles_to_ns(double cycles)
>> +{
>> +#if defined(__aarch64__)
>> +	return cycles * (1e9 / timer_get_cntfrq());
>> +#elif defined(__x86_64__)
>> +	return cycles * (1e9 / (KVM_GET_TSC_KHZ * 1000));

> The x86 implementation is wrong. KVM_GET_TSC_KHZ is the ioctl needed
> to get at the guest's TSC frequency (from the perpsective of host
> userspace).

Oops. Absent minded on that one. Forgot it wasn't just a value I could use.

> So at the very least this needs to do an ioctl on the VM fd to work out
> the frequency. This is expected to be called in host userspace, right?

That's the plan yes.

>> diff --git a/tools/testing/selftests/kvm/system_counter_offset_test.c  
>> b/tools/testing/selftests/kvm/system_counter_offset_test.c
>> index 7f5b330b6a1b..39b1249c7404 100644
>> --- a/tools/testing/selftests/kvm/system_counter_offset_test.c
>> +++ b/tools/testing/selftests/kvm/system_counter_offset_test.c
>> @@ -39,14 +39,9 @@ static void setup_system_counter(struct kvm_vcpu  
>> *vcpu, struct test_case *test)
>>   			     &test->tsc_offset);
>>   }

>> -static uint64_t guest_read_system_counter(struct test_case *test)
>> -{
>> -	return rdtsc();
>> -}
>> -
>>   static uint64_t host_read_guest_system_counter(struct test_case *test)
>>   {
>> -	return rdtsc() + test->tsc_offset;
>> +	return guest_cycles_read() + test->tsc_offset;

> The 'guest_' part is rather confusing here. What this function is really
> trying to do is read the counter from host userspace and apply an offset
> to construct the expected value for the test.

> It all compiles down to the same thing, but as written is seemingly
> wrong.

I'll eliminate the guest_prefix.

Re: [PATCH] KVM: selftests: Provide generic way to read system counter

[Sean Christopherson]

On Fri, Feb 24, 2023, Colton Lewis wrote:
> Oliver Upton <oliver.upton@linux.dev> writes:
> 
> > > These functions were originally part of my patch to introduce latency
> > > measurements into dirty_log_perf_test. [1] Sean Christopherson
> > > suggested lifting these functions into their own patch in generic code
> > > so they can be used by any test. [2] Ricardo Koller suggested the
> > > addition of the MEASURE macro to more easily time individual
> > > statements. [3]
> 
> > > [1] https://lore.kernel.org/kvm/20221115173258.2530923-1-coltonlewis@google.com/
> > > [2] https://lore.kernel.org/kvm/Y8gfOP5CMXK60AtH@google.com/
> > > [3] https://lore.kernel.org/kvm/Y8cIdxp5k8HivVAe@google.com/
> 
> > This patch doesn't make a great deal of sense outside of [1]. Can you
> > send this as part of your larger series next time around?
> 
> I copied the wrong email link where Sean suggested this should be
> generic code in a separate patch. I may have been mistaken in thinking
> he meant to upstream it separately.
> 
> https://lore.kernel.org/kvm/Y8gjG6gG5UR6T3Yg@google.com/
> 
> But yes, I would prefer to keep this as part of the larger series.

Yeah, I just meant put it into a separate patch, but keep it in the series.