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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Apr 2023 11:57:31 -0700
From:   Yang Shi <yang@...amperecomputing.com>
To:     Pierre Gondois <pierre.gondois@....com>,
        Viresh Kumar <viresh.kumar@...aro.org>
Cc:     rafael@...nel.org, scott@...amperecomputing.com,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        vincent.guittot@...aro.org, lukasz.luba@....com,
        ionela.voinescu@....com
Subject: Re: [PATCH] cpufreq: CPPC: use 10ms delay instead of 2us to avoid
 high error



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@...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