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

[Yang Shi <yang@os.amperecomputing.com>]

On 3/31/23 2:53 AM, Pierre Gondois wrote:
> 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.
>

Thanks for the points. IIUC, the delay should be limited by the two 
factors. So it should be max(cppc_cpufreq_get_transition_delay_us(), 
Time Winder Register)?

> Regards,
> Pierre