[0/7] cpufreq: Call transition notifier only once for each policy

Message ID cover.1552545525.git.viresh.kumar@linaro.org
Headers show
Series
  • cpufreq: Call transition notifier only once for each policy
Related show

Message

Viresh Kumar March 14, 2019, 6:42 a.m.
Currently we call the cpufreq transition notifiers once for each CPU of
the policy->cpus cpumask, which isn't that efficient. This patchset
tries to simplify that by adding another field in struct cpufreq_freqs,
cpus, so the callback has all the information available with a single
call for each policy.

I have tested this on arm64 platform and is compile tested for other
platforms. This has gone through 0-day testing as well, I have pushed my
branch over a week back to the public tree which gets tested by 0-day
bot.

FWIW, it maybe possible to make the callback implementation more
efficient now that they are called only once for each policy, but this
patchset only did the minimum amount of changes to make sure we don't
end up breaking otherwise working code.

--
viresh

Viresh Kumar (7):
  cpufreq: Pass policy->related_cpus to transition notifiers
  ARM: smp: Update cpufreq transition notifier to handle multiple CPUs
  ARM: twd: Update cpufreq transition notifier to handle multiple CPUs
  sparc64: Update cpufreq transition notifier to handle multiple CPUs
  x86/tsc: Update cpufreq transition notifier to handle multiple CPUs
  KVM: x86: Update cpufreq transition notifier to handle multiple CPUs
  cpufreq: Call transition notifiers only once for each policy

 arch/arm/kernel/smp.c       | 23 ++++++++++++++---------
 arch/arm/kernel/smp_twd.c   |  9 ++++++---
 arch/sparc/kernel/time_64.c | 28 ++++++++++++++++------------
 arch/x86/kernel/tsc.c       | 31 ++++++++++++++++++++-----------
 arch/x86/kvm/x86.c          | 31 ++++++++++++++++++++-----------
 drivers/cpufreq/cpufreq.c   | 19 ++++++++++---------
 include/linux/cpufreq.h     |  2 +-
 7 files changed, 87 insertions(+), 56 deletions(-)

-- 
2.21.0.rc0.269.g1a574e7a288b

Comments

Rafael J. Wysocki March 14, 2019, 9:28 a.m. | #1
On Thu, Mar 14, 2019 at 7:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> Currently we call the cpufreq transition notifiers once for each CPU of

> the policy->cpus cpumask, which isn't that efficient.


Why isn't it efficient?

Transitions are per-policy anyway, so if something needs to be done
for each CPU in the policy, it doesn't matter too much which part of
the code carries out the iteration.

I guess some notifiers need to know what other CPUs there are in the
policy?  If so, then why?

> This patchset tries to simplify that by adding another field in struct cpufreq_freqs,

> cpus, so the callback has all the information available with a single

> call for each policy.


Well, you can argue that the core is simplified by it somewhat, but
the notifiers aren't.  They actually get more complex, conceptually
too, because they now need to worry about offline vs online CPUs etc.

Also I wonder why you decided to pass a cpumask in freqs instead of
just passing a policy pointer.  If you change things from per-CPU to
per-policy, passing the whole policy seems more natural.
Rafael J. Wysocki March 14, 2019, 9:33 a.m. | #2
On Thu, Mar 14, 2019 at 7:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> The cpufreq core currently calls the cpufreq transition notifier

> callback once for each affected CPU. This is going to change soon and

> the cpufreq core will call the callback only once for each cpufreq

> policy. The callback must look at the newly added field in struct

> cpufreq_freqs, "cpus", which contains policy->related_cpus (both

> online/offline CPUs) and perform per-cpu actions for them if any.

>

> This patch updates time_cpufreq_notifier() to use the new "cpus" field,

> update per-cpu data for all the related CPUs and call set_cyc2ns_scale()

> for all online related_cpus.

>

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  arch/x86/kernel/tsc.c | 31 ++++++++++++++++++++-----------

>  1 file changed, 20 insertions(+), 11 deletions(-)

>

> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c

> index 3fae23834069..587a6aa72f38 100644

> --- a/arch/x86/kernel/tsc.c

> +++ b/arch/x86/kernel/tsc.c

> @@ -956,28 +956,37 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,

>                                 void *data)

>  {

>         struct cpufreq_freqs *freq = data;

> -       unsigned long *lpj;

> -

> -       lpj = &boot_cpu_data.loops_per_jiffy;

> -#ifdef CONFIG_SMP

> -       if (!(freq->flags & CPUFREQ_CONST_LOOPS))

> -               lpj = &cpu_data(freq->cpu).loops_per_jiffy;

> -#endif

> +       bool boot_cpu = !IS_ENABLED(CONFIG_SMP) || freq->flags & CPUFREQ_CONST_LOOPS;

> +       unsigned long lpj;

> +       int cpu;

>

>         if (!ref_freq) {

>                 ref_freq = freq->old;

> -               loops_per_jiffy_ref = *lpj;

>                 tsc_khz_ref = tsc_khz;

> +

> +               if (boot_cpu)

> +                       loops_per_jiffy_ref = boot_cpu_data.loops_per_jiffy;

> +               else

> +                       loops_per_jiffy_ref = cpu_data(cpumask_first(freq->cpus)).loops_per_jiffy;

>         }

> +

>         if ((val == CPUFREQ_PRECHANGE  && freq->old < freq->new) ||

>                         (val == CPUFREQ_POSTCHANGE && freq->old > freq->new)) {

> -               *lpj = cpufreq_scale(loops_per_jiffy_ref, ref_freq, freq->new);

> -

> +               lpj = cpufreq_scale(loops_per_jiffy_ref, ref_freq, freq->new);

>                 tsc_khz = cpufreq_scale(tsc_khz_ref, ref_freq, freq->new);

> +

>                 if (!(freq->flags & CPUFREQ_CONST_LOOPS))

>                         mark_tsc_unstable("cpufreq changes");

>

> -               set_cyc2ns_scale(tsc_khz, freq->cpu, rdtsc());

> +               if (boot_cpu) {

> +                       boot_cpu_data.loops_per_jiffy = lpj;

> +               } else {

> +                       for_each_cpu(cpu, freq->cpus)


This needs to iterate over policy->cpus or you change the behavior.

Not that it will matter a lot (x86 in one CPU per policy anyway in the
vast majority of cases), but it is a change nevertheless.  Moreover,
I'm not even sure if doing that for offline CPUs makes sense.

> +                               cpu_data(cpu).loops_per_jiffy = lpj;

> +               }

> +

> +               for_each_cpu_and(cpu, freq->cpus, cpu_online_mask)

> +                       set_cyc2ns_scale(tsc_khz, cpu, rdtsc());

>         }

>

>         return 0;

> --
Viresh Kumar March 14, 2019, 10:03 a.m. | #3
On 14-03-19, 10:33, Rafael J. Wysocki wrote:
> On Thu, Mar 14, 2019 at 7:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > -               set_cyc2ns_scale(tsc_khz, freq->cpu, rdtsc());

> > +               if (boot_cpu) {

> > +                       boot_cpu_data.loops_per_jiffy = lpj;

> > +               } else {

> > +                       for_each_cpu(cpu, freq->cpus)

> 

> This needs to iterate over policy->cpus or you change the behavior.

> 

> Not that it will matter a lot (x86 in one CPU per policy anyway in the

> vast majority of cases), but it is a change nevertheless.  Moreover,

> I'm not even sure if doing that for offline CPUs makes sense.


Okay.

-- 
viresh
Viresh Kumar March 14, 2019, 11:03 a.m. | #4
On 14-03-19, 11:55, Rafael J. Wysocki wrote:
> On Thu, Mar 14, 2019 at 11:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> But some of them need to combine the new cpumask with

> cpu_online_mask() to get what would be policy->cpus effectively.  That

> would be avoidable if you passed the policy pointer to them.


Right, that's what I also thought after your previous email. Will pass
the policy pointer instead.

-- 
viresh
David Miller March 14, 2019, 5:27 p.m. | #5
From: Viresh Kumar <viresh.kumar@linaro.org>

Date: Thu, 14 Mar 2019 12:12:50 +0530

> The cpufreq core currently calls the cpufreq transition notifier

> callback once for each affected CPU. This is going to change soon and

> the cpufreq core will call the callback only once for each cpufreq

> policy. The callback must look at the newly added field in struct

> cpufreq_freqs, "cpus", which contains policy->related_cpus (both

> online/offline CPUs) and perform per-cpu actions for them if any.

> 

> This patch updates sparc64_cpufreq_notifier() to use the new "cpus" field

> and update per-cpu data for all the related CPUs.

> 

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>


Acked-by: David S. Miller <davem@davemloft.net>