[PATCH V2 0/3] cpufreq: cppc: Add support for frequency invariance

[PATCH V2 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

Hello,

Changes since V1:

- Few of the patches migrating users to ->exit() callback are posted separately.

- The CPPC patch was completely reverted and so the support for FIE is again
  added here from scratch.

- The start_cpu() and stop_cpu() interface is reworked a little so stop_cpu() is
  only ever called for a CPU if start_cpu() was called for it earlier.

- A new patch to implement RCU locking in arch_topology core to avoid some
  races.

- Some cleanup and very clear/separate paths for FIE in cppc driver now.


-------------------------8<-------------------------

CPPC cpufreq driver is used for ARM servers and this patch series tries to
provide counter-based frequency invariance support for them in the absence for
architecture specific counters (like AMUs).

This was reverted earlier for the 5.13 kernel after Qian Cai reported kernel
oops during suspend/resume.

This is based of pm/linux-next + a cleanup patchset:

https://lore.kernel.org/linux-pm/cover.1623825358.git.viresh.kumar@linaro.org/

All the patches are pushed here together for people to run.

https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=cpufreq/cppc

This is tested on my Hikey platform (without the actual read/write to
performance counters), with this script for over an hour:

while true; do
    for i in `seq 1 7`;
    do
        echo 0 > /sys/devices/system/cpu/cpu$i/online;
    done;

    for i in `seq 1 7`;
    do
        echo 1 > /sys/devices/system/cpu/cpu$i/online;
    done;
done


Vincent will be giving this patchset a try on ThunderX2.

Meanwhile it is up for review. Ideally I would like to get this merged for 5.14,
but lets see how it goes.

Thanks.

--
Viresh

Viresh Kumar (3):
  cpufreq: Add start_cpu() and stop_cpu() callbacks
  arch_topology: Avoid use-after-free for scale_freq_data
  cpufreq: CPPC: Add support for frequency invariance

 Documentation/cpu-freq/cpu-drivers.rst |   7 +-
 drivers/base/arch_topology.c           |  27 ++-
 drivers/cpufreq/Kconfig.arm            |  10 ++
 drivers/cpufreq/cppc_cpufreq.c         | 232 +++++++++++++++++++++++--
 drivers/cpufreq/cpufreq.c              |  19 +-
 include/linux/arch_topology.h          |   1 +
 include/linux/cpufreq.h                |   5 +-
 kernel/sched/core.c                    |   1 +
 8 files changed, 272 insertions(+), 30 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH V2 1/3] cpufreq: Add start_cpu() and stop_cpu() callbacks

[Viresh Kumar]

On CPU hot-unplug, the cpufreq core doesn't call any driver specific
callback unless all the CPUs of a policy went away, in which case we
call stop_cpu() callback.

For the CPPC cpufreq driver, we need to perform per-cpu init/exit work
which that can't be performed from policy specific init()/exit()
callbacks.

This patch adds a new callback, start_cpu() and modifies the stop_cpu()
callback, to perform such CPU specific work.

These routines are called whenever a CPU is added or removed from a
policy.

Note that this also moves the setting of policy->cpus to online CPUs
only, outside of rwsem as we needed to call start_cpu() for online CPUs
only. This shouldn't have any side effects.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.rst |  7 +++++--
 drivers/cpufreq/cpufreq.c              | 19 +++++++++++++++----
 include/linux/cpufreq.h                |  5 ++++-
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.rst b/Documentation/cpu-freq/cpu-drivers.rst
index a697278ce190..15cfe42b4075 100644
--- a/Documentation/cpu-freq/cpu-drivers.rst
+++ b/Documentation/cpu-freq/cpu-drivers.rst
@@ -71,8 +71,11 @@ And optionally
  .exit - A pointer to a per-policy cleanup function called during
  CPU_POST_DEAD phase of cpu hotplug process.
 
- .stop_cpu - A pointer to a per-policy stop function called during
- CPU_DOWN_PREPARE phase of cpu hotplug process.
+ .start_cpu - A pointer to a per-policy per-cpu start function called
+ during CPU online phase.
+
+ .stop_cpu - A pointer to a per-policy per-cpu stop function called
+ during CPU offline phase.
 
  .suspend - A pointer to a per-policy suspend function which is called
  with interrupts disabled and _after_ the governor is stopped for the
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 802abc925b2a..128dfb1b0cdf 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1119,6 +1119,10 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp
 
 	cpumask_set_cpu(cpu, policy->cpus);
 
+	/* Do CPU specific initialization if required */
+	if (cpufreq_driver->start_cpu)
+		cpufreq_driver->start_cpu(policy, cpu);
+
 	if (has_target()) {
 		ret = cpufreq_start_governor(policy);
 		if (ret)
@@ -1375,13 +1379,19 @@ static int cpufreq_online(unsigned int cpu)
 		cpumask_copy(policy->related_cpus, policy->cpus);
 	}
 
-	down_write(&policy->rwsem);
 	/*
 	 * affected cpus must always be the one, which are online. We aren't
 	 * managing offline cpus here.
 	 */
 	cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
 
+	/* Do CPU specific initialization if required */
+	if (cpufreq_driver->start_cpu) {
+		for_each_cpu(j, policy->cpus)
+			cpufreq_driver->start_cpu(policy, j);
+	}
+
+	down_write(&policy->rwsem);
 	if (new_policy) {
 		for_each_cpu(j, policy->related_cpus) {
 			per_cpu(cpufreq_cpu_data, j) = policy;
@@ -1581,6 +1591,10 @@ static int cpufreq_offline(unsigned int cpu)
 		policy->cpu = cpumask_any(policy->cpus);
 	}
 
+	/* Do CPU specific de-initialization if required */
+	if (cpufreq_driver->stop_cpu)
+		cpufreq_driver->stop_cpu(policy, cpu);
+
 	/* Start governor again for active policy */
 	if (!policy_is_inactive(policy)) {
 		if (has_target()) {
@@ -1597,9 +1611,6 @@ static int cpufreq_offline(unsigned int cpu)
 		policy->cdev = NULL;
 	}
 
-	if (cpufreq_driver->stop_cpu)
-		cpufreq_driver->stop_cpu(policy);
-
 	if (has_target())
 		cpufreq_exit_governor(policy);
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 353969c7acd3..c281b3df4e2f 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -371,7 +371,10 @@ struct cpufreq_driver {
 	int		(*online)(struct cpufreq_policy *policy);
 	int		(*offline)(struct cpufreq_policy *policy);
 	int		(*exit)(struct cpufreq_policy *policy);
-	void		(*stop_cpu)(struct cpufreq_policy *policy);
+
+	/* CPU specific start/stop */
+	void		(*start_cpu)(struct cpufreq_policy *policy, unsigned int cpu);
+	void		(*stop_cpu)(struct cpufreq_policy *policy, unsigned int cpu);
 	int		(*suspend)(struct cpufreq_policy *policy);
 	int		(*resume)(struct cpufreq_policy *policy);
 
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V2 0/3] cpufreq: cppc: Add support for frequency invariance

[Vincent Guittot]

On Wed, 16 Jun 2021 at 08:48, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Hello,
>
> Changes since V1:
>
> - Few of the patches migrating users to ->exit() callback are posted separately.
>
> - The CPPC patch was completely reverted and so the support for FIE is again
>   added here from scratch.
>
> - The start_cpu() and stop_cpu() interface is reworked a little so stop_cpu() is
>   only ever called for a CPU if start_cpu() was called for it earlier.
>
> - A new patch to implement RCU locking in arch_topology core to avoid some
>   races.
>
> - Some cleanup and very clear/separate paths for FIE in cppc driver now.
>
>
> -------------------------8<-------------------------
>
> CPPC cpufreq driver is used for ARM servers and this patch series tries to
> provide counter-based frequency invariance support for them in the absence for
> architecture specific counters (like AMUs).
>
> This was reverted earlier for the 5.13 kernel after Qian Cai reported kernel
> oops during suspend/resume.
>
> This is based of pm/linux-next + a cleanup patchset:
>
> https://lore.kernel.org/linux-pm/cover.1623825358.git.viresh.kumar@linaro.org/
>
> All the patches are pushed here together for people to run.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=cpufreq/cppc
>
> This is tested on my Hikey platform (without the actual read/write to
> performance counters), with this script for over an hour:
>
> while true; do
>     for i in `seq 1 7`;
>     do
>         echo 0 > /sys/devices/system/cpu/cpu$i/online;
>     done;
>
>     for i in `seq 1 7`;
>     do
>         echo 1 > /sys/devices/system/cpu/cpu$i/online;
>     done;
> done
>
>
> Vincent will be giving this patchset a try on ThunderX2.

I tested your branch and got the following while booting:

[   24.454543] zswap: loaded using pool lzo/zbud
[   24.454753] pstore: Using crash dump compression: deflate
[   24.454776] AppArmor: AppArmor sha1 policy hashing enabled
[   24.454784] ima: No TPM chip found, activating TPM-bypass!
[   24.454789] ima: Allocated hash algorithm: sha256
[   24.454801] ima: No architecture policies found
[   24.455750] pcieport 0000:0f:00.0: Adding to iommu group 0
[   24.893888] ------------[ cut here ]------------
[   24.893891] WARNING: CPU: 95 PID: 1442 at
drivers/cpufreq/cppc_cpufreq.c:123 cppc_scale_freq_workfn+0xc8/0xf8
[   24.893901] Modules linked in:
[   24.893906] CPU: 95 PID: 1442 Comm: cppc_fie Not tainted 5.13.0-rc6+ #359
[   24.893910] Hardware name: To be filled by O.E.M. Saber/Saber, BIOS
0ACKL026 03/19/2019
[   24.893912] pstate: 20400009 (nzCv daif +PAN -UAO -TCO BTYPE=--)
[   24.893915] pc : cppc_scale_freq_workfn+0xc8/0xf8
[   24.893918] lr : cppc_scale_freq_workfn+0x5c/0xf8
[   24.893921] sp : ffff80003727bd90
[   24.893922] x29: ffff80003727bd90 x28: 0000000000000000 x27: ffff800010ec2000
[   24.893928] x26: ffff800010ec2000 x25: ffff8000107c3d90 x24: 0000000000000001
[   24.893932] x23: ffff000816244880 x22: ffff8000113f9000 x21: ffff009f825a0a80
[   24.893935] x20: ffff009efc394220 x19: ffff800011199000 x18: 000000000000001b
[   24.893939] x17: 0000000000000007 x16: 0000000000000001 x15: 00000000000000bf
[   24.893943] x14: 0000000000000016 x13: 000000000000029b x12: 0000000000000016
[   24.893946] x11: 0000000000000000 x10: 0000000000000000 x9 : ffff009efc6958c0
[   24.893950] x8 : ffff009efc394248 x7 : 0000000002bde780 x6 : 00000000ffffffff
[   24.893954] x5 : 00000000916e502a x4 : 00000000d9730e80 x3 : ffffffffffffffff
[   24.893958] x2 : 00000000001e8480 x1 : 00000000002625a0 x0 : 0000000000000401
[   24.893962] Call trace:
[   24.893964]  cppc_scale_freq_workfn+0xc8/0xf8
[   24.893967]  kthread_worker_fn+0x110/0x318
[   24.893971]  kthread+0xf4/0x120
[   24.893973]  ret_from_fork+0x10/0x18
[   24.893977] ---[ end trace ea6dbaf832bce3e4 ]---


>
> Meanwhile it is up for review. Ideally I would like to get this merged for 5.14,
> but lets see how it goes.
>
> Thanks.
>
> --
> Viresh
>
> Viresh Kumar (3):
>   cpufreq: Add start_cpu() and stop_cpu() callbacks
>   arch_topology: Avoid use-after-free for scale_freq_data
>   cpufreq: CPPC: Add support for frequency invariance
>
>  Documentation/cpu-freq/cpu-drivers.rst |   7 +-
>  drivers/base/arch_topology.c           |  27 ++-
>  drivers/cpufreq/Kconfig.arm            |  10 ++
>  drivers/cpufreq/cppc_cpufreq.c         | 232 +++++++++++++++++++++++--
>  drivers/cpufreq/cpufreq.c              |  19 +-
>  include/linux/arch_topology.h          |   1 +
>  include/linux/cpufreq.h                |   5 +-
>  kernel/sched/core.c                    |   1 +
>  8 files changed, 272 insertions(+), 30 deletions(-)
>
> --
> 2.31.1.272.g89b43f80a514
>

Re: [PATCH V2 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

On 16-06-21, 12:02, Vincent Guittot wrote:
> I tested your branch and got the following while booting:
> 
> [   24.454543] zswap: loaded using pool lzo/zbud
> [   24.454753] pstore: Using crash dump compression: deflate
> [   24.454776] AppArmor: AppArmor sha1 policy hashing enabled
> [   24.454784] ima: No TPM chip found, activating TPM-bypass!
> [   24.454789] ima: Allocated hash algorithm: sha256
> [   24.454801] ima: No architecture policies found
> [   24.455750] pcieport 0000:0f:00.0: Adding to iommu group 0
> [   24.893888] ------------[ cut here ]------------
> [   24.893891] WARNING: CPU: 95 PID: 1442 at
> drivers/cpufreq/cppc_cpufreq.c:123 cppc_scale_freq_workfn+0xc8/0xf8
> [   24.893901] Modules linked in:
> [   24.893906] CPU: 95 PID: 1442 Comm: cppc_fie Not tainted 5.13.0-rc6+ #359
> [   24.893910] Hardware name: To be filled by O.E.M. Saber/Saber, BIOS
> 0ACKL026 03/19/2019
> [   24.893912] pstate: 20400009 (nzCv daif +PAN -UAO -TCO BTYPE=--)
> [   24.893915] pc : cppc_scale_freq_workfn+0xc8/0xf8
> [   24.893918] lr : cppc_scale_freq_workfn+0x5c/0xf8
> [   24.893921] sp : ffff80003727bd90
> [   24.893922] x29: ffff80003727bd90 x28: 0000000000000000 x27: ffff800010ec2000
> [   24.893928] x26: ffff800010ec2000 x25: ffff8000107c3d90 x24: 0000000000000001
> [   24.893932] x23: ffff000816244880 x22: ffff8000113f9000 x21: ffff009f825a0a80
> [   24.893935] x20: ffff009efc394220 x19: ffff800011199000 x18: 000000000000001b
> [   24.893939] x17: 0000000000000007 x16: 0000000000000001 x15: 00000000000000bf
> [   24.893943] x14: 0000000000000016 x13: 000000000000029b x12: 0000000000000016
> [   24.893946] x11: 0000000000000000 x10: 0000000000000000 x9 : ffff009efc6958c0
> [   24.893950] x8 : ffff009efc394248 x7 : 0000000002bde780 x6 : 00000000ffffffff
> [   24.893954] x5 : 00000000916e502a x4 : 00000000d9730e80 x3 : ffffffffffffffff
> [   24.893958] x2 : 00000000001e8480 x1 : 00000000002625a0 x0 : 0000000000000401
> [   24.893962] Call trace:
> [   24.893964]  cppc_scale_freq_workfn+0xc8/0xf8
> [   24.893967]  kthread_worker_fn+0x110/0x318
> [   24.893971]  kthread+0xf4/0x120
> [   24.893973]  ret_from_fork+0x10/0x18
> [   24.893977] ---[ end trace ea6dbaf832bce3e4 ]---

Thanks Vincent.

This is triggering from cppc_scale_freq_workfn():

        if (WARN_ON(local_freq_scale > 1024))

Looks like there is something fishy about the perf calculations here
after reading the counters, we tried to scale that in the range 0-1024
and it came larger than that.

Will keep you posted.

-- 
viresh

Re: [PATCH V2 1/3] cpufreq: Add start_cpu() and stop_cpu() callbacks

[Rafael J. Wysocki]

On Wed, Jun 16, 2021 at 8:48 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On CPU hot-unplug, the cpufreq core doesn't call any driver specific
> callback unless all the CPUs of a policy went away, in which case we
> call stop_cpu() callback.
>
> For the CPPC cpufreq driver, we need to perform per-cpu init/exit work
> which that can't be performed from policy specific init()/exit()
> callbacks.
>
> This patch adds a new callback, start_cpu() and modifies the stop_cpu()
> callback, to perform such CPU specific work.
>
> These routines are called whenever a CPU is added or removed from a
> policy.
>
> Note that this also moves the setting of policy->cpus to online CPUs
> only, outside of rwsem as we needed to call start_cpu() for online CPUs
> only. This shouldn't have any side effects.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/cpu-freq/cpu-drivers.rst |  7 +++++--
>  drivers/cpufreq/cpufreq.c              | 19 +++++++++++++++----
>  include/linux/cpufreq.h                |  5 ++++-
>  3 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/cpu-freq/cpu-drivers.rst b/Documentation/cpu-freq/cpu-drivers.rst
> index a697278ce190..15cfe42b4075 100644
> --- a/Documentation/cpu-freq/cpu-drivers.rst
> +++ b/Documentation/cpu-freq/cpu-drivers.rst
> @@ -71,8 +71,11 @@ And optionally
>   .exit - A pointer to a per-policy cleanup function called during
>   CPU_POST_DEAD phase of cpu hotplug process.
>
> - .stop_cpu - A pointer to a per-policy stop function called during
> - CPU_DOWN_PREPARE phase of cpu hotplug process.
> + .start_cpu - A pointer to a per-policy per-cpu start function called
> + during CPU online phase.
> +
> + .stop_cpu - A pointer to a per-policy per-cpu stop function called
> + during CPU offline phase.
>
>   .suspend - A pointer to a per-policy suspend function which is called
>   with interrupts disabled and _after_ the governor is stopped for the
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 802abc925b2a..128dfb1b0cdf 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1119,6 +1119,10 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp
>
>         cpumask_set_cpu(cpu, policy->cpus);
>
> +       /* Do CPU specific initialization if required */
> +       if (cpufreq_driver->start_cpu)
> +               cpufreq_driver->start_cpu(policy, cpu);
> +
>         if (has_target()) {
>                 ret = cpufreq_start_governor(policy);
>                 if (ret)
> @@ -1375,13 +1379,19 @@ static int cpufreq_online(unsigned int cpu)
>                 cpumask_copy(policy->related_cpus, policy->cpus);
>         }
>
> -       down_write(&policy->rwsem);
>         /*
>          * affected cpus must always be the one, which are online. We aren't
>          * managing offline cpus here.
>          */
>         cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
>
> +       /* Do CPU specific initialization if required */
> +       if (cpufreq_driver->start_cpu) {
> +               for_each_cpu(j, policy->cpus)
> +                       cpufreq_driver->start_cpu(policy, j);
> +       }
> +
> +       down_write(&policy->rwsem);
>         if (new_policy) {
>                 for_each_cpu(j, policy->related_cpus) {
>                         per_cpu(cpufreq_cpu_data, j) = policy;
> @@ -1581,6 +1591,10 @@ static int cpufreq_offline(unsigned int cpu)
>                 policy->cpu = cpumask_any(policy->cpus);
>         }
>
> +       /* Do CPU specific de-initialization if required */
> +       if (cpufreq_driver->stop_cpu)
> +               cpufreq_driver->stop_cpu(policy, cpu);
> +
>         /* Start governor again for active policy */
>         if (!policy_is_inactive(policy)) {
>                 if (has_target()) {
> @@ -1597,9 +1611,6 @@ static int cpufreq_offline(unsigned int cpu)
>                 policy->cdev = NULL;
>         }
>
> -       if (cpufreq_driver->stop_cpu)
> -               cpufreq_driver->stop_cpu(policy);
> -

This should be a separate patch IMO, after you've migrated everyone to
->offline/->exit.

BTW, IMO it might be better to migrate ->stop_cpu to ->offline rather
than to ->exit.

>         if (has_target())
>                 cpufreq_exit_governor(policy);
>
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 353969c7acd3..c281b3df4e2f 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -371,7 +371,10 @@ struct cpufreq_driver {
>         int             (*online)(struct cpufreq_policy *policy);
>         int             (*offline)(struct cpufreq_policy *policy);
>         int             (*exit)(struct cpufreq_policy *policy);
> -       void            (*stop_cpu)(struct cpufreq_policy *policy);
> +
> +       /* CPU specific start/stop */
> +       void            (*start_cpu)(struct cpufreq_policy *policy, unsigned int cpu);
> +       void            (*stop_cpu)(struct cpufreq_policy *policy, unsigned int cpu);
>         int             (*suspend)(struct cpufreq_policy *policy);
>         int             (*resume)(struct cpufreq_policy *policy);
>
> --
> 2.31.1.272.g89b43f80a514
>

Re: [PATCH V2 1/3] cpufreq: Add start_cpu() and stop_cpu() callbacks

[Viresh Kumar]

On 17-06-21, 15:33, Rafael J. Wysocki wrote:
> On Wed, Jun 16, 2021 at 8:48 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +       /* Do CPU specific de-initialization if required */
> > +       if (cpufreq_driver->stop_cpu)
> > +               cpufreq_driver->stop_cpu(policy, cpu);
> > +
> >         /* Start governor again for active policy */
> >         if (!policy_is_inactive(policy)) {
> >                 if (has_target()) {
> > @@ -1597,9 +1611,6 @@ static int cpufreq_offline(unsigned int cpu)
> >                 policy->cdev = NULL;
> >         }
> >
> > -       if (cpufreq_driver->stop_cpu)
> > -               cpufreq_driver->stop_cpu(policy);
> > -
> 
> This should be a separate patch IMO, after you've migrated everyone to
> ->offline/->exit.

Right, anyway this patch may not be required anymore. I will send a patch to
remove this.

> BTW, IMO it might be better to migrate ->stop_cpu to ->offline rather
> than to ->exit.

This is a bit tricky IMO.

First, offline() isn't implemented by everyone, out of the three implementations
which were using stop_cpu(), only intel-pstate had offline() as well.

Second, the primary purpose of online/offline callbacks was suspend/resume
oriented and for the same reason, we don't call online() when the policy first
comes up and so in case of errors during bring up, we end up calling exit()
directly and not offline().

IMO this is a very specific thing to drivers and they need to see what fits best
for them, exit() or offline() or both.

-- 
viresh