Re: [PATCH] arm64: Provide an AMU-based version of arch_freq_get_on_cpu
From: Sumit Gupta
Date: Fri Jun 23 2023 - 10:34:32 EST
On 16/06/23 15:23, Beata Michalska wrote:
External email: Use caution opening links or attachments
On Thu, Jun 15, 2023 at 12:29:57AM +0530, 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.
Would you mind sharing some more data re your testing ?
The arch_freq_get_on_cpu will provided an average freq for last tick period,
with an updated occurring on each sched tick so the differences between set
and get might show up. With your stress testing, if the frequency change comes
at the end of current tick period, it might not be reflected until next one
elapses.
In case of idle states - if the CPU for which the current frequency is being
requested is in idle mode, the frequency returned will be the last one before
entering idle, which seems reasonable (?).
I guess the question here would be what is your tolerance level for those
differences.
The test waits for 50ms before reading back the set frequency which is
much more than a tick period.
High delta value might be due to the reference counter increments
happening in bursts in Tegra241 SoC as mentioned in [1].
If the CPU is idle for most of the period then observation window will
be small due to AMU counters stopping at WFI. This can cause more error
in Tegra241 as increasing the observation time interval reduces error
percentage.
I created 100% CPU load and then the test is passing. So, looks like we
will have to create load before the frequency switch test. Please share
if more suggestions.
[1] https://lore.kernel.org/lkml/20230418113459.12860-5-sumitg@xxxxxxxxxx/
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.
Both CPU_CYCLES and CNT_CYCLES are not incremented in WFI.
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@xxxxxxxxxx/
[2] https://lore.kernel.org/lkml/cde1d8a9-3a21-e82b-7895-40603a14d898@xxxxxxxxxx/T/#mb898a75fd0c72d166b26b04da3ad162afe068a82