[PATCH] arm64: perf_event: Fix time_offset for arch timer

[PATCH] arm64: perf_event: Fix time_offset for arch timer

[Leo Yan]

Between the system powering on and kernel's sched clock registration,
the arch timer usually has been enabled at the early time and its
counter is incremented during the period of the booting up.  Thus the
arch timer's counter is not completely accounted into the sched clock,
and has a delta between the arch timer's counter and sched clock.  This
delta value should be stored into userpg->time_offset, which later can
be retrieved by Perf tool in the user space for sample timestamp
calculation.

Now userpg->time_offset is assigned to the negative sched clock with
'-now', this value cannot reflect the delta between arch timer's counter
and sched clock, so Perf cannot use it to calculate the sample time.

To fix this issue, this patch calculate the delta between the arch
timer's and sched clock and assign the delta to userpg->time_offset.
The detailed steps are firstly to convert counter to nanoseconds 'ns',
then the offset is calculated as 'now' minus 'ns'.

        |<------------------- 'ns' ---------------------->|
                                |<-------- 'now' -------->|
        |<---- time_offset ---->|
        |-----------------------|-------------------------|
        ^                       ^                         ^
  Power on system     sched clock registration      Perf starts

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/kernel/perf_event.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index e40b65645c86..226d25d77072 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -1143,6 +1143,7 @@ void arch_perf_update_userpage(struct perf_event *event,
 {
 	u32 freq;
 	u32 shift;
+	u64 count, ns, quot, rem;
 
 	/*
 	 * Internal timekeeping for enabled/running/stopped times
@@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
 		userpg->time_mult >>= 1;
 	}
 	userpg->time_shift = (u16)shift;
-	userpg->time_offset = -now;
+
+	/*
+	 * Since arch timer is enabled ealier than sched clock registration,
+	 * compuate the delta (in nanosecond unit) between the arch timer
+	 * counter and sched clock, assign the delta to time_offset and
+	 * perf tool can use it for timestamp calculation.
+	 *
+	 * The formula for conversion arch timer cycle to ns is:
+	 *   quot = (cyc >> time_shift);
+	 *   rem  = cyc & ((1 << time_shift) - 1);
+	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
+	 */
+	count = arch_timer_read_counter();
+	quot = count >> shift;
+	rem = count & ((1 << shift) - 1);
+	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
+	userpg->time_offset = now - ns;
 }
-- 
2.17.1

[PATCH] arm64: perf_event: Fix time_offset for arch timer

[Leo Yan]

Between the system powering on and kernel's sched clock registration,
the arch timer usually has been enabled at the early time and its
counter is incremented during the period of the booting up.  Thus the
arch timer's counter is not completely accounted into the sched clock,
and has a delta between the arch timer's counter and sched clock.  This
delta value should be stored into userpg->time_offset, which later can
be retrieved by Perf tool in the user space for sample timestamp
calculation.

Now userpg->time_offset is assigned to the negative sched clock with
'-now', this value cannot reflect the delta between arch timer's counter
and sched clock, so Perf cannot use it to calculate the sample time.

To fix this issue, this patch calculate the delta between the arch
timer's and sched clock and assign the delta to userpg->time_offset.
The detailed steps are firstly to convert counter to nanoseconds 'ns',
then the offset is calculated as 'now' minus 'ns'.

        |<------------------- 'ns' ---------------------->|
                                |<-------- 'now' -------->|
        |<---- time_offset ---->|
        |-----------------------|-------------------------|
        ^                       ^                         ^
  Power on system     sched clock registration      Perf starts

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/kernel/perf_event.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index e40b65645c86..226d25d77072 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -1143,6 +1143,7 @@ void arch_perf_update_userpage(struct perf_event *event,
 {
 	u32 freq;
 	u32 shift;
+	u64 count, ns, quot, rem;
 
 	/*
 	 * Internal timekeeping for enabled/running/stopped times
@@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
 		userpg->time_mult >>= 1;
 	}
 	userpg->time_shift = (u16)shift;
-	userpg->time_offset = -now;
+
+	/*
+	 * Since arch timer is enabled ealier than sched clock registration,
+	 * compuate the delta (in nanosecond unit) between the arch timer
+	 * counter and sched clock, assign the delta to time_offset and
+	 * perf tool can use it for timestamp calculation.
+	 *
+	 * The formula for conversion arch timer cycle to ns is:
+	 *   quot = (cyc >> time_shift);
+	 *   rem  = cyc & ((1 << time_shift) - 1);
+	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
+	 */
+	count = arch_timer_read_counter();
+	quot = count >> shift;
+	rem = count & ((1 << shift) - 1);
+	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
+	userpg->time_offset = now - ns;
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Leo Yan]

On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:
> Between the system powering on and kernel's sched clock registration,
> the arch timer usually has been enabled at the early time and its
> counter is incremented during the period of the booting up.  Thus the
> arch timer's counter is not completely accounted into the sched clock,
> and has a delta between the arch timer's counter and sched clock.  This
> delta value should be stored into userpg->time_offset, which later can
> be retrieved by Perf tool in the user space for sample timestamp
> calculation.
> 
> Now userpg->time_offset is assigned to the negative sched clock with
> '-now', this value cannot reflect the delta between arch timer's counter
> and sched clock, so Perf cannot use it to calculate the sample time.
> 
> To fix this issue, this patch calculate the delta between the arch
> timer's and sched clock and assign the delta to userpg->time_offset.
> The detailed steps are firstly to convert counter to nanoseconds 'ns',
> then the offset is calculated as 'now' minus 'ns'.
> 
>         |<------------------- 'ns' ---------------------->|
>                                 |<-------- 'now' -------->|
>         |<---- time_offset ---->|
>         |-----------------------|-------------------------|
>         ^                       ^                         ^
>   Power on system     sched clock registration      Perf starts
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/kernel/perf_event.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

Gentle ping ...

Hi Mike R., Peter,

If possible, could you give a look for this patch?

Thank you,
Leo

> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index e40b65645c86..226d25d77072 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -1143,6 +1143,7 @@ void arch_perf_update_userpage(struct perf_event *event,
>  {
>  	u32 freq;
>  	u32 shift;
> +	u64 count, ns, quot, rem;
>  
>  	/*
>  	 * Internal timekeeping for enabled/running/stopped times
> @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
>  		userpg->time_mult >>= 1;
>  	}
>  	userpg->time_shift = (u16)shift;
> -	userpg->time_offset = -now;
> +
> +	/*
> +	 * Since arch timer is enabled ealier than sched clock registration,
> +	 * compuate the delta (in nanosecond unit) between the arch timer
> +	 * counter and sched clock, assign the delta to time_offset and
> +	 * perf tool can use it for timestamp calculation.
> +	 *
> +	 * The formula for conversion arch timer cycle to ns is:
> +	 *   quot = (cyc >> time_shift);
> +	 *   rem  = cyc & ((1 << time_shift) - 1);
> +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> +	 */
> +	count = arch_timer_read_counter();
> +	quot = count >> shift;
> +	rem = count & ((1 << shift) - 1);
> +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> +	userpg->time_offset = now - ns;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Leo Yan]

On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:
> Between the system powering on and kernel's sched clock registration,
> the arch timer usually has been enabled at the early time and its
> counter is incremented during the period of the booting up.  Thus the
> arch timer's counter is not completely accounted into the sched clock,
> and has a delta between the arch timer's counter and sched clock.  This
> delta value should be stored into userpg->time_offset, which later can
> be retrieved by Perf tool in the user space for sample timestamp
> calculation.
> 
> Now userpg->time_offset is assigned to the negative sched clock with
> '-now', this value cannot reflect the delta between arch timer's counter
> and sched clock, so Perf cannot use it to calculate the sample time.
> 
> To fix this issue, this patch calculate the delta between the arch
> timer's and sched clock and assign the delta to userpg->time_offset.
> The detailed steps are firstly to convert counter to nanoseconds 'ns',
> then the offset is calculated as 'now' minus 'ns'.
> 
>         |<------------------- 'ns' ---------------------->|
>                                 |<-------- 'now' -------->|
>         |<---- time_offset ---->|
>         |-----------------------|-------------------------|
>         ^                       ^                         ^
>   Power on system     sched clock registration      Perf starts
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/kernel/perf_event.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

Gentle ping ...

Hi Mike R., Peter,

If possible, could you give a look for this patch?

Thank you,
Leo

> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index e40b65645c86..226d25d77072 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -1143,6 +1143,7 @@ void arch_perf_update_userpage(struct perf_event *event,
>  {
>  	u32 freq;
>  	u32 shift;
> +	u64 count, ns, quot, rem;
>  
>  	/*
>  	 * Internal timekeeping for enabled/running/stopped times
> @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
>  		userpg->time_mult >>= 1;
>  	}
>  	userpg->time_shift = (u16)shift;
> -	userpg->time_offset = -now;
> +
> +	/*
> +	 * Since arch timer is enabled ealier than sched clock registration,
> +	 * compuate the delta (in nanosecond unit) between the arch timer
> +	 * counter and sched clock, assign the delta to time_offset and
> +	 * perf tool can use it for timestamp calculation.
> +	 *
> +	 * The formula for conversion arch timer cycle to ns is:
> +	 *   quot = (cyc >> time_shift);
> +	 *   rem  = cyc & ((1 << time_shift) - 1);
> +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> +	 */
> +	count = arch_timer_read_counter();
> +	quot = count >> shift;
> +	rem = count & ((1 << shift) - 1);
> +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> +	userpg->time_offset = now - ns;
>  }
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

Hi Leo,

[+Maz and tglx in case I'm barking up the wrong tree]

On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:
> Between the system powering on and kernel's sched clock registration,
> the arch timer usually has been enabled at the early time and its
> counter is incremented during the period of the booting up.  Thus the
> arch timer's counter is not completely accounted into the sched clock,
> and has a delta between the arch timer's counter and sched clock.  This
> delta value should be stored into userpg->time_offset, which later can
> be retrieved by Perf tool in the user space for sample timestamp
> calculation.
> 
> Now userpg->time_offset is assigned to the negative sched clock with
> '-now', this value cannot reflect the delta between arch timer's counter
> and sched clock, so Perf cannot use it to calculate the sample time.
> 
> To fix this issue, this patch calculate the delta between the arch
> timer's and sched clock and assign the delta to userpg->time_offset.
> The detailed steps are firstly to convert counter to nanoseconds 'ns',
> then the offset is calculated as 'now' minus 'ns'.
> 
>         |<------------------- 'ns' ---------------------->|
>                                 |<-------- 'now' -------->|
>         |<---- time_offset ---->|
>         |-----------------------|-------------------------|
>         ^                       ^                         ^
>   Power on system     sched clock registration      Perf starts

FWIW, I'm /really/ struggling to understand the problem here.

If I've grokked it correctly (big 'if'), then you can't just factor in
what you call "time_offset" in the diagram above, because there isn't
a guarantee that the counter is zero-initialised at the start.

> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/kernel/perf_event.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index e40b65645c86..226d25d77072 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -1143,6 +1143,7 @@ void arch_perf_update_userpage(struct perf_event *event,
>  {
>  	u32 freq;
>  	u32 shift;
> +	u64 count, ns, quot, rem;
>  
>  	/*
>  	 * Internal timekeeping for enabled/running/stopped times
> @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
>  		userpg->time_mult >>= 1;
>  	}
>  	userpg->time_shift = (u16)shift;
> -	userpg->time_offset = -now;
> +
> +	/*
> +	 * Since arch timer is enabled ealier than sched clock registration,
> +	 * compuate the delta (in nanosecond unit) between the arch timer
> +	 * counter and sched clock, assign the delta to time_offset and
> +	 * perf tool can use it for timestamp calculation.
> +	 *
> +	 * The formula for conversion arch timer cycle to ns is:
> +	 *   quot = (cyc >> time_shift);
> +	 *   rem  = cyc & ((1 << time_shift) - 1);
> +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> +	 */
> +	count = arch_timer_read_counter();
> +	quot = count >> shift;
> +	rem = count & ((1 << shift) - 1);
> +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> +	userpg->time_offset = now - ns;

Hmm, reading the counter and calculating the delta feels horribly
approximate to me. It would be much better if we could get hold of the
initial epoch cycles from the point at which sched_clock was initialised
using the counter. This represents the true cycle delta between the counter
and what sched_clock uses for 0 ns.

Unfortunately, I can't see a straightforward way to grab that information.
It looks like x86 pulls this directly from the TSC driver.

Will

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

Hi Leo,

[+Maz and tglx in case I'm barking up the wrong tree]

On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:
> Between the system powering on and kernel's sched clock registration,
> the arch timer usually has been enabled at the early time and its
> counter is incremented during the period of the booting up.  Thus the
> arch timer's counter is not completely accounted into the sched clock,
> and has a delta between the arch timer's counter and sched clock.  This
> delta value should be stored into userpg->time_offset, which later can
> be retrieved by Perf tool in the user space for sample timestamp
> calculation.
> 
> Now userpg->time_offset is assigned to the negative sched clock with
> '-now', this value cannot reflect the delta between arch timer's counter
> and sched clock, so Perf cannot use it to calculate the sample time.
> 
> To fix this issue, this patch calculate the delta between the arch
> timer's and sched clock and assign the delta to userpg->time_offset.
> The detailed steps are firstly to convert counter to nanoseconds 'ns',
> then the offset is calculated as 'now' minus 'ns'.
> 
>         |<------------------- 'ns' ---------------------->|
>                                 |<-------- 'now' -------->|
>         |<---- time_offset ---->|
>         |-----------------------|-------------------------|
>         ^                       ^                         ^
>   Power on system     sched clock registration      Perf starts

FWIW, I'm /really/ struggling to understand the problem here.

If I've grokked it correctly (big 'if'), then you can't just factor in
what you call "time_offset" in the diagram above, because there isn't
a guarantee that the counter is zero-initialised at the start.

> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/kernel/perf_event.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index e40b65645c86..226d25d77072 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -1143,6 +1143,7 @@ void arch_perf_update_userpage(struct perf_event *event,
>  {
>  	u32 freq;
>  	u32 shift;
> +	u64 count, ns, quot, rem;
>  
>  	/*
>  	 * Internal timekeeping for enabled/running/stopped times
> @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
>  		userpg->time_mult >>= 1;
>  	}
>  	userpg->time_shift = (u16)shift;
> -	userpg->time_offset = -now;
> +
> +	/*
> +	 * Since arch timer is enabled ealier than sched clock registration,
> +	 * compuate the delta (in nanosecond unit) between the arch timer
> +	 * counter and sched clock, assign the delta to time_offset and
> +	 * perf tool can use it for timestamp calculation.
> +	 *
> +	 * The formula for conversion arch timer cycle to ns is:
> +	 *   quot = (cyc >> time_shift);
> +	 *   rem  = cyc & ((1 << time_shift) - 1);
> +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> +	 */
> +	count = arch_timer_read_counter();
> +	quot = count >> shift;
> +	rem = count & ((1 << shift) - 1);
> +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> +	userpg->time_offset = now - ns;

Hmm, reading the counter and calculating the delta feels horribly
approximate to me. It would be much better if we could get hold of the
initial epoch cycles from the point at which sched_clock was initialised
using the counter. This represents the true cycle delta between the counter
and what sched_clock uses for 0 ns.

Unfortunately, I can't see a straightforward way to grab that information.
It looks like x86 pulls this directly from the TSC driver.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Marc Zyngier]

On 2020-04-30 15:58, Will Deacon wrote:
> Hi Leo,
> 
> [+Maz and tglx in case I'm barking up the wrong tree]
> 
> On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:
>> Between the system powering on and kernel's sched clock registration,
>> the arch timer usually has been enabled at the early time and its
>> counter is incremented during the period of the booting up.  Thus the
>> arch timer's counter is not completely accounted into the sched clock,
>> and has a delta between the arch timer's counter and sched clock.  
>> This
>> delta value should be stored into userpg->time_offset, which later can
>> be retrieved by Perf tool in the user space for sample timestamp
>> calculation.
>> 
>> Now userpg->time_offset is assigned to the negative sched clock with
>> '-now', this value cannot reflect the delta between arch timer's 
>> counter
>> and sched clock, so Perf cannot use it to calculate the sample time.
>> 
>> To fix this issue, this patch calculate the delta between the arch
>> timer's and sched clock and assign the delta to userpg->time_offset.
>> The detailed steps are firstly to convert counter to nanoseconds 'ns',
>> then the offset is calculated as 'now' minus 'ns'.
>> 
>>         |<------------------- 'ns' ---------------------->|
>>                                 |<-------- 'now' -------->|
>>         |<---- time_offset ---->|
>>         |-----------------------|-------------------------|
>>         ^                       ^                         ^
>>   Power on system     sched clock registration      Perf starts
> 
> FWIW, I'm /really/ struggling to understand the problem here.
> 
> If I've grokked it correctly (big 'if'), then you can't just factor in
> what you call "time_offset" in the diagram above, because there isn't
> a guarantee that the counter is zero-initialised at the start.

Even if it was, we have no idea of *when* that was. Think kexec, for a
start. Or spending some variable in firmware because of $REASON.

> 
>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> ---
>>  arch/arm64/kernel/perf_event.c | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm64/kernel/perf_event.c 
>> b/arch/arm64/kernel/perf_event.c
>> index e40b65645c86..226d25d77072 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -1143,6 +1143,7 @@ void arch_perf_update_userpage(struct perf_event 
>> *event,
>>  {
>>  	u32 freq;
>>  	u32 shift;
>> +	u64 count, ns, quot, rem;
>> 
>>  	/*
>>  	 * Internal timekeeping for enabled/running/stopped times
>> @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct 
>> perf_event *event,
>>  		userpg->time_mult >>= 1;
>>  	}
>>  	userpg->time_shift = (u16)shift;
>> -	userpg->time_offset = -now;
>> +
>> +	/*
>> +	 * Since arch timer is enabled ealier than sched clock registration,
>> +	 * compuate the delta (in nanosecond unit) between the arch timer
>> +	 * counter and sched clock, assign the delta to time_offset and
>> +	 * perf tool can use it for timestamp calculation.
>> +	 *
>> +	 * The formula for conversion arch timer cycle to ns is:
>> +	 *   quot = (cyc >> time_shift);
>> +	 *   rem  = cyc & ((1 << time_shift) - 1);
>> +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
>> +	 */
>> +	count = arch_timer_read_counter();
>> +	quot = count >> shift;
>> +	rem = count & ((1 << shift) - 1);
>> +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> 
>> shift);
>> +	userpg->time_offset = now - ns;
> 
> Hmm, reading the counter and calculating the delta feels horribly
> approximate to me. It would be much better if we could get hold of the
> initial epoch cycles from the point at which sched_clock was 
> initialised
> using the counter. This represents the true cycle delta between the 
> counter
> and what sched_clock uses for 0 ns.

I think this is a sensible solution if you want an epoch that starts at 
0 with
sched_clock being initialized. The other question is whether it is 
possible to
use a different timestamping source for perf that wouldn't need to be 
offset.

> Unfortunately, I can't see a straightforward way to grab that 
> information.
> It looks like x86 pulls this directly from the TSC driver.

I wonder if we could/should make __sched_clock_offset available even 
when
CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
help with this particular can or worm...

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Marc Zyngier]

On 2020-04-30 15:58, Will Deacon wrote:
> Hi Leo,
> 
> [+Maz and tglx in case I'm barking up the wrong tree]
> 
> On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:
>> Between the system powering on and kernel's sched clock registration,
>> the arch timer usually has been enabled at the early time and its
>> counter is incremented during the period of the booting up.  Thus the
>> arch timer's counter is not completely accounted into the sched clock,
>> and has a delta between the arch timer's counter and sched clock.  
>> This
>> delta value should be stored into userpg->time_offset, which later can
>> be retrieved by Perf tool in the user space for sample timestamp
>> calculation.
>> 
>> Now userpg->time_offset is assigned to the negative sched clock with
>> '-now', this value cannot reflect the delta between arch timer's 
>> counter
>> and sched clock, so Perf cannot use it to calculate the sample time.
>> 
>> To fix this issue, this patch calculate the delta between the arch
>> timer's and sched clock and assign the delta to userpg->time_offset.
>> The detailed steps are firstly to convert counter to nanoseconds 'ns',
>> then the offset is calculated as 'now' minus 'ns'.
>> 
>>         |<------------------- 'ns' ---------------------->|
>>                                 |<-------- 'now' -------->|
>>         |<---- time_offset ---->|
>>         |-----------------------|-------------------------|
>>         ^                       ^                         ^
>>   Power on system     sched clock registration      Perf starts
> 
> FWIW, I'm /really/ struggling to understand the problem here.
> 
> If I've grokked it correctly (big 'if'), then you can't just factor in
> what you call "time_offset" in the diagram above, because there isn't
> a guarantee that the counter is zero-initialised at the start.

Even if it was, we have no idea of *when* that was. Think kexec, for a
start. Or spending some variable in firmware because of $REASON.

> 
>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> ---
>>  arch/arm64/kernel/perf_event.c | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm64/kernel/perf_event.c 
>> b/arch/arm64/kernel/perf_event.c
>> index e40b65645c86..226d25d77072 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -1143,6 +1143,7 @@ void arch_perf_update_userpage(struct perf_event 
>> *event,
>>  {
>>  	u32 freq;
>>  	u32 shift;
>> +	u64 count, ns, quot, rem;
>> 
>>  	/*
>>  	 * Internal timekeeping for enabled/running/stopped times
>> @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct 
>> perf_event *event,
>>  		userpg->time_mult >>= 1;
>>  	}
>>  	userpg->time_shift = (u16)shift;
>> -	userpg->time_offset = -now;
>> +
>> +	/*
>> +	 * Since arch timer is enabled ealier than sched clock registration,
>> +	 * compuate the delta (in nanosecond unit) between the arch timer
>> +	 * counter and sched clock, assign the delta to time_offset and
>> +	 * perf tool can use it for timestamp calculation.
>> +	 *
>> +	 * The formula for conversion arch timer cycle to ns is:
>> +	 *   quot = (cyc >> time_shift);
>> +	 *   rem  = cyc & ((1 << time_shift) - 1);
>> +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
>> +	 */
>> +	count = arch_timer_read_counter();
>> +	quot = count >> shift;
>> +	rem = count & ((1 << shift) - 1);
>> +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> 
>> shift);
>> +	userpg->time_offset = now - ns;
> 
> Hmm, reading the counter and calculating the delta feels horribly
> approximate to me. It would be much better if we could get hold of the
> initial epoch cycles from the point at which sched_clock was 
> initialised
> using the counter. This represents the true cycle delta between the 
> counter
> and what sched_clock uses for 0 ns.

I think this is a sensible solution if you want an epoch that starts at 
0 with
sched_clock being initialized. The other question is whether it is 
possible to
use a different timestamping source for perf that wouldn't need to be 
offset.

> Unfortunately, I can't see a straightforward way to grab that 
> information.
> It looks like x86 pulls this directly from the TSC driver.

I wonder if we could/should make __sched_clock_offset available even 
when
CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
help with this particular can or worm...

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Peter Zijlstra]

On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:

> I wonder if we could/should make __sched_clock_offset available even when
> CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
> help with this particular can or worm...

Errrgh. __sched_clock_offset is only needed on x86 because we transition
from one clock device to another on boot. It really shouldn't exist on
anything sane.

Let me try and understand your particular problem better.

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Peter Zijlstra]

On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:

> I wonder if we could/should make __sched_clock_offset available even when
> CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
> help with this particular can or worm...

Errrgh. __sched_clock_offset is only needed on x86 because we transition
from one clock device to another on boot. It really shouldn't exist on
anything sane.

Let me try and understand your particular problem better.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
> 
> > I wonder if we could/should make __sched_clock_offset available even when
> > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
> > help with this particular can or worm...
> 
> Errrgh. __sched_clock_offset is only needed on x86 because we transition
> from one clock device to another on boot. It really shouldn't exist on
> anything sane.

I think we still transition from jiffies on arm64, because we don't register
with sched_clock until the timer driver probes. Marc, is that right?

> Let me try and understand your particular problem better.

I think the long and short of it is that userspace needs a way to convert
the raw counter cycles into a ns value that can be compared against values
coming out of sched_clock. To do this accurately, I think it needs the
cycles value at the point when sched_clock was initialised.

Will

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
> 
> > I wonder if we could/should make __sched_clock_offset available even when
> > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
> > help with this particular can or worm...
> 
> Errrgh. __sched_clock_offset is only needed on x86 because we transition
> from one clock device to another on boot. It really shouldn't exist on
> anything sane.

I think we still transition from jiffies on arm64, because we don't register
with sched_clock until the timer driver probes. Marc, is that right?

> Let me try and understand your particular problem better.

I think the long and short of it is that userspace needs a way to convert
the raw counter cycles into a ns value that can be compared against values
coming out of sched_clock. To do this accurately, I think it needs the
cycles value at the point when sched_clock was initialised.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Peter Zijlstra]

On Thu, Apr 30, 2020 at 03:58:24PM +0100, Will Deacon wrote:
> On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:

> > @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
> >  		userpg->time_mult >>= 1;
> >  	}
> >  	userpg->time_shift = (u16)shift;
> > -	userpg->time_offset = -now;
> > +
> > +	/*
> > +	 * Since arch timer is enabled ealier than sched clock registration,
> > +	 * compuate the delta (in nanosecond unit) between the arch timer
> > +	 * counter and sched clock, assign the delta to time_offset and
> > +	 * perf tool can use it for timestamp calculation.
> > +	 *
> > +	 * The formula for conversion arch timer cycle to ns is:
> > +	 *   quot = (cyc >> time_shift);
> > +	 *   rem  = cyc & ((1 << time_shift) - 1);
> > +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> > +	 */
> > +	count = arch_timer_read_counter();
> > +	quot = count >> shift;
> > +	rem = count & ((1 << shift) - 1);
> > +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> > +	userpg->time_offset = now - ns;
> 
> Hmm, reading the counter and calculating the delta feels horribly
> approximate to me. It would be much better if we could get hold of the
> initial epoch cycles from the point at which sched_clock was initialised
> using the counter. This represents the true cycle delta between the counter
> and what sched_clock uses for 0 ns.
> 
> Unfortunately, I can't see a straightforward way to grab that information.
> It looks like x86 pulls this directly from the TSC driver.

Yeah, and I'm thinking you should do the same. IIRC ARM uses this
kernel/time/sched_clock.c thing, and if I read that right, the struct
clock_data there has all the bits you need here.

So I'm thinking that you might want to add a helper function here to get
you the good stuff.

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Peter Zijlstra]

On Thu, Apr 30, 2020 at 03:58:24PM +0100, Will Deacon wrote:
> On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:

> > @@ -1164,5 +1165,21 @@ void arch_perf_update_userpage(struct perf_event *event,
> >  		userpg->time_mult >>= 1;
> >  	}
> >  	userpg->time_shift = (u16)shift;
> > -	userpg->time_offset = -now;
> > +
> > +	/*
> > +	 * Since arch timer is enabled ealier than sched clock registration,
> > +	 * compuate the delta (in nanosecond unit) between the arch timer
> > +	 * counter and sched clock, assign the delta to time_offset and
> > +	 * perf tool can use it for timestamp calculation.
> > +	 *
> > +	 * The formula for conversion arch timer cycle to ns is:
> > +	 *   quot = (cyc >> time_shift);
> > +	 *   rem  = cyc & ((1 << time_shift) - 1);
> > +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> > +	 */
> > +	count = arch_timer_read_counter();
> > +	quot = count >> shift;
> > +	rem = count & ((1 << shift) - 1);
> > +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> > +	userpg->time_offset = now - ns;
> 
> Hmm, reading the counter and calculating the delta feels horribly
> approximate to me. It would be much better if we could get hold of the
> initial epoch cycles from the point at which sched_clock was initialised
> using the counter. This represents the true cycle delta between the counter
> and what sched_clock uses for 0 ns.
> 
> Unfortunately, I can't see a straightforward way to grab that information.
> It looks like x86 pulls this directly from the TSC driver.

Yeah, and I'm thinking you should do the same. IIRC ARM uses this
kernel/time/sched_clock.c thing, and if I read that right, the struct
clock_data there has all the bits you need here.

So I'm thinking that you might want to add a helper function here to get
you the good stuff.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

On Thu, Apr 30, 2020 at 06:27:50PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 30, 2020 at 03:58:24PM +0100, Will Deacon wrote:
> > On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:
> > > +	/*
> > > +	 * Since arch timer is enabled ealier than sched clock registration,
> > > +	 * compuate the delta (in nanosecond unit) between the arch timer
> > > +	 * counter and sched clock, assign the delta to time_offset and
> > > +	 * perf tool can use it for timestamp calculation.
> > > +	 *
> > > +	 * The formula for conversion arch timer cycle to ns is:
> > > +	 *   quot = (cyc >> time_shift);
> > > +	 *   rem  = cyc & ((1 << time_shift) - 1);
> > > +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> > > +	 */
> > > +	count = arch_timer_read_counter();
> > > +	quot = count >> shift;
> > > +	rem = count & ((1 << shift) - 1);
> > > +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> > > +	userpg->time_offset = now - ns;
> > 
> > Hmm, reading the counter and calculating the delta feels horribly
> > approximate to me. It would be much better if we could get hold of the
> > initial epoch cycles from the point at which sched_clock was initialised
> > using the counter. This represents the true cycle delta between the counter
> > and what sched_clock uses for 0 ns.
> > 
> > Unfortunately, I can't see a straightforward way to grab that information.
> > It looks like x86 pulls this directly from the TSC driver.
> 
> Yeah, and I'm thinking you should do the same. IIRC ARM uses this
> kernel/time/sched_clock.c thing, and if I read that right, the struct
> clock_data there has all the bits you need here.
> 
> So I'm thinking that you might want to add a helper function here to get
> you the good stuff.

Thanks, Peter.

Leo -- do you think you could look at implementing this as part of a v2,
please?

Will

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

On Thu, Apr 30, 2020 at 06:27:50PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 30, 2020 at 03:58:24PM +0100, Will Deacon wrote:
> > On Fri, Mar 20, 2020 at 05:35:45PM +0800, Leo Yan wrote:
> > > +	/*
> > > +	 * Since arch timer is enabled ealier than sched clock registration,
> > > +	 * compuate the delta (in nanosecond unit) between the arch timer
> > > +	 * counter and sched clock, assign the delta to time_offset and
> > > +	 * perf tool can use it for timestamp calculation.
> > > +	 *
> > > +	 * The formula for conversion arch timer cycle to ns is:
> > > +	 *   quot = (cyc >> time_shift);
> > > +	 *   rem  = cyc & ((1 << time_shift) - 1);
> > > +	 *   ns   = quot * time_mult + ((rem * time_mult) >> time_shift);
> > > +	 */
> > > +	count = arch_timer_read_counter();
> > > +	quot = count >> shift;
> > > +	rem = count & ((1 << shift) - 1);
> > > +	ns = quot * userpg->time_mult + ((rem * userpg->time_mult) >> shift);
> > > +	userpg->time_offset = now - ns;
> > 
> > Hmm, reading the counter and calculating the delta feels horribly
> > approximate to me. It would be much better if we could get hold of the
> > initial epoch cycles from the point at which sched_clock was initialised
> > using the counter. This represents the true cycle delta between the counter
> > and what sched_clock uses for 0 ns.
> > 
> > Unfortunately, I can't see a straightforward way to grab that information.
> > It looks like x86 pulls this directly from the TSC driver.
> 
> Yeah, and I'm thinking you should do the same. IIRC ARM uses this
> kernel/time/sched_clock.c thing, and if I read that right, the struct
> clock_data there has all the bits you need here.
> 
> So I'm thinking that you might want to add a helper function here to get
> you the good stuff.

Thanks, Peter.

Leo -- do you think you could look at implementing this as part of a v2,
please?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Marc Zyngier]

On 2020-04-30 17:18, Will Deacon wrote:
> On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
>> 
>> > I wonder if we could/should make __sched_clock_offset available even when
>> > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
>> > help with this particular can or worm...
>> 
>> Errrgh. __sched_clock_offset is only needed on x86 because we 
>> transition
>> from one clock device to another on boot. It really shouldn't exist on
>> anything sane.
> 
> I think we still transition from jiffies on arm64, because we don't 
> register
> with sched_clock until the timer driver probes. Marc, is that right?

Indeed. The clocksource is only available relatively late, as we need to
discover the details of the platform and enable the various workarounds
(because nobody can get a simple 64bit counter right). So it is only at
that stage that we transition to it.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Marc Zyngier]

On 2020-04-30 17:18, Will Deacon wrote:
> On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
>> 
>> > I wonder if we could/should make __sched_clock_offset available even when
>> > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
>> > help with this particular can or worm...
>> 
>> Errrgh. __sched_clock_offset is only needed on x86 because we 
>> transition
>> from one clock device to another on boot. It really shouldn't exist on
>> anything sane.
> 
> I think we still transition from jiffies on arm64, because we don't 
> register
> with sched_clock until the timer driver probes. Marc, is that right?

Indeed. The clocksource is only available relatively late, as we need to
discover the details of the platform and enable the various workarounds
(because nobody can get a simple 64bit counter right). So it is only at
that stage that we transition to it.

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Leo Yan]

Hi all,

On Thu, Apr 30, 2020 at 05:18:15PM +0100, Will Deacon wrote:
> On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
> > 
> > > I wonder if we could/should make __sched_clock_offset available even when
> > > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
> > > help with this particular can or worm...
> > 
> > Errrgh. __sched_clock_offset is only needed on x86 because we transition
> > from one clock device to another on boot. It really shouldn't exist on
> > anything sane.
> 
> I think we still transition from jiffies on arm64, because we don't register
> with sched_clock until the timer driver probes. Marc, is that right?
> 
> > Let me try and understand your particular problem better.
> 
> I think the long and short of it is that userspace needs a way to convert
> the raw counter cycles into a ns value that can be compared against values
> coming out of sched_clock. To do this accurately, I think it needs the
> cycles value at the point when sched_clock was initialised.

Will's understanding is exactly what I want to resolve in this patch.

The background info is for the ARM SPE [1] decoding with perf tool, if
the timestamp is enabled, it uses the generic timer's counter as
timestamp source.  SPE trace data only contains the raw counter cycles,
as Will mentioned, the perf tool needs to convert it to a coordinate
value with sched_clock.  This is why this patch tries to calculate the
offset between the raw counter's ns value and sched_clock, eventually
this offset value will be used by SPE's decoding code in Perf tool to
calibrate a 'correct' timestamp.

Based on your suggestions, I will use __sched_clock_offset to resolve
the accuracy issue in patch v2.  (I noticed Peter suggested to use a
new API for wrapping clock_data structure, IIUC, __sched_clock_offset
is more straightforward for this case).

Please correct if I miss anything.  Thank you for reviewing and
suggestions!

Thanks,
Leo

[1] https://lkml.org/lkml/2020/1/23/571

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Leo Yan]

Hi all,

On Thu, Apr 30, 2020 at 05:18:15PM +0100, Will Deacon wrote:
> On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
> > 
> > > I wonder if we could/should make __sched_clock_offset available even when
> > > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
> > > help with this particular can or worm...
> > 
> > Errrgh. __sched_clock_offset is only needed on x86 because we transition
> > from one clock device to another on boot. It really shouldn't exist on
> > anything sane.
> 
> I think we still transition from jiffies on arm64, because we don't register
> with sched_clock until the timer driver probes. Marc, is that right?
> 
> > Let me try and understand your particular problem better.
> 
> I think the long and short of it is that userspace needs a way to convert
> the raw counter cycles into a ns value that can be compared against values
> coming out of sched_clock. To do this accurately, I think it needs the
> cycles value at the point when sched_clock was initialised.

Will's understanding is exactly what I want to resolve in this patch.

The background info is for the ARM SPE [1] decoding with perf tool, if
the timestamp is enabled, it uses the generic timer's counter as
timestamp source.  SPE trace data only contains the raw counter cycles,
as Will mentioned, the perf tool needs to convert it to a coordinate
value with sched_clock.  This is why this patch tries to calculate the
offset between the raw counter's ns value and sched_clock, eventually
this offset value will be used by SPE's decoding code in Perf tool to
calibrate a 'correct' timestamp.

Based on your suggestions, I will use __sched_clock_offset to resolve
the accuracy issue in patch v2.  (I noticed Peter suggested to use a
new API for wrapping clock_data structure, IIUC, __sched_clock_offset
is more straightforward for this case).

Please correct if I miss anything.  Thank you for reviewing and
suggestions!

Thanks,
Leo

[1] https://lkml.org/lkml/2020/1/23/571

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

On Fri, May 01, 2020 at 11:14:48PM +0800, Leo Yan wrote:
> On Thu, Apr 30, 2020 at 05:18:15PM +0100, Will Deacon wrote:
> > On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
> > > On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
> > > 
> > > > I wonder if we could/should make __sched_clock_offset available even when
> > > > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
> > > > help with this particular can or worm...
> > > 
> > > Errrgh. __sched_clock_offset is only needed on x86 because we transition
> > > from one clock device to another on boot. It really shouldn't exist on
> > > anything sane.
> > 
> > I think we still transition from jiffies on arm64, because we don't register
> > with sched_clock until the timer driver probes. Marc, is that right?
> > 
> > > Let me try and understand your particular problem better.
> > 
> > I think the long and short of it is that userspace needs a way to convert
> > the raw counter cycles into a ns value that can be compared against values
> > coming out of sched_clock. To do this accurately, I think it needs the
> > cycles value at the point when sched_clock was initialised.
> 
> Will's understanding is exactly what I want to resolve in this patch.
> 
> The background info is for the ARM SPE [1] decoding with perf tool, if
> the timestamp is enabled, it uses the generic timer's counter as
> timestamp source.  SPE trace data only contains the raw counter cycles,
> as Will mentioned, the perf tool needs to convert it to a coordinate
> value with sched_clock.  This is why this patch tries to calculate the
> offset between the raw counter's ns value and sched_clock, eventually
> this offset value will be used by SPE's decoding code in Perf tool to
> calibrate a 'correct' timestamp.
> 
> Based on your suggestions, I will use __sched_clock_offset to resolve
> the accuracy issue in patch v2.  (I noticed Peter suggested to use a
> new API for wrapping clock_data structure, IIUC, __sched_clock_offset
> is more straightforward for this case).
> 
> Please correct if I miss anything.  Thank you for reviewing and
> suggestions!

I don't think you can use __sched_clock_offset without selecting
HAVE_UNSTABLE_SCHED_CLOCK, and we really don't want to do that just
for this. So Peter's idea about exposing what we need is better, although
you'll probably need to take care with the switch-over from jiffies.

It needs some thought, but one possibility would be to introduce a new
variant of sthe ched_clock_register() function that returns the cycle
offset, and then we could fish that out of the timer driver. If we're
crossing all the 'i's and dotting all the 't's then we'd want to disable the
perf userpage if sched_clock changes clocksource too (a bit like we do for
the vDSO).

Will

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

On Fri, May 01, 2020 at 11:14:48PM +0800, Leo Yan wrote:
> On Thu, Apr 30, 2020 at 05:18:15PM +0100, Will Deacon wrote:
> > On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
> > > On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
> > > 
> > > > I wonder if we could/should make __sched_clock_offset available even when
> > > > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
> > > > help with this particular can or worm...
> > > 
> > > Errrgh. __sched_clock_offset is only needed on x86 because we transition
> > > from one clock device to another on boot. It really shouldn't exist on
> > > anything sane.
> > 
> > I think we still transition from jiffies on arm64, because we don't register
> > with sched_clock until the timer driver probes. Marc, is that right?
> > 
> > > Let me try and understand your particular problem better.
> > 
> > I think the long and short of it is that userspace needs a way to convert
> > the raw counter cycles into a ns value that can be compared against values
> > coming out of sched_clock. To do this accurately, I think it needs the
> > cycles value at the point when sched_clock was initialised.
> 
> Will's understanding is exactly what I want to resolve in this patch.
> 
> The background info is for the ARM SPE [1] decoding with perf tool, if
> the timestamp is enabled, it uses the generic timer's counter as
> timestamp source.  SPE trace data only contains the raw counter cycles,
> as Will mentioned, the perf tool needs to convert it to a coordinate
> value with sched_clock.  This is why this patch tries to calculate the
> offset between the raw counter's ns value and sched_clock, eventually
> this offset value will be used by SPE's decoding code in Perf tool to
> calibrate a 'correct' timestamp.
> 
> Based on your suggestions, I will use __sched_clock_offset to resolve
> the accuracy issue in patch v2.  (I noticed Peter suggested to use a
> new API for wrapping clock_data structure, IIUC, __sched_clock_offset
> is more straightforward for this case).
> 
> Please correct if I miss anything.  Thank you for reviewing and
> suggestions!

I don't think you can use __sched_clock_offset without selecting
HAVE_UNSTABLE_SCHED_CLOCK, and we really don't want to do that just
for this. So Peter's idea about exposing what we need is better, although
you'll probably need to take care with the switch-over from jiffies.

It needs some thought, but one possibility would be to introduce a new
variant of sthe ched_clock_register() function that returns the cycle
offset, and then we could fish that out of the timer driver. If we're
crossing all the 'i's and dotting all the 't's then we'd want to disable the
perf userpage if sched_clock changes clocksource too (a bit like we do for
the vDSO).

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Marc Zyngier]

On 2020-05-01 16:14, Leo Yan wrote:
> Hi all,
> 
> On Thu, Apr 30, 2020 at 05:18:15PM +0100, Will Deacon wrote:
>> On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
>> > On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
>> >
>> > > I wonder if we could/should make __sched_clock_offset available even when
>> > > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
>> > > help with this particular can or worm...
>> >
>> > Errrgh. __sched_clock_offset is only needed on x86 because we transition
>> > from one clock device to another on boot. It really shouldn't exist on
>> > anything sane.
>> 
>> I think we still transition from jiffies on arm64, because we don't 
>> register
>> with sched_clock until the timer driver probes. Marc, is that right?
>> 
>> > Let me try and understand your particular problem better.
>> 
>> I think the long and short of it is that userspace needs a way to 
>> convert
>> the raw counter cycles into a ns value that can be compared against 
>> values
>> coming out of sched_clock. To do this accurately, I think it needs the
>> cycles value at the point when sched_clock was initialised.
> 
> Will's understanding is exactly what I want to resolve in this patch.
> 
> The background info is for the ARM SPE [1] decoding with perf tool, if
> the timestamp is enabled, it uses the generic timer's counter as
> timestamp source.  SPE trace data only contains the raw counter cycles,
> as Will mentioned, the perf tool needs to convert it to a coordinate
> value with sched_clock.  This is why this patch tries to calculate the
> offset between the raw counter's ns value and sched_clock, eventually
> this offset value will be used by SPE's decoding code in Perf tool to
> calibrate a 'correct' timestamp.
> 
> Based on your suggestions, I will use __sched_clock_offset to resolve
> the accuracy issue in patch v2.  (I noticed Peter suggested to use a
> new API for wrapping clock_data structure, IIUC, __sched_clock_offset
> is more straightforward for this case).

I think Peter's point was *not* to use __sched_clock_offset which
appears to be only there for the purpose of an x86 workaround (and not
available to other architecture), but to instead expose the relevant
field (epoch_cyc) to the perf subsystem.

I feel it makes sense to make this a generic API, and see whether x86
can move over to it rather than the other way around.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Marc Zyngier]

On 2020-05-01 16:14, Leo Yan wrote:
> Hi all,
> 
> On Thu, Apr 30, 2020 at 05:18:15PM +0100, Will Deacon wrote:
>> On Thu, Apr 30, 2020 at 06:04:36PM +0200, Peter Zijlstra wrote:
>> > On Thu, Apr 30, 2020 at 04:29:23PM +0100, Marc Zyngier wrote:
>> >
>> > > I wonder if we could/should make __sched_clock_offset available even when
>> > > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK isn't defined. It feels like it would
>> > > help with this particular can or worm...
>> >
>> > Errrgh. __sched_clock_offset is only needed on x86 because we transition
>> > from one clock device to another on boot. It really shouldn't exist on
>> > anything sane.
>> 
>> I think we still transition from jiffies on arm64, because we don't 
>> register
>> with sched_clock until the timer driver probes. Marc, is that right?
>> 
>> > Let me try and understand your particular problem better.
>> 
>> I think the long and short of it is that userspace needs a way to 
>> convert
>> the raw counter cycles into a ns value that can be compared against 
>> values
>> coming out of sched_clock. To do this accurately, I think it needs the
>> cycles value at the point when sched_clock was initialised.
> 
> Will's understanding is exactly what I want to resolve in this patch.
> 
> The background info is for the ARM SPE [1] decoding with perf tool, if
> the timestamp is enabled, it uses the generic timer's counter as
> timestamp source.  SPE trace data only contains the raw counter cycles,
> as Will mentioned, the perf tool needs to convert it to a coordinate
> value with sched_clock.  This is why this patch tries to calculate the
> offset between the raw counter's ns value and sched_clock, eventually
> this offset value will be used by SPE's decoding code in Perf tool to
> calibrate a 'correct' timestamp.
> 
> Based on your suggestions, I will use __sched_clock_offset to resolve
> the accuracy issue in patch v2.  (I noticed Peter suggested to use a
> new API for wrapping clock_data structure, IIUC, __sched_clock_offset
> is more straightforward for this case).

I think Peter's point was *not* to use __sched_clock_offset which
appears to be only there for the purpose of an x86 workaround (and not
available to other architecture), but to instead expose the relevant
field (epoch_cyc) to the perf subsystem.

I feel it makes sense to make this a generic API, and see whether x86
can move over to it rather than the other way around.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Leo Yan]

On Fri, May 01, 2020 at 04:26:09PM +0100, Will Deacon wrote:

[...]

> > > > Let me try and understand your particular problem better.
> > > 
> > > I think the long and short of it is that userspace needs a way to convert
> > > the raw counter cycles into a ns value that can be compared against values
> > > coming out of sched_clock. To do this accurately, I think it needs the
> > > cycles value at the point when sched_clock was initialised.
> > 
> > Will's understanding is exactly what I want to resolve in this patch.
> > 
> > The background info is for the ARM SPE [1] decoding with perf tool, if
> > the timestamp is enabled, it uses the generic timer's counter as
> > timestamp source.  SPE trace data only contains the raw counter cycles,
> > as Will mentioned, the perf tool needs to convert it to a coordinate
> > value with sched_clock.  This is why this patch tries to calculate the
> > offset between the raw counter's ns value and sched_clock, eventually
> > this offset value will be used by SPE's decoding code in Perf tool to
> > calibrate a 'correct' timestamp.
> > 
> > Based on your suggestions, I will use __sched_clock_offset to resolve
> > the accuracy issue in patch v2.  (I noticed Peter suggested to use a
> > new API for wrapping clock_data structure, IIUC, __sched_clock_offset
> > is more straightforward for this case).
> > 
> > Please correct if I miss anything.  Thank you for reviewing and
> > suggestions!
> 
> I don't think you can use __sched_clock_offset without selecting
> HAVE_UNSTABLE_SCHED_CLOCK, and we really don't want to do that just
> for this. So Peter's idea about exposing what we need is better, although
> you'll probably need to take care with the switch-over from jiffies.
> 
> It needs some thought, but one possibility would be to introduce a new
> variant of sthe ched_clock_register() function that returns the cycle
> offset, and then we could fish that out of the timer driver.

Thanks a lot for you and Marc for correction.

> If we're
> crossing all the 'i's and dotting all the 't's then we'd want to disable the
> perf userpage if sched_clock changes clocksource too (a bit like we do for
> the vDSO).

To be honest, one thing is not clear for me is how the perf tool to
update the arch timer's parameters in the middle of tracing after
disable and re-enable per userpage.  I will note for this and look
into detailed implementation for this part.

Thanks for sharing comprehensive thoughts!

Leo

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Leo Yan]

On Fri, May 01, 2020 at 04:26:09PM +0100, Will Deacon wrote:

[...]

> > > > Let me try and understand your particular problem better.
> > > 
> > > I think the long and short of it is that userspace needs a way to convert
> > > the raw counter cycles into a ns value that can be compared against values
> > > coming out of sched_clock. To do this accurately, I think it needs the
> > > cycles value at the point when sched_clock was initialised.
> > 
> > Will's understanding is exactly what I want to resolve in this patch.
> > 
> > The background info is for the ARM SPE [1] decoding with perf tool, if
> > the timestamp is enabled, it uses the generic timer's counter as
> > timestamp source.  SPE trace data only contains the raw counter cycles,
> > as Will mentioned, the perf tool needs to convert it to a coordinate
> > value with sched_clock.  This is why this patch tries to calculate the
> > offset between the raw counter's ns value and sched_clock, eventually
> > this offset value will be used by SPE's decoding code in Perf tool to
> > calibrate a 'correct' timestamp.
> > 
> > Based on your suggestions, I will use __sched_clock_offset to resolve
> > the accuracy issue in patch v2.  (I noticed Peter suggested to use a
> > new API for wrapping clock_data structure, IIUC, __sched_clock_offset
> > is more straightforward for this case).
> > 
> > Please correct if I miss anything.  Thank you for reviewing and
> > suggestions!
> 
> I don't think you can use __sched_clock_offset without selecting
> HAVE_UNSTABLE_SCHED_CLOCK, and we really don't want to do that just
> for this. So Peter's idea about exposing what we need is better, although
> you'll probably need to take care with the switch-over from jiffies.
> 
> It needs some thought, but one possibility would be to introduce a new
> variant of sthe ched_clock_register() function that returns the cycle
> offset, and then we could fish that out of the timer driver.

Thanks a lot for you and Marc for correction.

> If we're
> crossing all the 'i's and dotting all the 't's then we'd want to disable the
> perf userpage if sched_clock changes clocksource too (a bit like we do for
> the vDSO).

To be honest, one thing is not clear for me is how the perf tool to
update the arch timer's parameters in the middle of tracing after
disable and re-enable per userpage.  I will note for this and look
into detailed implementation for this part.

Thanks for sharing comprehensive thoughts!

Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

On Sat, May 02, 2020 at 12:10:50AM +0800, Leo Yan wrote:
> On Fri, May 01, 2020 at 04:26:09PM +0100, Will Deacon wrote:
> 
> [...]
> 
> > > > > Let me try and understand your particular problem better.
> > > > 
> > > > I think the long and short of it is that userspace needs a way to convert
> > > > the raw counter cycles into a ns value that can be compared against values
> > > > coming out of sched_clock. To do this accurately, I think it needs the
> > > > cycles value at the point when sched_clock was initialised.
> > > 
> > > Will's understanding is exactly what I want to resolve in this patch.
> > > 
> > > The background info is for the ARM SPE [1] decoding with perf tool, if
> > > the timestamp is enabled, it uses the generic timer's counter as
> > > timestamp source.  SPE trace data only contains the raw counter cycles,
> > > as Will mentioned, the perf tool needs to convert it to a coordinate
> > > value with sched_clock.  This is why this patch tries to calculate the
> > > offset between the raw counter's ns value and sched_clock, eventually
> > > this offset value will be used by SPE's decoding code in Perf tool to
> > > calibrate a 'correct' timestamp.
> > > 
> > > Based on your suggestions, I will use __sched_clock_offset to resolve
> > > the accuracy issue in patch v2.  (I noticed Peter suggested to use a
> > > new API for wrapping clock_data structure, IIUC, __sched_clock_offset
> > > is more straightforward for this case).
> > > 
> > > Please correct if I miss anything.  Thank you for reviewing and
> > > suggestions!
> > 
> > I don't think you can use __sched_clock_offset without selecting
> > HAVE_UNSTABLE_SCHED_CLOCK, and we really don't want to do that just
> > for this. So Peter's idea about exposing what we need is better, although
> > you'll probably need to take care with the switch-over from jiffies.
> > 
> > It needs some thought, but one possibility would be to introduce a new
> > variant of sthe ched_clock_register() function that returns the cycle
> > offset, and then we could fish that out of the timer driver.
> 
> Thanks a lot for you and Marc for correction.
> 
> > If we're
> > crossing all the 'i's and dotting all the 't's then we'd want to disable the
> > perf userpage if sched_clock changes clocksource too (a bit like we do for
> > the vDSO).
> 
> To be honest, one thing is not clear for me is how the perf tool to
> update the arch timer's parameters in the middle of tracing after
> disable and re-enable per userpage.  I will note for this and look
> into detailed implementation for this part.

I don't fully understand the concern but, generally, the seqlock should
take care of any inconsistencies in the data page.

Will

Re: [PATCH] arm64: perf_event: Fix time_offset for arch timer

[Will Deacon]

On Sat, May 02, 2020 at 12:10:50AM +0800, Leo Yan wrote:
> On Fri, May 01, 2020 at 04:26:09PM +0100, Will Deacon wrote:
> 
> [...]
> 
> > > > > Let me try and understand your particular problem better.
> > > > 
> > > > I think the long and short of it is that userspace needs a way to convert
> > > > the raw counter cycles into a ns value that can be compared against values
> > > > coming out of sched_clock. To do this accurately, I think it needs the
> > > > cycles value at the point when sched_clock was initialised.
> > > 
> > > Will's understanding is exactly what I want to resolve in this patch.
> > > 
> > > The background info is for the ARM SPE [1] decoding with perf tool, if
> > > the timestamp is enabled, it uses the generic timer's counter as
> > > timestamp source.  SPE trace data only contains the raw counter cycles,
> > > as Will mentioned, the perf tool needs to convert it to a coordinate
> > > value with sched_clock.  This is why this patch tries to calculate the
> > > offset between the raw counter's ns value and sched_clock, eventually
> > > this offset value will be used by SPE's decoding code in Perf tool to
> > > calibrate a 'correct' timestamp.
> > > 
> > > Based on your suggestions, I will use __sched_clock_offset to resolve
> > > the accuracy issue in patch v2.  (I noticed Peter suggested to use a
> > > new API for wrapping clock_data structure, IIUC, __sched_clock_offset
> > > is more straightforward for this case).
> > > 
> > > Please correct if I miss anything.  Thank you for reviewing and
> > > suggestions!
> > 
> > I don't think you can use __sched_clock_offset without selecting
> > HAVE_UNSTABLE_SCHED_CLOCK, and we really don't want to do that just
> > for this. So Peter's idea about exposing what we need is better, although
> > you'll probably need to take care with the switch-over from jiffies.
> > 
> > It needs some thought, but one possibility would be to introduce a new
> > variant of sthe ched_clock_register() function that returns the cycle
> > offset, and then we could fish that out of the timer driver.
> 
> Thanks a lot for you and Marc for correction.
> 
> > If we're
> > crossing all the 'i's and dotting all the 't's then we'd want to disable the
> > perf userpage if sched_clock changes clocksource too (a bit like we do for
> > the vDSO).
> 
> To be honest, one thing is not clear for me is how the perf tool to
> update the arch timer's parameters in the middle of tracing after
> disable and re-enable per userpage.  I will note for this and look
> into detailed implementation for this part.

I don't fully understand the concern but, generally, the seqlock should
take care of any inconsistencies in the data page.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel