Messages in this thread | | | Date | Tue, 4 Apr 2023 11:57:31 -0700 | Subject | Re: [PATCH] cpufreq: CPPC: use 10ms delay instead of 2us to avoid high error | From | 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@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
| |