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

[Yang Shi]

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