[PATCH] cpuidle: psci: Support CPU hotplug for the hierarchical model

[PATCH] cpuidle: psci: Support CPU hotplug for the hierarchical model

[Ulf Hansson]

When the hierarchical CPU topology is used and when a CPU is put offline,
that CPU prevents its PM domain from being powered off, which is because
genpd observes the corresponding attached device as being active from a
runtime PM point of view. Furthermore, any potential master PM domains are
also prevented from being powered off.

To address this limitation, let's add add a new CPU hotplug state
(CPUHP_AP_CPU_PM_STARTING) and register up/down callbacks for it, which
allows us to deal with runtime PM accordingly.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Lorenzo, Sudeep, Rafael, Daniel,

Note that, this patch is based upon a recently posted series [1] and the intent
is to queue it on top. I can fold it into the series and resend it all, if that
makes it easier for people. Just tell me what you prefer.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/cover/11263735/

---
 drivers/cpuidle/cpuidle-psci.c | 45 +++++++++++++++++++++++++++++++++-
 include/linux/cpuhotplug.h     |  1 +
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 835c7c3aa118..46b481c524cc 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -8,6 +8,7 @@
 
 #define pr_fmt(fmt) "CPUidle PSCI: " fmt
 
+#include <linux/cpuhotplug.h>
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
@@ -31,6 +32,7 @@ struct psci_cpuidle_data {
 
 static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
 static DEFINE_PER_CPU(u32, domain_state);
+static bool psci_cpuidle_use_cpuhp;
 
 void psci_set_domain_state(u32 state)
 {
@@ -72,6 +74,44 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	return ret;
 }
 
+static int psci_idle_cpuhp_up(unsigned int cpu)
+{
+	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
+
+	if (pd_dev)
+		pm_runtime_get_sync(pd_dev);
+
+	return 0;
+}
+
+static int psci_idle_cpuhp_down(unsigned int cpu)
+{
+	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
+
+	if (pd_dev) {
+		pm_runtime_put_sync(pd_dev);
+		/* Clear domain state to start fresh at next online. */
+		psci_set_domain_state(0);
+	}
+
+	return 0;
+}
+
+static void psci_idle_init_cpuhp(void)
+{
+	int err;
+
+	if (!psci_cpuidle_use_cpuhp)
+		return;
+
+	err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,
+					"cpuidle/psci:online",
+					psci_idle_cpuhp_up,
+					psci_idle_cpuhp_down);
+	if (err)
+		pr_warn("Failed %d while setup cpuhp state\n", err);
+}
+
 static int psci_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
@@ -161,9 +201,11 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 	}
 
 	/* Manage the deepest state via a dedicated enter-function. */
-	if (dev)
+	if (dev) {
 		drv->states[state_count - 1].enter =
 			psci_enter_domain_idle_state;
+		psci_cpuidle_use_cpuhp = true;
+	}
 
 	data->dev = dev;
 
@@ -285,6 +327,7 @@ static int __init psci_idle_init(void)
 			goto out_fail;
 	}
 
+	psci_idle_init_cpuhp();
 	return 0;
 
 out_fail:
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index e51ee772b9f5..01f04ed6ad92 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -95,6 +95,7 @@ enum cpuhp_state {
 	CPUHP_AP_OFFLINE,
 	CPUHP_AP_SCHED_STARTING,
 	CPUHP_AP_RCUTREE_DYING,
+	CPUHP_AP_CPU_PM_STARTING,
 	CPUHP_AP_IRQ_GIC_STARTING,
 	CPUHP_AP_IRQ_HIP04_STARTING,
 	CPUHP_AP_IRQ_ARMADA_XP_STARTING,
-- 
2.17.1

Re: [PATCH] cpuidle: psci: Support CPU hotplug for the hierarchical model

[Lorenzo Pieralisi]

On Thu, Dec 05, 2019 at 11:33:30AM +0100, Ulf Hansson wrote:
> When the hierarchical CPU topology is used and when a CPU is put offline,
> that CPU prevents its PM domain from being powered off, which is because
> genpd observes the corresponding attached device as being active from a
> runtime PM point of view. Furthermore, any potential master PM domains are
> also prevented from being powered off.
> 
> To address this limitation, let's add add a new CPU hotplug state
> (CPUHP_AP_CPU_PM_STARTING) and register up/down callbacks for it, which
> allows us to deal with runtime PM accordingly.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Lorenzo, Sudeep, Rafael, Daniel,
> 
> Note that, this patch is based upon a recently posted series [1] and the intent
> is to queue it on top. I can fold it into the series and resend it all, if that
> makes it easier for people. Just tell me what you prefer.
> 
> Kind regards
> Uffe
> 
> [1]
> https://patchwork.kernel.org/cover/11263735/
> 
> ---
>  drivers/cpuidle/cpuidle-psci.c | 45 +++++++++++++++++++++++++++++++++-
>  include/linux/cpuhotplug.h     |  1 +
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 835c7c3aa118..46b481c524cc 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -8,6 +8,7 @@
>  
>  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
>  
> +#include <linux/cpuhotplug.h>
>  #include <linux/cpuidle.h>
>  #include <linux/cpumask.h>
>  #include <linux/cpu_pm.h>
> @@ -31,6 +32,7 @@ struct psci_cpuidle_data {
>  
>  static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
>  static DEFINE_PER_CPU(u32, domain_state);
> +static bool psci_cpuidle_use_cpuhp;
>  
>  void psci_set_domain_state(u32 state)
>  {
> @@ -72,6 +74,44 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
>  	return ret;
>  }
>  
> +static int psci_idle_cpuhp_up(unsigned int cpu)
> +{
> +	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
> +
> +	if (pd_dev)
> +		pm_runtime_get_sync(pd_dev);
> +
> +	return 0;
> +}
> +
> +static int psci_idle_cpuhp_down(unsigned int cpu)
> +{
> +	struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
> +
> +	if (pd_dev) {
> +		pm_runtime_put_sync(pd_dev);
> +		/* Clear domain state to start fresh at next online. */
> +		psci_set_domain_state(0);
> +	}
> +
> +	return 0;
> +}
> +
> +static void psci_idle_init_cpuhp(void)
> +{
> +	int err;
> +
> +	if (!psci_cpuidle_use_cpuhp)
> +		return;
> +
> +	err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,
> +					"cpuidle/psci:online",
> +					psci_idle_cpuhp_up,
> +					psci_idle_cpuhp_down);

I would make it a cpuhp_setup_state() call and remove

if (cpu_online(cpu))
	pm_runtime_get_sync_dev(dev);

in check in psci_dt_attach_cpu().

Lorenzo

> +	if (err)
> +		pr_warn("Failed %d while setup cpuhp state\n", err);
> +}
> +
>  static int psci_enter_idle_state(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv, int idx)
>  {
> @@ -161,9 +201,11 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
>  	}
>  
>  	/* Manage the deepest state via a dedicated enter-function. */
> -	if (dev)
> +	if (dev) {
>  		drv->states[state_count - 1].enter =
>  			psci_enter_domain_idle_state;
> +		psci_cpuidle_use_cpuhp = true;
> +	}
>  
>  	data->dev = dev;
>  
> @@ -285,6 +327,7 @@ static int __init psci_idle_init(void)
>  			goto out_fail;
>  	}
>  
> +	psci_idle_init_cpuhp();
>  	return 0;
>  
>  out_fail:
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index e51ee772b9f5..01f04ed6ad92 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -95,6 +95,7 @@ enum cpuhp_state {
>  	CPUHP_AP_OFFLINE,
>  	CPUHP_AP_SCHED_STARTING,
>  	CPUHP_AP_RCUTREE_DYING,
> +	CPUHP_AP_CPU_PM_STARTING,
>  	CPUHP_AP_IRQ_GIC_STARTING,
>  	CPUHP_AP_IRQ_HIP04_STARTING,
>  	CPUHP_AP_IRQ_ARMADA_XP_STARTING,
> -- 
> 2.17.1
> 

Re: [PATCH] cpuidle: psci: Support CPU hotplug for the hierarchical model

[Ulf Hansson]

On Thu, 5 Dec 2019 at 19:07, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Dec 05, 2019 at 11:33:30AM +0100, Ulf Hansson wrote:
> > When the hierarchical CPU topology is used and when a CPU is put offline,
> > that CPU prevents its PM domain from being powered off, which is because
> > genpd observes the corresponding attached device as being active from a
> > runtime PM point of view. Furthermore, any potential master PM domains are
> > also prevented from being powered off.
> >
> > To address this limitation, let's add add a new CPU hotplug state
> > (CPUHP_AP_CPU_PM_STARTING) and register up/down callbacks for it, which
> > allows us to deal with runtime PM accordingly.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Lorenzo, Sudeep, Rafael, Daniel,
> >
> > Note that, this patch is based upon a recently posted series [1] and the intent
> > is to queue it on top. I can fold it into the series and resend it all, if that
> > makes it easier for people. Just tell me what you prefer.
> >
> > Kind regards
> > Uffe
> >
> > [1]
> > https://patchwork.kernel.org/cover/11263735/
> >
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 45 +++++++++++++++++++++++++++++++++-
> >  include/linux/cpuhotplug.h     |  1 +
> >  2 files changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 835c7c3aa118..46b481c524cc 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -8,6 +8,7 @@
> >
> >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> >
> > +#include <linux/cpuhotplug.h>
> >  #include <linux/cpuidle.h>
> >  #include <linux/cpumask.h>
> >  #include <linux/cpu_pm.h>
> > @@ -31,6 +32,7 @@ struct psci_cpuidle_data {
> >
> >  static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
> >  static DEFINE_PER_CPU(u32, domain_state);
> > +static bool psci_cpuidle_use_cpuhp;
> >
> >  void psci_set_domain_state(u32 state)
> >  {
> > @@ -72,6 +74,44 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> >       return ret;
> >  }
> >
> > +static int psci_idle_cpuhp_up(unsigned int cpu)
> > +{
> > +     struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
> > +
> > +     if (pd_dev)
> > +             pm_runtime_get_sync(pd_dev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int psci_idle_cpuhp_down(unsigned int cpu)
> > +{
> > +     struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
> > +
> > +     if (pd_dev) {
> > +             pm_runtime_put_sync(pd_dev);
> > +             /* Clear domain state to start fresh at next online. */
> > +             psci_set_domain_state(0);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void psci_idle_init_cpuhp(void)
> > +{
> > +     int err;
> > +
> > +     if (!psci_cpuidle_use_cpuhp)
> > +             return;
> > +
> > +     err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,
> > +                                     "cpuidle/psci:online",
> > +                                     psci_idle_cpuhp_up,
> > +                                     psci_idle_cpuhp_down);
>
> I would make it a cpuhp_setup_state() call and remove
>
> if (cpu_online(cpu))
>         pm_runtime_get_sync_dev(dev);
>
> in check in psci_dt_attach_cpu().
>
> Lorenzo

Thanks for the suggestion, but that doesn't work when
CONFIG_CPU_HOTPLUG is unset.

As a matter of fact, I just realized that I haven't tried to compile
without that Kconfig option. I should probably add a stub for
psci_idle_init_cpuhp() to address that.

>
> > +     if (err)
> > +             pr_warn("Failed %d while setup cpuhp state\n", err);
> > +}
> > +
> >  static int psci_enter_idle_state(struct cpuidle_device *dev,
> >                               struct cpuidle_driver *drv, int idx)
> >  {
> > @@ -161,9 +201,11 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> >       }
> >
> >       /* Manage the deepest state via a dedicated enter-function. */
> > -     if (dev)
> > +     if (dev) {
> >               drv->states[state_count - 1].enter =
> >                       psci_enter_domain_idle_state;
> > +             psci_cpuidle_use_cpuhp = true;
> > +     }
> >
> >       data->dev = dev;
> >
> > @@ -285,6 +327,7 @@ static int __init psci_idle_init(void)
> >                       goto out_fail;
> >       }
> >
> > +     psci_idle_init_cpuhp();
> >       return 0;
> >
> >  out_fail:
> > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> > index e51ee772b9f5..01f04ed6ad92 100644
> > --- a/include/linux/cpuhotplug.h
> > +++ b/include/linux/cpuhotplug.h
> > @@ -95,6 +95,7 @@ enum cpuhp_state {
> >       CPUHP_AP_OFFLINE,
> >       CPUHP_AP_SCHED_STARTING,
> >       CPUHP_AP_RCUTREE_DYING,
> > +     CPUHP_AP_CPU_PM_STARTING,
> >       CPUHP_AP_IRQ_GIC_STARTING,
> >       CPUHP_AP_IRQ_HIP04_STARTING,
> >       CPUHP_AP_IRQ_ARMADA_XP_STARTING,
> > --
> > 2.17.1
> >

Kind regards
Uffe

Re: [PATCH] cpuidle: psci: Support CPU hotplug for the hierarchical model

[Ulf Hansson]

On Thu, 5 Dec 2019 at 20:09, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 5 Dec 2019 at 19:07, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Thu, Dec 05, 2019 at 11:33:30AM +0100, Ulf Hansson wrote:
> > > When the hierarchical CPU topology is used and when a CPU is put offline,
> > > that CPU prevents its PM domain from being powered off, which is because
> > > genpd observes the corresponding attached device as being active from a
> > > runtime PM point of view. Furthermore, any potential master PM domains are
> > > also prevented from being powered off.
> > >
> > > To address this limitation, let's add add a new CPU hotplug state
> > > (CPUHP_AP_CPU_PM_STARTING) and register up/down callbacks for it, which
> > > allows us to deal with runtime PM accordingly.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >
> > > Lorenzo, Sudeep, Rafael, Daniel,
> > >
> > > Note that, this patch is based upon a recently posted series [1] and the intent
> > > is to queue it on top. I can fold it into the series and resend it all, if that
> > > makes it easier for people. Just tell me what you prefer.
> > >
> > > Kind regards
> > > Uffe
> > >
> > > [1]
> > > https://patchwork.kernel.org/cover/11263735/
> > >
> > > ---
> > >  drivers/cpuidle/cpuidle-psci.c | 45 +++++++++++++++++++++++++++++++++-
> > >  include/linux/cpuhotplug.h     |  1 +
> > >  2 files changed, 45 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > > index 835c7c3aa118..46b481c524cc 100644
> > > --- a/drivers/cpuidle/cpuidle-psci.c
> > > +++ b/drivers/cpuidle/cpuidle-psci.c
> > > @@ -8,6 +8,7 @@
> > >
> > >  #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > >
> > > +#include <linux/cpuhotplug.h>
> > >  #include <linux/cpuidle.h>
> > >  #include <linux/cpumask.h>
> > >  #include <linux/cpu_pm.h>
> > > @@ -31,6 +32,7 @@ struct psci_cpuidle_data {
> > >
> > >  static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
> > >  static DEFINE_PER_CPU(u32, domain_state);
> > > +static bool psci_cpuidle_use_cpuhp;
> > >
> > >  void psci_set_domain_state(u32 state)
> > >  {
> > > @@ -72,6 +74,44 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > >       return ret;
> > >  }
> > >
> > > +static int psci_idle_cpuhp_up(unsigned int cpu)
> > > +{
> > > +     struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
> > > +
> > > +     if (pd_dev)
> > > +             pm_runtime_get_sync(pd_dev);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int psci_idle_cpuhp_down(unsigned int cpu)
> > > +{
> > > +     struct device *pd_dev = __this_cpu_read(psci_cpuidle_data.dev);
> > > +
> > > +     if (pd_dev) {
> > > +             pm_runtime_put_sync(pd_dev);
> > > +             /* Clear domain state to start fresh at next online. */
> > > +             psci_set_domain_state(0);
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void psci_idle_init_cpuhp(void)
> > > +{
> > > +     int err;
> > > +
> > > +     if (!psci_cpuidle_use_cpuhp)
> > > +             return;
> > > +
> > > +     err = cpuhp_setup_state_nocalls(CPUHP_AP_CPU_PM_STARTING,
> > > +                                     "cpuidle/psci:online",
> > > +                                     psci_idle_cpuhp_up,
> > > +                                     psci_idle_cpuhp_down);
> >
> > I would make it a cpuhp_setup_state() call and remove
> >
> > if (cpu_online(cpu))
> >         pm_runtime_get_sync_dev(dev);
> >
> > in check in psci_dt_attach_cpu().
> >
> > Lorenzo
>
> Thanks for the suggestion, but that doesn't work when
> CONFIG_CPU_HOTPLUG is unset.
>
> As a matter of fact, I just realized that I haven't tried to compile
> without that Kconfig option. I should probably add a stub for
> psci_idle_init_cpuhp() to address that.

Just forget about what I said above, it's nonsense.

cpuhp_setup_state() and friends works fine even if CONFIG_CPU_HOTPLUG
is unset. I did test this, but some reason I forgot about it.

However, there is another problem with your proposal above, which is
that the CPUHP callback is assigned after the cpuidle drivers (one per
CPU) have been registered. Which means, CPUs may enter idle even
before the CPUHP callbacks are registered. In other words, we need to
keep the call to pm_runtime_get_sync_dev() in psci_dt_attach_cpu() as
to properly reflect the state of the CPU.

[...]

BTW, I am about to post a new version in a couple of hours. Any other
thing you want to point out, before the next submission?

Kind regards
Uffe