Re: [PATCH] cpufreq: CPPC: use 10ms delay instead of 2us to avoid high error

[Pierre Gondois <pierre.gondois@arm.com>]

Hello,

On 3/30/23 05:56, Viresh Kumar wrote:
> + few folks.
> 
> On 28-03-23, 12:38, Yang Shi wrote:
>> When testing CPPC cpufreq on our platform, we noticed the error may be quite
>> high and the high error may happen quite often.  For example, on a platform
>> with a maximum frequency of 2.8GHz when the CPUs were fully loaded (100% load),
>> we saw cpuinfo_cur_freq may show 4GHz, it means the error is > 40%.  And the
>> high error (> 1%) happened 256 times out of 2127 samples (sampled every 3
>> seconds) in an approximate 2hrs test.
>>
>> We tried to enlarge the delay, and tested with 100us, 1ms and 10ms.  The
>> below is the results.
>>
>> 100us:
>> The highest error is 4GHz, 22 times out of 3623 samples
>>
>> 1ms:
>> The highest error is 3.3GHz, 3 times out of 2814 samples
>>
>> 10ms:
>> No high error anymore
>>
>> Increase the measurement delay in cppc_cpufreq_get_rate to 10ms to avoid
>> high measurement errors.
>>
>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 022e3555407c..c2bf65448d3d 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -851,7 +851,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>>   	if (ret)
>>   		return ret;
>>   
>> -	udelay(2); /* 2usec delay between sampling */
>> +	mdelay(10); /* 10msec delay between sampling */
>>   
>>   	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>>   	if (ret)
>> -- 
>> 2.39.2
> 

Just 2 considerations:
-
When using the schedutil governor, frequencies should be updated with
a period of cppc_cpufreq_get_transition_delay_us().
This period should be 1ms if CPPC doesn't rely on PCC channels, otherwise
the value depends on the PCC channel (cf. cppc_get_transition_latency()).

If the evaluation duration for the perf/ref counters is higher than this
period, I think this would mean that multiple frequency update would happen
while trying to evaluate the current frequency of a CPU.

-
There is a TimeWindowRegister field in CPPC (cf. enum cppc_regs::TIME_WINDOW
and ACPI 6.5 s8.4.6.1.2.5 "Time Window Register") that should approximately
match what this patch aims to solve.
"""
When Autonomous Selection is enabled, values written to the Time Window Register are ignored. Reads of the Time
Window register indicate minimum length of time (in ms) between successive reads of the platform’s performance
counters.
"""
The only issue being that we should be in the case where Autonomous Selection
is disabled, where the description of the register is different.

Regards,
Pierre