Re: [PATCH 3/3] arm64: topology: Reduce the skew for AMU feedback counter sampling

[Beata Michalska <beata.michalska@arm.com>]

On Thu, Nov 12, 2026 at 09:10:28AM +0000, Christian Loehle wrote:
> On 11/11/26 18:38, Beata Michalska wrote:
> > Hi Christian,
> > On Wed, Nov 11, 2026 at 01:56:49PM +0000, Christian Loehle wrote:
> >> On 11/11/26 12:39, Beata Michalska wrote:
> >>> With the FFH support for CPPC, the AMU feedback counters, core and const
> >>> counter, are being sampled to measure CPU performance. Checking for errata
> >>> (which can affect both counters) between the reads delays the second
> >>> sample and can skew the result.
> >>>
> >>> Take both samples in succession, then discard values from affected
> >>> counters. This narrows the sampling interval used for CPPC feedback.
> >>>
> >>> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> >>> ---
> >>>  arch/arm64/kernel/topology.c | 76 +++++++++++++++++++++++-------------
> >>>  1 file changed, 49 insertions(+), 27 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> >>> index 66ac67bd025e..c968fa5b0c88 100644
> >>> --- a/arch/arm64/kernel/topology.c
> >>> +++ b/arch/arm64/kernel/topology.c
> >>> @@ -381,29 +381,45 @@ enum cpc_ffh_ctr_id {
> >>>  	CPC_FFH_CTR_CONST = 0x1,
> >>>  };
> >>>  
> >>> -static void cpu_read_corecnt(void *val)
> >>> +static inline void cpu_read_corecnt(void *val)
> >>>  {
> >>> -	/*
> >>> -	 * A value of 0 can be returned if the current CPU does not support AMUs
> >>> -	 * or if the counter is disabled for this CPU. A return value of 0 at
> >>> -	 * counter read is properly handled as an error case by the users of the
> >>> -	 * counter.
> >>> -	 */
> >>> -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT) ?
> >>> -		0UL : read_corecnt();
> >>> +	*(u64 *)val = read_corecnt();
> >>>  }
> >>>  
> >>> -static void cpu_read_constcnt(void *val)
> >>> +static void cpu_try_read_corecnt(void *val)
> >>>  {
> >>> -	/*
> >>> -	 * Return 0 if the current CPU is affected by a HW erratum.
> >>> -	 * A value of 0 is also returned if the current CPU does not
> >>> -	 * support AMUs or if the counter is disabled. A return
> >>> -	 * value of 0 at counter read is properly handled as an error
> >>> -	 * case by the users of the counter.
> >>> -	 */
> >>> -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT) ?
> >>> -		      0UL : read_constcnt();
> >>> +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT)) {
> >>> +		cpu_read_corecnt(val);
> >>> +	} else {
> >>> +		/*
> >>> +		 * A value of 0 can be returned if the current CPU does not support AMUs
> >>> +		 * or if the counter is disabled for this CPU. A return value of 0 at
> >>> +		 * counter read is properly handled as an error case by the users of the
> >>> +		 * counter.
> >>> +		 */
> >>> +		*(u64 *)val = 0UL;
> >>> +	}
> >>> +}
> >>> +
> >>> +static inline void cpu_read_constcnt(void *val)
> >>> +{
> >>> +	*(u64 *)val = read_constcnt();
> >>> +}
> >>> +
> >>> +static void cpu_try_read_constcnt(void *val)
> >>> +{
> >>> +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT)) {
> >>> +		cpu_read_constcnt(val);
> >>> +	} else {
> >>> +		/*
> >>> +		 * Return 0 if the current CPU is affected by a HW erratum.
> >>> +		 * A value of 0 is also returned if the current CPU does not
> >>> +		 * support AMUs or if the counter is disabled. A return
> >>> +		 * value of 0 at counter read is properly handled as an error
> >>> +		 * case by the users of the counter.
> >>> +		 */
> >>> +		*(u64 *)val = 0UL;
> >>> +	}
> >>>  }
> >>>  
> >>>  static inline
> >>> @@ -461,14 +477,20 @@ static void amu_read_core_const_ctrs(void *val)
> >>>  {
> >>>  	struct amu_ffh_ctrs *ctrs = val;
> >>>  
> >>> +	cpu_read_corecnt(&ctrs->corecnt);
> >>> +	cpu_read_constcnt(&ctrs->constcnt);
> >>> +
> >>>  	/*
> >>> -	 * cpu_read_constcnt() incurs slight latency due to the
> >>> -	 * ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT check.
> >>> -	 * Read it first to minimize the sampling skew between the const
> >>> -	 * and core counters.
> >>> +	 * Return 0 if the current CPU is affected by a HW erratum.
> >>> +	 * A value of 0 is also returned if the current CPU does not
> >>> +	 * support AMUs or if the counter is disabled. A return
> >>> +	 * value of 0 at counter read is properly handled as an error
> >>> +	 * case by the users of the counter.
> >>
> >> Why do we read them first then on 
> >> ARM64_WORKAROUND_BROKEN_AMU_CORECNT or ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT
> >> respectively?
> >>
> > That's what this patch is all about - isn't it ?
> 
> I thought it's about getting (all GCC 11.4 here)
> mrs x0, amevcntr00_el0
> str x0, [x19]
> mrs x0, amevcntr01_el0
> 
> instead of
> mrs     x1, amevcntr01_el0
> [100 instructions here...]
> mrs     x1, amevcntr00_el0
> 
> (before this patch)
> 
> > It allows direct read of both counters (those being read first),
> > and only once that's done, it verifies whether the workaround should be applied
> > or not. For cores that do not have either it eliminates the check between
> > reading the counters.
> > Or you had smth else in mind I did not quite manage to grasp ?
> 
> Something like:
> 
>     bool core_broken = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT);
>     bool const_broken = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT);
> 
>     if (core_broken) {
>         ctrs->corecnt = 0;
>         ctrs->constcnt = const_broken ? 0 : read_constcnt();
>     } else if (const_broken) {
>         ctrs->corecnt = read_corecnt();
>         ctrs->constcnt = 0;
>     } else {
>         ctrs->corecnt = read_corecnt();
>         ctrs->constcnt = read_constcnt();
>     }
> 
> 
> gives us
> 
> mrs x0, amevcntr00_el0
> mrs x1, amevcntr01_el0
> stp x0, x1, [x19]
> 
> (for the non-broken case obviously)
Will update that to eliminate the pointless MRS case the workarounds are active.
Not that it will change anything, but will look better ? :)

---
BR
Beata