Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

From: Viresh Kumar
Date: Wed Jun 21 2017 - 23:55:27 EST


On 21-06-17, 17:38, Dietmar Eggemann wrote:
> On 20/06/17 07:17, Viresh Kumar wrote:

> > Any specific reason on why are we doing this from PRECHANGE and
> > not POSTCHANGE ? i.e. we are doing this before the frequency is
> > really updated.
>
> Not really. In case I get a CPUFREQ_POSTCHANGE all the time the
> frequency actually changed I can switch to CPUFREQ_POSTCHANGE.

Yes, you should always get that. And its not right to do any such
change in PRECHANGE notifier as we may fail to change the frequency as
well..

> > Wanted to make sure that we all understand the constraints this is going to add
> > for the ARM64 platforms.
> >
> > With the introduction of this transition notifier, we would not be able to use
> > the fast-switch path in the schedutil governor. I am not sure if there are any
> > ARM platforms that can actually use the fast-switch path in future or not
> > though. The requirement of fast-switch path is that the freq can be changed
> > without sleeping in the hot-path.
>
> That's a good point. The cpufreq transition notifier based Frequency
> Invariance Engine (FIE) can only work if none of the cpufreq policies
> support fast frequency switching.

At least with the current design, yes.

> What about we still enable cpufreq transition notifier based FIE for
> systems where this is true. This will cover 100% of all arm/arm64
> systems today.

I would suggest having a single solution for everyone if we can.

> In case one day we have a cpufreq driver which allows fast frequency
> switching we would need a FIE based on something else than cpufreq
> transition notifier. Maybe based on performance counters (something
> similar to x86 APERF/MPERF) or cpufreq core could provide a function
> which provides the avg frequency value.
>
> I could make the current implementation more future-proof by only
> using the notifier based FIE in case all policies use slow frequency
> switching:
>
> >From afe64b5c0606cad4304b77fc5cff819d3083a88d Mon Sep 17 00:00:00 2001
> From: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
> Date: Wed, 21 Jun 2017 14:53:26 +0100
> Subject: [PATCH] drivers base/arch_topology: enable cpufreq transistion
> notifier based FIE only for slow frequency switching
>
> Fast frequency switching is incompatible with cpufreq transition
> notifiers.
>
> Enable the cpufreq transition notifier based Frequency Invariance Engine
> (FIE) only in case there are no cpufreq policies able to use fast
> frequency switching.
>
> Currently there are no cpufreq drivers for arm/arm64 which support fast
> frequency switching. In case such a driver will appear the FEI
> topology_get_freq_scale() has to be extended to provide frequency
> invariance based on something else than cpufreq transition notifiers,
> e.g. performance counters.
>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
> ---
> drivers/base/arch_topology.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index c2539dc584d5..bd14c5e81f63 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -171,6 +171,7 @@ static bool cap_parsing_done;
> static void parsing_done_workfn(struct work_struct *work);
> static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
> static DEFINE_PER_CPU(unsigned long, max_freq);
> +static bool enable_freq_inv = true;
>
> static int
> init_cpu_capacity_callback(struct notifier_block *nb,
> @@ -199,6 +200,8 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> policy->cpuinfo.max_freq / 1000UL;
> capacity_scale = max(raw_capacity[cpu], capacity_scale);
> }
> + if (policy->fast_switch_possible)
> + enable_freq_inv = false;
> if (cpumask_empty(cpus_to_visit)) {
> if (!cap_parsing_failed) {
> topology_normalize_cpu_scale();
> @@ -268,21 +271,23 @@ static int __init register_cpufreq_notifier(void)
> ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> CPUFREQ_POLICY_NOTIFIER);
>
> - if (ret) {
> + if (ret)
> free_cpumask_var(cpus_to_visit);
> - return ret;
> - }
>
> - return cpufreq_register_notifier(&set_freq_scale_notifier,
> - CPUFREQ_TRANSITION_NOTIFIER);
> + return ret;
> }
> core_initcall(register_cpufreq_notifier);
>
> static void parsing_done_workfn(struct work_struct *work)
> {
> +
> free_cpumask_var(cpus_to_visit);
> cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
> CPUFREQ_POLICY_NOTIFIER);
> +
> + if (enable_freq_inv)
> + cpufreq_register_notifier(&set_freq_scale_notifier,
> + CPUFREQ_TRANSITION_NOTIFIER);
> }

This may work, but lets see if we can find a way of doing this for
everyone at once.

(I will continue to reply on Morten's email now)..

--
viresh