* [PATCH v2 2/3] oslat: Add aarch64 support
2021-09-13 8:39 [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz Nicolas Saenz Julienne
@ 2021-09-13 8:39 ` Nicolas Saenz Julienne
2021-09-13 18:38 ` John Kacur
2021-09-14 1:52 ` Punit Agrawal
2021-09-13 8:39 ` [PATCH v2 3/3] oslat: Allow for arch specific counter frequency measurements Nicolas Saenz Julienne
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Nicolas Saenz Julienne @ 2021-09-13 8:39 UTC (permalink / raw)
To: linux-rt-users, peterx; +Cc: williams, jkacur, nsaenzju
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/3] oslat: Add aarch64 support
2021-09-13 8:39 ` [PATCH v2 2/3] oslat: Add aarch64 support Nicolas Saenz Julienne
@ 2021-09-13 18:38 ` John Kacur
2021-09-14 1:52 ` Punit Agrawal
1 sibling, 0 replies; 13+ messages in thread
From: John Kacur @ 2021-09-13 18:38 UTC (permalink / raw)
To: Nicolas Saenz Julienne; +Cc: linux-rt-users, peterx, williams
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>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/3] oslat: Add aarch64 support
2021-09-13 8:39 ` [PATCH v2 2/3] oslat: Add aarch64 support Nicolas Saenz Julienne
2021-09-13 18:38 ` John Kacur
@ 2021-09-14 1:52 ` Punit Agrawal
2021-09-14 10:16 ` nsaenzju
1 sibling, 1 reply; 13+ messages in thread
From: Punit Agrawal @ 2021-09-14 1:52 UTC (permalink / raw)
To: Nicolas Saenz Julienne; +Cc: linux-rt-users, peterx, williams, jkacur
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)
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/3] oslat: Add aarch64 support
2021-09-14 1:52 ` Punit Agrawal
@ 2021-09-14 10:16 ` nsaenzju
2021-09-14 12:48 ` John Kacur
2021-09-15 1:52 ` Punit Agrawal
0 siblings, 2 replies; 13+ messages in thread
From: nsaenzju @ 2021-09-14 10:16 UTC (permalink / raw)
To: Punit Agrawal, jkacur, Peter Xu; +Cc: linux-rt-users, peterx, williams, jkacur
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/3] oslat: Add aarch64 support
2021-09-14 10:16 ` nsaenzju
@ 2021-09-14 12:48 ` John Kacur
2021-09-15 1:52 ` Punit Agrawal
1 sibling, 0 replies; 13+ messages in thread
From: John Kacur @ 2021-09-14 12:48 UTC (permalink / raw)
To: nsaenzju; +Cc: Punit Agrawal, Peter Xu, linux-rt-users, williams
[-- 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/3] oslat: Add aarch64 support
2021-09-14 10:16 ` nsaenzju
2021-09-14 12:48 ` John Kacur
@ 2021-09-15 1:52 ` Punit Agrawal
1 sibling, 0 replies; 13+ messages in thread
From: Punit Agrawal @ 2021-09-15 1:52 UTC (permalink / raw)
To: nsaenzju; +Cc: jkacur, Peter Xu, linux-rt-users, williams
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
[...]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] oslat: Allow for arch specific counter frequency measurements
2021-09-13 8:39 [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz Nicolas Saenz Julienne
2021-09-13 8:39 ` [PATCH v2 2/3] oslat: Add aarch64 support Nicolas Saenz Julienne
@ 2021-09-13 8:39 ` Nicolas Saenz Julienne
2021-09-13 18:39 ` John Kacur
2021-09-13 13:35 ` [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz John Kacur
2021-09-13 18:38 ` John Kacur
3 siblings, 1 reply; 13+ messages in thread
From: Nicolas Saenz Julienne @ 2021-09-13 8:39 UTC (permalink / raw)
To: linux-rt-users, peterx; +Cc: williams, jkacur, nsaenzju
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] oslat: Allow for arch specific counter frequency measurements
2021-09-13 8:39 ` [PATCH v2 3/3] oslat: Allow for arch specific counter frequency measurements Nicolas Saenz Julienne
@ 2021-09-13 18:39 ` John Kacur
0 siblings, 0 replies; 13+ messages in thread
From: John Kacur @ 2021-09-13 18:39 UTC (permalink / raw)
To: Nicolas Saenz Julienne; +Cc: linux-rt-users, peterx, williams
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz
2021-09-13 8:39 [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz Nicolas Saenz Julienne
2021-09-13 8:39 ` [PATCH v2 2/3] oslat: Add aarch64 support Nicolas Saenz Julienne
2021-09-13 8:39 ` [PATCH v2 3/3] oslat: Allow for arch specific counter frequency measurements Nicolas Saenz Julienne
@ 2021-09-13 13:35 ` John Kacur
2021-09-13 14:57 ` Peter Xu
2021-09-13 18:38 ` John Kacur
3 siblings, 1 reply; 13+ messages in thread
From: John Kacur @ 2021-09-13 13:35 UTC (permalink / raw)
To: Nicolas Saenz Julienne; +Cc: linux-rt-users, peterx, williams
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz
2021-09-13 13:35 ` [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz John Kacur
@ 2021-09-13 14:57 ` Peter Xu
2021-09-13 18:39 ` John Kacur
0 siblings, 1 reply; 13+ messages in thread
From: Peter Xu @ 2021-09-13 14:57 UTC (permalink / raw)
To: John Kacur; +Cc: Nicolas Saenz Julienne, linux-rt-users, williams
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz
2021-09-13 14:57 ` Peter Xu
@ 2021-09-13 18:39 ` John Kacur
0 siblings, 0 replies; 13+ messages in thread
From: John Kacur @ 2021-09-13 18:39 UTC (permalink / raw)
To: Peter Xu; +Cc: Nicolas Saenz Julienne, linux-rt-users, williams
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz
2021-09-13 8:39 [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz Nicolas Saenz Julienne
` (2 preceding siblings ...)
2021-09-13 13:35 ` [PATCH v2 1/3] oslat: Rename cpu_mhz/cpu_hz to counter_mhz/counter_hz John Kacur
@ 2021-09-13 18:38 ` John Kacur
3 siblings, 0 replies; 13+ messages in thread
From: John Kacur @ 2021-09-13 18:38 UTC (permalink / raw)
To: Nicolas Saenz Julienne; +Cc: linux-rt-users, peterx, williams
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>
^ permalink raw reply [flat|nested] 13+ messages in thread