Messages in this thread | | | Date | Wed, 18 Oct 2023 18:35:47 +0530 | Subject | Re: [PATCH] arm64: Provide an AMU-based version of arch_freq_get_on_cpu | From | Sumit Gupta <> |
| |
On 15/06/23 00:29, Sumit Gupta wrote: > > > On 06/06/23 21:27, Beata Michalska wrote: >> External email: Use caution opening links or attachments >> >> >> With the Frequency Invariance Engine (FIE) being already wired up with >> sched tick and making use of relevant (core counter and constant >> counter) AMU counters, getting the current frequency for a given CPU >> on supported platforms, can be achieved by utilizing the frequency scale >> factor which reflects an average CPU frequency for the last tick period >> length. >> >> With that at hand, arch_freq_get_on_cpu dedicated implementation >> gets enrolled into cpuinfo_cur_freq policy sysfs attribute handler, >> which is expected to represent the current frequency of a given CPU, >> as obtained by the hardware. This is exactly the type of feedback that >> cycle counters provide. >> >> In order to avoid calling arch_freq_get_on_cpu from the scaling_cur_freq >> attribute handler for platforms that do provide cpuinfo_cur_freq, and >> yet keeping things intact for those platform that do not, its use gets >> conditioned on the presence of cpufreq_driver (*get) callback (which also >> seems to be the case for creating cpuinfo_cur_freq attribute). >> > > Tested the change with frequency switch stress test but was getting big > delta between set and get freq. > After passing "nohz=off" and commenting "wfi" in "cpu_do_idle()", the > delta is less. This confirms that more delta is due to AMU counters > stopping at "WFI". > > +++ b/arch/arm64/kernel/idle.c > @@ -27,7 +27,7 @@ void noinstr cpu_do_idle(void) > arm_cpuidle_save_irq_context(&context); > > dsb(sy); > - wfi(); > +// wfi(); > > I am not sure if the expected behavior here is right. > In our tests, we compare the last set frequency against the re-generated > value from counters to confirm that the CPU is actually running at the > requested frequency and the counters are working correct. But that won't > happen with this change. > > In [1] and later in the updated patch within [2], we are busy looping > on the target CPU and avoid WFI to get the actual frequency. > > Please share what you think is the right expected behavior. > > [1] https://lore.kernel.org/lkml/20230418113459.12860-7-sumitg@nvidia.com/ > [2] > https://lore.kernel.org/lkml/cde1d8a9-3a21-e82b-7895-40603a14d898@nvidia.com/T/#mb898a75fd0c72d166b26b04da3ad162afe068a82
Observed another issue where CPUFREQ is coming too high when the performance governor is set as default.
Below change solves that by using the new API arch_freq_get_on_cpu() if present over the existing one, while verifying the currently set frequency.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 62face349fd2..2c74e70f701e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1761,9 +1761,12 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b { unsigned int new_freq;
- new_freq = cpufreq_driver->get(policy->cpu); - if (!new_freq) - return 0; + new_freq = arch_freq_get_on_cpu(policy->cpu); + if (!new_freq) { + new_freq = cpufreq_driver->get(policy->cpu); + if (!new_freq) + return 0; + }
Best Regards, Sumit Gupta
| |