[PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz

[PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz

[Nicolas Saenz Julienne]

'cpu_mhz' in oslat actually represents the frequency at which the high
frequency counter we measure with ticks. There is no requirement for the
counter to match the CPU frequency, nor is forced to do so on any of the
supported architectures[1][2]. So rename it to 'counter_mhz' in order to
better match reality.

[1] x86_64
Intel TRM Vol 3B, 17.17 Time Stamp Counter:
"Constant TSC behavior ensures that the duration of each clock tick is
uniform and supports the use of the TSC as a wall clock timer even if
the processor core changes frequency."

[2] ppc64
From __ppc_get_timebase() manpages: The Time Base Register is a 64-bit
register provided by Power Architecture processors. It stores a
monotonically incremented value that is updated at a system-dependent
frequency that may be different from the processor frequency. Note that
glibc's __ppc_get_timebase() and oslat's ppc64 frc() implementations are
the same.

Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
---

Changes since v1:
 - More complete commit message
 - s/timer/counter/

 src/oslat/oslat.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
index 6ff5ba8..33cccd3 100644
--- a/src/oslat/oslat.c
+++ b/src/oslat/oslat.c
@@ -123,7 +123,7 @@ struct thread {
 	pthread_t            thread_id;
 
 	/* NOTE! this is also how many ticks per us */
-	unsigned int         cpu_mhz;
+	unsigned int         counter_mhz;
 	cycles_t             int_total;
 	stamp_t              frc_start;
 	stamp_t              frc_stop;
@@ -228,7 +228,7 @@ static int move_to_core(int core_i)
 	return sched_setaffinity(0, sizeof(cpus), &cpus);
 }
 
-static cycles_t __measure_cpu_hz(void)
+static cycles_t __measure_counter_hz(void)
 {
 	struct timeval tvs, tve;
 	stamp_t s, e;
@@ -244,13 +244,13 @@ static cycles_t __measure_cpu_hz(void)
 	return (cycles_t) ((e - s) / sec);
 }
 
-static unsigned int measure_cpu_mhz(void)
+static unsigned int measure_counter_mhz(void)
 {
 	cycles_t m, mprev, d;
 
-	mprev = __measure_cpu_hz();
+	mprev = __measure_counter_hz();
 	do {
-		m = __measure_cpu_hz();
+		m = __measure_counter_hz();
 		if (m > mprev)
 			d = m - mprev;
 		else
@@ -263,7 +263,7 @@ static unsigned int measure_cpu_mhz(void)
 
 static void thread_init(struct thread *t)
 {
-	t->cpu_mhz = measure_cpu_mhz();
+	t->counter_mhz = measure_counter_mhz();
 	t->maxlat = 0;
 	t->overflow_sum = 0;
 	t->minlat = (uint64_t)-1;
@@ -288,7 +288,7 @@ static void thread_init(struct thread *t)
 
 static float cycles_to_sec(const struct thread *t, uint64_t cycles)
 {
-	return cycles / (t->cpu_mhz * 1e6);
+	return cycles / (t->counter_mhz * 1e6);
 }
 
 static void insert_bucket(struct thread *t, stamp_t value)
@@ -296,7 +296,7 @@ static void insert_bucket(struct thread *t, stamp_t value)
 	int index, us;
 	uint64_t extra;
 
-	index = value / t->cpu_mhz;
+	index = value / t->counter_mhz;
 	assert(index >= 0);
 	us = index + 1;
 	assert(us > 0);
@@ -450,7 +450,7 @@ static void write_summary(struct thread *t)
 	calculate(t);
 
 	putfield("Core", t[i].core_i, "d", "");
-	putfield("CPU Freq", t[i].cpu_mhz, "u", " (Mhz)");
+	putfield("Counter Freq", t[i].counter_mhz, "u", " (Mhz)");
 
 	for (j = 0; j < g.bucket_size; j++) {
 		if (j < g.bucket_size-1 && g.output_omit_zero_buckets) {
@@ -494,7 +494,7 @@ static void write_summary_json(FILE *f, void *data)
 	for (i = 0; i < g.n_threads; ++i) {
 		fprintf(f, "    \"%u\": {\n", i);
 		fprintf(f, "      \"cpu\": %d,\n", t[i].core_i);
-		fprintf(f, "      \"freq\": %d,\n", t[i].cpu_mhz);
+		fprintf(f, "      \"freq\": %d,\n", t[i].counter_mhz);
 		fprintf(f, "      \"min\": %" PRIu64 ",\n", t[i].minlat);
 		fprintf(f, "      \"avg\": %3lf,\n", t[i].average);
 		fprintf(f, "      \"max\": %" PRIu64 ",\n", t[i].maxlat);
-- 
2.31.1

[PATCH v2 2/3] oslat: Add aarch64 support

[Nicolas Saenz Julienne]

The callbacks are based on Linux's implementation:
 - CNTVCT_EL0 provides direct access to the system virtual timer[1].
 - 'yield' serves as a CPU hint with similar semantics as x86's
   'pause'[2].

In contrast with the kernel's implementation, there isn't a need for
isb() after reading CNTVCT_EL0,  this is only needed in-kernel as the
register read has to be ordered with a subsequent locking operation.

[1] See Linux's '__arch_get_hw_counter()' in arch/arm64/include/asm/vdso/gettimeofday.h
[2] See Linux's 1baa82f4803 ("arm64: Implement cpu_relax as yield").
Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
--

Changes since v1:
 - Code cleanup
 - Add compiler barriers

 src/oslat/oslat.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
index 33cccd3..c90ec1a 100644
--- a/src/oslat/oslat.c
+++ b/src/oslat/oslat.c
@@ -71,6 +71,19 @@ static inline void frc(uint64_t *pval)
 {
 	__asm__ __volatile__("mfspr %0, 268\n" : "=r" (*pval));
 }
+# elif defined(__aarch64__)
+#  define relax()          __asm__ __volatile("yield" : : : "memory")
+
+static inline void frc(uint64_t *pval)
+{
+	/*
+	 * This isb() is required to prevent that the counter value
+	 * is speculated.
+	 */
+	__asm__ __volatile__("isb" : : : "memory");
+	__asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (*pval) :: "memory");
+
+}
 # else
 #  define relax()          do { } while (0)
 #  define frc(x)
-- 
2.31.1

[PATCH v2 3/3] oslat: Allow for arch specific counter frequency measurements

[Nicolas Saenz Julienne]

Some architectures have special purpose registers to query the system
counter's frequency. Let's use that when available.

Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
--

Changes since v1:
 - Use cleaner method to have generic and arch functions to measure
   couter's freq

 src/oslat/oslat.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
index c90ec1a..5fce223 100644
--- a/src/oslat/oslat.c
+++ b/src/oslat/oslat.c
@@ -74,6 +74,16 @@ static inline void frc(uint64_t *pval)
 # elif defined(__aarch64__)
 #  define relax()          __asm__ __volatile("yield" : : : "memory")
 
+#define arch_measure_counter_mhz
+static unsigned int measure_counter_mhz(void)
+{
+	unsigned int val;
+
+	__asm__ __volatile__("mrs %0, cntfrq_el0" : "=r" (val));
+
+	return val / 1e6;
+}
+
 static inline void frc(uint64_t *pval)
 {
 	/*
@@ -241,6 +251,7 @@ static int move_to_core(int core_i)
 	return sched_setaffinity(0, sizeof(cpus), &cpus);
 }
 
+#ifndef arch_measure_counter_mhz
 static cycles_t __measure_counter_hz(void)
 {
 	struct timeval tvs, tve;
@@ -273,6 +284,7 @@ static unsigned int measure_counter_mhz(void)
 
 	return (unsigned int) (m / 1000000);
 }
+#endif
 
 static void thread_init(struct thread *t)
 {
-- 
2.31.1

Re: [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz

[John Kacur]

On Mon, 13 Sep 2021, Nicolas Saenz Julienne wrote:

> 'cpu_mhz' in oslat actually represents the frequency at which the high
> frequency counter we measure with ticks. There is no requirement for the
> counter to match the CPU frequency, nor is forced to do so on any of the
> supported architectures[1][2]. So rename it to 'counter_mhz' in order to
> better match reality.
> 
> [1] x86_64
> Intel TRM Vol 3B, 17.17 Time Stamp Counter:
> "Constant TSC behavior ensures that the duration of each clock tick is
> uniform and supports the use of the TSC as a wall clock timer even if
> the processor core changes frequency."
> 
> [2] ppc64
> From __ppc_get_timebase() manpages: The Time Base Register is a 64-bit
> register provided by Power Architecture processors. It stores a
> monotonically incremented value that is updated at a system-dependent
> frequency that may be different from the processor frequency. Note that
> glibc's __ppc_get_timebase() and oslat's ppc64 frc() implementations are
> the same.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> ---
> 
> Changes since v1:
>  - More complete commit message
>  - s/timer/counter/
> 
>  src/oslat/oslat.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index 6ff5ba8..33cccd3 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -123,7 +123,7 @@ struct thread {
>  	pthread_t            thread_id;
>  
>  	/* NOTE! this is also how many ticks per us */
> -	unsigned int         cpu_mhz;
> +	unsigned int         counter_mhz;
>  	cycles_t             int_total;
>  	stamp_t              frc_start;
>  	stamp_t              frc_stop;
> @@ -228,7 +228,7 @@ static int move_to_core(int core_i)
>  	return sched_setaffinity(0, sizeof(cpus), &cpus);
>  }
>  
> -static cycles_t __measure_cpu_hz(void)
> +static cycles_t __measure_counter_hz(void)
>  {
>  	struct timeval tvs, tve;
>  	stamp_t s, e;
> @@ -244,13 +244,13 @@ static cycles_t __measure_cpu_hz(void)
>  	return (cycles_t) ((e - s) / sec);
>  }
>  
> -static unsigned int measure_cpu_mhz(void)
> +static unsigned int measure_counter_mhz(void)
>  {
>  	cycles_t m, mprev, d;
>  
> -	mprev = __measure_cpu_hz();
> +	mprev = __measure_counter_hz();
>  	do {
> -		m = __measure_cpu_hz();
> +		m = __measure_counter_hz();
>  		if (m > mprev)
>  			d = m - mprev;
>  		else
> @@ -263,7 +263,7 @@ static unsigned int measure_cpu_mhz(void)
>  
>  static void thread_init(struct thread *t)
>  {
> -	t->cpu_mhz = measure_cpu_mhz();
> +	t->counter_mhz = measure_counter_mhz();
>  	t->maxlat = 0;
>  	t->overflow_sum = 0;
>  	t->minlat = (uint64_t)-1;
> @@ -288,7 +288,7 @@ static void thread_init(struct thread *t)
>  
>  static float cycles_to_sec(const struct thread *t, uint64_t cycles)
>  {
> -	return cycles / (t->cpu_mhz * 1e6);
> +	return cycles / (t->counter_mhz * 1e6);
>  }
>  
>  static void insert_bucket(struct thread *t, stamp_t value)
> @@ -296,7 +296,7 @@ static void insert_bucket(struct thread *t, stamp_t value)
>  	int index, us;
>  	uint64_t extra;
>  
> -	index = value / t->cpu_mhz;
> +	index = value / t->counter_mhz;
>  	assert(index >= 0);
>  	us = index + 1;
>  	assert(us > 0);
> @@ -450,7 +450,7 @@ static void write_summary(struct thread *t)
>  	calculate(t);
>  
>  	putfield("Core", t[i].core_i, "d", "");
> -	putfield("CPU Freq", t[i].cpu_mhz, "u", " (Mhz)");
> +	putfield("Counter Freq", t[i].counter_mhz, "u", " (Mhz)");
>  
>  	for (j = 0; j < g.bucket_size; j++) {
>  		if (j < g.bucket_size-1 && g.output_omit_zero_buckets) {
> @@ -494,7 +494,7 @@ static void write_summary_json(FILE *f, void *data)
>  	for (i = 0; i < g.n_threads; ++i) {
>  		fprintf(f, "    \"%u\": {\n", i);
>  		fprintf(f, "      \"cpu\": %d,\n", t[i].core_i);
> -		fprintf(f, "      \"freq\": %d,\n", t[i].cpu_mhz);
> +		fprintf(f, "      \"freq\": %d,\n", t[i].counter_mhz);
>  		fprintf(f, "      \"min\": %" PRIu64 ",\n", t[i].minlat);
>  		fprintf(f, "      \"avg\": %3lf,\n", t[i].average);
>  		fprintf(f, "      \"max\": %" PRIu64 ",\n", t[i].maxlat);
> -- 
> 2.31.1
> 
> 

Waiting for a Signed-off-by: from Peter Xu before I integrate these.

Thanks

John

Re: [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz

[Peter Xu]

On Mon, Sep 13, 2021 at 09:35:29AM -0400, John Kacur wrote:
> Waiting for a Signed-off-by: from Peter Xu before I integrate these.

All 3 patches:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz

[John Kacur]

On Mon, 13 Sep 2021, Nicolas Saenz Julienne wrote:

> 'cpu_mhz' in oslat actually represents the frequency at which the high
> frequency counter we measure with ticks. There is no requirement for the
> counter to match the CPU frequency, nor is forced to do so on any of the
> supported architectures[1][2]. So rename it to 'counter_mhz' in order to
> better match reality.
> 
> [1] x86_64
> Intel TRM Vol 3B, 17.17 Time Stamp Counter:
> "Constant TSC behavior ensures that the duration of each clock tick is
> uniform and supports the use of the TSC as a wall clock timer even if
> the processor core changes frequency."
> 
> [2] ppc64
> From __ppc_get_timebase() manpages: The Time Base Register is a 64-bit
> register provided by Power Architecture processors. It stores a
> monotonically incremented value that is updated at a system-dependent
> frequency that may be different from the processor frequency. Note that
> glibc's __ppc_get_timebase() and oslat's ppc64 frc() implementations are
> the same.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> ---
> 
> Changes since v1:
>  - More complete commit message
>  - s/timer/counter/
> 
>  src/oslat/oslat.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index 6ff5ba8..33cccd3 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -123,7 +123,7 @@ struct thread {
>  	pthread_t            thread_id;
>  
>  	/* NOTE! this is also how many ticks per us */
> -	unsigned int         cpu_mhz;
> +	unsigned int         counter_mhz;
>  	cycles_t             int_total;
>  	stamp_t              frc_start;
>  	stamp_t              frc_stop;
> @@ -228,7 +228,7 @@ static int move_to_core(int core_i)
>  	return sched_setaffinity(0, sizeof(cpus), &cpus);
>  }
>  
> -static cycles_t __measure_cpu_hz(void)
> +static cycles_t __measure_counter_hz(void)
>  {
>  	struct timeval tvs, tve;
>  	stamp_t s, e;
> @@ -244,13 +244,13 @@ static cycles_t __measure_cpu_hz(void)
>  	return (cycles_t) ((e - s) / sec);
>  }
>  
> -static unsigned int measure_cpu_mhz(void)
> +static unsigned int measure_counter_mhz(void)
>  {
>  	cycles_t m, mprev, d;
>  
> -	mprev = __measure_cpu_hz();
> +	mprev = __measure_counter_hz();
>  	do {
> -		m = __measure_cpu_hz();
> +		m = __measure_counter_hz();
>  		if (m > mprev)
>  			d = m - mprev;
>  		else
> @@ -263,7 +263,7 @@ static unsigned int measure_cpu_mhz(void)
>  
>  static void thread_init(struct thread *t)
>  {
> -	t->cpu_mhz = measure_cpu_mhz();
> +	t->counter_mhz = measure_counter_mhz();
>  	t->maxlat = 0;
>  	t->overflow_sum = 0;
>  	t->minlat = (uint64_t)-1;
> @@ -288,7 +288,7 @@ static void thread_init(struct thread *t)
>  
>  static float cycles_to_sec(const struct thread *t, uint64_t cycles)
>  {
> -	return cycles / (t->cpu_mhz * 1e6);
> +	return cycles / (t->counter_mhz * 1e6);
>  }
>  
>  static void insert_bucket(struct thread *t, stamp_t value)
> @@ -296,7 +296,7 @@ static void insert_bucket(struct thread *t, stamp_t value)
>  	int index, us;
>  	uint64_t extra;
>  
> -	index = value / t->cpu_mhz;
> +	index = value / t->counter_mhz;
>  	assert(index >= 0);
>  	us = index + 1;
>  	assert(us > 0);
> @@ -450,7 +450,7 @@ static void write_summary(struct thread *t)
>  	calculate(t);
>  
>  	putfield("Core", t[i].core_i, "d", "");
> -	putfield("CPU Freq", t[i].cpu_mhz, "u", " (Mhz)");
> +	putfield("Counter Freq", t[i].counter_mhz, "u", " (Mhz)");
>  
>  	for (j = 0; j < g.bucket_size; j++) {
>  		if (j < g.bucket_size-1 && g.output_omit_zero_buckets) {
> @@ -494,7 +494,7 @@ static void write_summary_json(FILE *f, void *data)
>  	for (i = 0; i < g.n_threads; ++i) {
>  		fprintf(f, "    \"%u\": {\n", i);
>  		fprintf(f, "      \"cpu\": %d,\n", t[i].core_i);
> -		fprintf(f, "      \"freq\": %d,\n", t[i].cpu_mhz);
> +		fprintf(f, "      \"freq\": %d,\n", t[i].counter_mhz);
>  		fprintf(f, "      \"min\": %" PRIu64 ",\n", t[i].minlat);
>  		fprintf(f, "      \"avg\": %3lf,\n", t[i].average);
>  		fprintf(f, "      \"max\": %" PRIu64 ",\n", t[i].maxlat);
> -- 
> 2.31.1
> 
> 
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH v2 2/3] oslat: Add aarch64 support

[John Kacur]

On Mon, 13 Sep 2021, Nicolas Saenz Julienne wrote:

> The callbacks are based on Linux's implementation:
>  - CNTVCT_EL0 provides direct access to the system virtual timer[1].
>  - 'yield' serves as a CPU hint with similar semantics as x86's
>    'pause'[2].
> 
> In contrast with the kernel's implementation, there isn't a need for
> isb() after reading CNTVCT_EL0,  this is only needed in-kernel as the
> register read has to be ordered with a subsequent locking operation.
> 
> [1] See Linux's '__arch_get_hw_counter()' in arch/arm64/include/asm/vdso/gettimeofday.h
> [2] See Linux's 1baa82f4803 ("arm64: Implement cpu_relax as yield").
> Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> --
> 
> Changes since v1:
>  - Code cleanup
>  - Add compiler barriers
> 
>  src/oslat/oslat.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index 33cccd3..c90ec1a 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -71,6 +71,19 @@ static inline void frc(uint64_t *pval)
>  {
>  	__asm__ __volatile__("mfspr %0, 268\n" : "=r" (*pval));
>  }
> +# elif defined(__aarch64__)
> +#  define relax()          __asm__ __volatile("yield" : : : "memory")
> +
> +static inline void frc(uint64_t *pval)
> +{
> +	/*
> +	 * This isb() is required to prevent that the counter value
> +	 * is speculated.
> +	 */
> +	__asm__ __volatile__("isb" : : : "memory");
> +	__asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (*pval) :: "memory");
> +
> +}
>  # else
>  #  define relax()          do { } while (0)
>  #  define frc(x)
> -- 
> 2.31.1
> 
> 
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH v2 3/3] oslat: Allow for arch specific counter frequency measurements

[John Kacur]

On Mon, 13 Sep 2021, Nicolas Saenz Julienne wrote:

> Some architectures have special purpose registers to query the system
> counter's frequency. Let's use that when available.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> --
> 
> Changes since v1:
>  - Use cleaner method to have generic and arch functions to measure
>    couter's freq
> 
>  src/oslat/oslat.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index c90ec1a..5fce223 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -74,6 +74,16 @@ static inline void frc(uint64_t *pval)
>  # elif defined(__aarch64__)
>  #  define relax()          __asm__ __volatile("yield" : : : "memory")
>  
> +#define arch_measure_counter_mhz
> +static unsigned int measure_counter_mhz(void)
> +{
> +	unsigned int val;
> +
> +	__asm__ __volatile__("mrs %0, cntfrq_el0" : "=r" (val));
> +
> +	return val / 1e6;
> +}
> +
>  static inline void frc(uint64_t *pval)
>  {
>  	/*
> @@ -241,6 +251,7 @@ static int move_to_core(int core_i)
>  	return sched_setaffinity(0, sizeof(cpus), &cpus);
>  }
>  
> +#ifndef arch_measure_counter_mhz
>  static cycles_t __measure_counter_hz(void)
>  {
>  	struct timeval tvs, tve;
> @@ -273,6 +284,7 @@ static unsigned int measure_counter_mhz(void)
>  
>  	return (unsigned int) (m / 1000000);
>  }
> +#endif
>  
>  static void thread_init(struct thread *t)
>  {
> -- 
> 2.31.1
> 
> 
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz

[John Kacur]

On Mon, 13 Sep 2021, Peter Xu wrote:

> On Mon, Sep 13, 2021 at 09:35:29AM -0400, John Kacur wrote:
> > Waiting for a Signed-off-by: from Peter Xu before I integrate these.
> 
> All 3 patches:
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> Thanks,
> 
> -- 
> Peter Xu
> 
> 

Pushed upstream

Re: [PATCH v2 2/3] oslat: Add aarch64 support

[Punit Agrawal]

Hi Nicolas,

Apologies for the late comment. I just caught up with the thread.

Nicolas Saenz Julienne <nsaenzju@redhat.com> writes:

> The callbacks are based on Linux's implementation:
>  - CNTVCT_EL0 provides direct access to the system virtual timer[1].
>  - 'yield' serves as a CPU hint with similar semantics as x86's
>    'pause'[2].
>
> In contrast with the kernel's implementation, there isn't a need for
> isb() after reading CNTVCT_EL0,  this is only needed in-kernel as the
> register read has to be ordered with a subsequent locking operation.
>
> [1] See Linux's '__arch_get_hw_counter()' in arch/arm64/include/asm/vdso/gettimeofday.h
> [2] See Linux's 1baa82f4803 ("arm64: Implement cpu_relax as yield").
> Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> --
>
> Changes since v1:
>  - Code cleanup
>  - Add compiler barriers
>
>  src/oslat/oslat.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index 33cccd3..c90ec1a 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -71,6 +71,19 @@ static inline void frc(uint64_t *pval)
>  {
>  	__asm__ __volatile__("mfspr %0, 268\n" : "=r" (*pval));
>  }
> +# elif defined(__aarch64__)
> +#  define relax()          __asm__ __volatile("yield" : : : "memory")
> +
> +static inline void frc(uint64_t *pval)
> +{
> +	/*
> +	 * This isb() is required to prevent that the counter value
> +	 * is speculated.
> +	 */
> +	__asm__ __volatile__("isb" : : : "memory");
> +	__asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (*pval) :: "memory");
> +

Although the isb() ensures completion of instructions before the counter
is read, I think there is still the problem of speculative execution of
instructions after the counter read being moved forward. See the
examples in Arm ARM DDI 0487F.b Section D11.2.2 "The Virtual counter"

So from my understanding the problem would be something like below -

isb()
...  <-  speculatively executed instructions from after the counter read
mrs %0, cntvct_el0

This would skew the counter value to a later point than what is intended
- a following isb() would address the issue.

> +}
>  # else
>  #  define relax()          do { } while (0)
>  #  define frc(x)

Re: [PATCH v2 2/3] oslat: Add aarch64 support

[nsaenzju]

On Tue, 2021-09-14 at 10:52 +0900, Punit Agrawal wrote:
> Hi Nicolas,
> 
> Apologies for the late comment. I just caught up with the thread.

No worries, thanks for the input!

[...]

> > +static inline void frc(uint64_t *pval)
> > +{
> > +	/*
> > +	 * This isb() is required to prevent that the counter value
> > +	 * is speculated.
> > +	 */
> > +	__asm__ __volatile__("isb" : : : "memory");
> > +	__asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (*pval) :: "memory");
> > +
> 
> Although the isb() ensures completion of instructions before the counter
> is read, I think there is still the problem of speculative execution of
> instructions after the counter read being moved forward. See the
> examples in Arm ARM DDI 0487F.b Section D11.2.2 "The Virtual counter"
> 
> So from my understanding the problem would be something like below -
> 
> isb()
> ...  <-  speculatively executed instructions from after the counter read
> mrs %0, cntvct_el0
> 
> This would skew the counter value to a later point than what is intended
> - a following isb() would address the issue.

For the record, here's what the Arm ARM states:

  Accesses to memory appearing in program order after the read of the counter
  are executed before the counter has been read. [...]

  To ensure that a memory access only occurs after a read of the counter, the
  following sequence should be used:

    MRS Xn, CNTVCT_EL0
    ISB
    LDR Xa, [Xb] ; this load will be executed after the timer has been read"

As stated in the commit description, I made sure the program logic can't suffer
from this. I hadn't thought of the timing angle though. I doubt we'll see any
difference, given we have a 1us granularity, but I don't mind adding it for the
sake of correctness.

@John Should I send a v3 of the series or a separate patch adding the extra
isb().

-- 
Nicolás Sáenz

Re: [PATCH v2 2/3] oslat: Add aarch64 support

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]



On Tue, 14 Sep 2021, nsaenzju@redhat.com wrote:

> On Tue, 2021-09-14 at 10:52 +0900, Punit Agrawal wrote:
> > Hi Nicolas,
> > 
> > Apologies for the late comment. I just caught up with the thread.
> 
> No worries, thanks for the input!
> 
> [...]
> 
> > > +static inline void frc(uint64_t *pval)
> > > +{
> > > +	/*
> > > +	 * This isb() is required to prevent that the counter value
> > > +	 * is speculated.
> > > +	 */
> > > +	__asm__ __volatile__("isb" : : : "memory");
> > > +	__asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (*pval) :: "memory");
> > > +
> > 
> > Although the isb() ensures completion of instructions before the counter
> > is read, I think there is still the problem of speculative execution of
> > instructions after the counter read being moved forward. See the
> > examples in Arm ARM DDI 0487F.b Section D11.2.2 "The Virtual counter"
> > 
> > So from my understanding the problem would be something like below -
> > 
> > isb()
> > ...  <-  speculatively executed instructions from after the counter read
> > mrs %0, cntvct_el0
> > 
> > This would skew the counter value to a later point than what is intended
> > - a following isb() would address the issue.
> 
> For the record, here's what the Arm ARM states:
> 
>   Accesses to memory appearing in program order after the read of the counter
>   are executed before the counter has been read. [...]
> 
>   To ensure that a memory access only occurs after a read of the counter, the
>   following sequence should be used:
> 
>     MRS Xn, CNTVCT_EL0
>     ISB
>     LDR Xa, [Xb] ; this load will be executed after the timer has been read"
> 
> As stated in the commit description, I made sure the program logic can't suffer
> from this. I hadn't thought of the timing angle though. I doubt we'll see any
> difference, given we have a 1us granularity, but I don't mind adding it for the
> sake of correctness.
> 
> @John Should I send a v3 of the series or a separate patch adding the extra
> isb().
> 
> -- 
> Nicolás Sáenz
> 
> 

I already integrated and pushed your v2 patch, so you should pull the code 
from upstream and then create a new patch to add the isb()

Thanks

John

Re: [PATCH v2 2/3] oslat: Add aarch64 support

[Punit Agrawal]

nsaenzju@redhat.com writes:

> On Tue, 2021-09-14 at 10:52 +0900, Punit Agrawal wrote:
>> Hi Nicolas,
>> 
>> Apologies for the late comment. I just caught up with the thread.
>
> No worries, thanks for the input!
>
> [...]
>
>> > +static inline void frc(uint64_t *pval)
>> > +{
>> > +	/*
>> > +	 * This isb() is required to prevent that the counter value
>> > +	 * is speculated.
>> > +	 */
>> > +	__asm__ __volatile__("isb" : : : "memory");
>> > +	__asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (*pval) :: "memory");
>> > +
>> 
>> Although the isb() ensures completion of instructions before the counter
>> is read, I think there is still the problem of speculative execution of
>> instructions after the counter read being moved forward. See the
>> examples in Arm ARM DDI 0487F.b Section D11.2.2 "The Virtual counter"
>> 
>> So from my understanding the problem would be something like below -
>> 
>> isb()
>> ...  <-  speculatively executed instructions from after the counter read
>> mrs %0, cntvct_el0
>> 
>> This would skew the counter value to a later point than what is intended
>> - a following isb() would address the issue.
>
> For the record, here's what the Arm ARM states:
>
>   Accesses to memory appearing in program order after the read of the counter
>   are executed before the counter has been read. [...]
>
>   To ensure that a memory access only occurs after a read of the counter, the
>   following sequence should be used:
>
>     MRS Xn, CNTVCT_EL0
>     ISB
>     LDR Xa, [Xb] ; this load will be executed after the timer has been read"
>
> As stated in the commit description, I made sure the program logic can't suffer
> from this. I hadn't thought of the timing angle though. I doubt we'll see any
> difference, given we have a 1us granularity, but I don't mind adding it for the
> sake of correctness.

Thanks - indeed that's the part I was referring to. Indeed the impact
will likely be low or none here but thought I'd mention the issue since
I noticed it while going through the patch.

Punit

[...]