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

[Christian Loehle]

On 11/12/26 19:12, 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
> one and can skew the result.
> 
> Read both counters in succession if none is actually affected by HW
> errata.
> 
> And while at it, update the comments to be more accurate (or less
> inaccurate).
> 
> Signed-off-by: Beata Michalska <beata.michalska@xxxxxxx>

Reviewed-by: Christian Loehle <christian.loehle@xxxxxxx>

Testing this via the sysfs cpuinfo_cur_freq on orion o6 A520 (not
affected, both counters work) I get a p99 sampling error of:


  Reader / target workload             Before           Patch
  -------------------------------------------------------------
  Remote big CPU / idle                11.11             0.17
  Remote big CPU / busy                 2.22             0.22
  Another little CPU / busy             2.06             0.22
  Local / busy                         13.56             1.17

Nice!
Tested-by: Christian Loehle <christian.loehle@xxxxxxx>

> ---
>  arch/arm64/kernel/topology.c | 38 ++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 30a52659f15c..d00aec6354e4 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -384,10 +384,10 @@ enum cpc_ffh_ctr_id {
>  static 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.
> +	 * Return 0 if the current CPU is affected by a HW erratum.
> +	 * Otherwise, return the AMU counter value.
> +	 *
> +	 * A counter value of 0 is treated as an error by callers.
>  	 */
>  	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT) ?
>  		      0UL : read_corecnt();
> @@ -397,10 +397,9 @@ static void cpu_read_constcnt(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.
> +	 * Otherwise, return the AMU counter value.
> +	 *
> +	 * A counter value of 0 is treated as an error by callers.
>  	 */
>  	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT) ?
>  		      0UL : read_constcnt();
> @@ -460,15 +459,24 @@ bool cpc_ffh_supported(void)
>  static void amu_read_core_const_ctrs(void *val)
>  {
>  	struct amu_ffh_ctrs *ctrs = val;
> -
> +	bool corecntr_broken =
> +		this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT);
> +	bool constcntr_broken =
> +		this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT);
> +
> +	if (!corecntr_broken && !constcntr_broken) {
> +		ctrs->corecnt = read_corecnt();
> +		ctrs->constcnt = read_constcnt();
> +		return;
> +	}
>  	/*
> -	 * 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.
> +	 * Otherwise, return the AMU counter value.
> +	 *
> +	 * A counter value of 0 is treated as an error by callers.
>  	 */
> -	cpu_read_constcnt(&ctrs->constcnt);
> -	cpu_read_corecnt(&ctrs->corecnt);
> +	ctrs->corecnt = corecntr_broken ? 0UL : read_corecnt();
> +	ctrs->constcnt = constcntr_broken ? 0UL : read_constcnt();
>  }
>  
>  static bool cpc_ffh_reg_valid(const struct cpc_reg *reg)