Re: [PATCH 4/8] cpufreq,vexpress-spc: fix Frequency Invariance (FI) for bL switching

[Ionela Voinescu]

On Wednesday 01 Jul 2020 at 16:16:19 (+0530), Viresh Kumar wrote:
> On 01-07-20, 10:07, Ionela Voinescu wrote:
> > In the majority of cases, the index argument to cpufreq's target_index()
> > is meant to identify the frequency that is requested from the hardware,
> > according to the frequency table: policy->freq_table[index].frequency.
> > 
> > After successfully requesting it from the hardware, this value, together
> > with the maximum hardware frequency (policy->cpuinfo.max_freq) are used
> > as arguments to arch_set_freq_scale(), in order to set the task scheduler
> > frequency scale factor. This is a normalized indication of a CPU's
> > current performance.
> > 
> > But for the vexpress-spc-cpufreq driver, when big.LITTLE switching [1]
> > is enabled, there are three issues with using the above information for
> > setting the FI scale factor:
> > 
> >  - cur_freq: policy->freq_table[index].frequency is not the frequency
> >    requested from the hardware. ve_spc_cpufreq_set_rate() will convert
> >    from this virtual frequency to an actual frequency, which is then
> >    requested from the hardware. For the A7 cluster, the virtual frequency
> >    is half the actual frequency. The use of the virtual policy->freq_table
> >    frequency results in an incorrect FI scale factor.
> > 
> >  - max_freq: policy->cpuinfo.max_freq does not correctly identify the
> >    maximum frequency of the physical cluster. This value identifies the
> >    maximum frequency achievable by the big-LITTLE pair, that is the
> >    maximum frequency of the big CPU. But when the LITTLE CPU in the group
> >    is used, the hardware maximum frquency passed to arch_set_freq_scale()
> >    is incorrect.
> > 
> >  - missing a scale factor update: when switching clusters, the driver
> >    recalculates the frequency of the old clock domain based on the
> >    requests of the remaining CPUs in the domain and asks for a clock
> >    change. But this does not result in an update in the scale factor.
> > 
> > Therefore, introduce a local function bLs_set_sched_freq_scale() that
> > helps call arch_set_freq_scale() with correct information for the
> > is_bL_switching_enabled() case, while maintaining the old, more
> > efficient, call site of arch_set_freq_scale() for when cluster
> > switching is disabled.
> > 
> > Also, because of these requirements in computing the scale factor, this
> > driver is the only one that maintains custom support for FI, which is
> > marked by the presence of the CPUFREQ_CUSTOM_SET_FREQ_SCALE flag.
> > 
> > [1] https://lwn.net/Articles/481055/
> > 
> > Signed-off-by: Ionela Voinescu <ionela.voinescu@xxxxxxx>
> > Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> > Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> > Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> > ---
> >  drivers/cpufreq/vexpress-spc-cpufreq.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> Is there anyone who cares for this driver and EAS ? I will just skip doing the
> FIE thing here and mark it skipped.
> 

That is a good question. The vexpress driver is still used for TC2, but
I don't know of any users of this bL switcher functionality that's part
of the driver. I think there were a few people wondering recently if
it's still used [1].

If we disconsider the bL switcher functionality, then the vexpress
driver itself gets in line with the other drivers supported by this
series. Therefore there would not be a flag set needed here. Also, to
maintain current functionality, we would not need to introduce a flag
at all.

But, the frequency invariance fix is also useful for schedutil to
better select a frequency based on invariant utilization. So if we
independently decide having a flag like the one introduced at 1/8 is
useful, I would recommend to consider this patch, as it does fix some
current functionality in the kernel (whether we can determine if it's
used much or not).

Therefore, IMO, if it's not used any more it should be removed, but if
it's kept it should be fixed.

[1] https://lore.kernel.org/linux-arm-kernel/20200624195811.435857-8-maz@xxxxxxxxxx/


Thanks,
Ionela.


> -- 
> viresh