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

[Christian Loehle <christian.loehle@arm.com>]

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)