Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1465237 > unrolled thread

[PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine

Started bySebastian Andrzej Siewior <bigeasy@linutronix.de>
First post2016-08-18 15:00 +0200
Last post2016-08-24 11:10 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-08-18 15:00 +0200
    Re: [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-08-22 18:20 +0200
      Re: [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-08-22 21:10 +0200
        Re: [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-08-23 16:20 +0200
          Re: [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-08-23 18:40 +0200
            [PATCH 10/16 v2] cpuidle: pseries: Convert to hotplug state machine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-08-24 11:10 +0200

#1465237 — [PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine

FromSebastian Andrzej Siewior <bigeasy@linutronix.de>
Date2016-08-18 15:00 +0200
Subject[PATCH 10/16] cpuidle: pseries: Convert to hotplug state machine
Message-ID<s7v5o-8lC-17@gated-at.bofh.it>
Install the callbacks via the state machine.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/cpuidle/cpuidle-pseries.c | 50 +++++++++++++++++----------------------
 include/linux/cpuhotplug.h        |  1 +
 2 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 07135e009d8b..10c4c51cd840 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -171,39 +171,29 @@ static struct cpuidle_state shared_states[] = {
 		.enter = &shared_cede_loop },
 };
 
-static int pseries_cpuidle_add_cpu_notifier(struct notifier_block *n,
-			unsigned long action, void *hcpu)
+static int pseries_cpuidle_cpu_online(unsigned int cpu)
 {
-	int hotcpu = (unsigned long)hcpu;
-	struct cpuidle_device *dev =
-				per_cpu(cpuidle_devices, hotcpu);
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
 
 	if (dev && cpuidle_get_driver()) {
-		switch (action) {
-		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_enable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		case CPU_DEAD:
-		case CPU_DEAD_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_disable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		default:
-			return NOTIFY_DONE;
-		}
+		cpuidle_pause_and_lock();
+		cpuidle_enable_device(dev);
+		cpuidle_resume_and_unlock();
 	}
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block setup_hotplug_notifier = {
-	.notifier_call = pseries_cpuidle_add_cpu_notifier,
-};
+static int pseries_cpuidle_cpu_dead(unsigned int cpu)
+{
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
+
+	if (dev && cpuidle_get_driver()) {
+		cpuidle_pause_and_lock();
+		cpuidle_disable_device(dev);
+		cpuidle_resume_and_unlock();
+	}
+	return 0;
+}
 
 /*
  * pseries_cpuidle_driver_init()
@@ -273,7 +263,11 @@ static int __init pseries_processor_idle_init(void)
 		return retval;
 	}
 
-	register_cpu_notifier(&setup_hotplug_notifier);
+	cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "CPUIDLE_PSERIES_ONLINE",
+				  pseries_cpuidle_cpu_online, NULL);
+	cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_PSERIES_DEAD,
+				  "CPUIDLE_PSERIES_DEAD", NULL,
+				  pseries_cpuidle_cpu_dead);
 	printk(KERN_DEBUG "pseries_idle_driver registered\n");
 	return 0;
 }
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 5811954809af..baecc4faf028 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -19,6 +19,7 @@ enum cpuhp_state {
 	CPUHP_MM_WRITEBACK_DEAD,
 	CPUHP_SOFTIRQ_DEAD,
 	CPUHP_NET_MVNETA_DEAD,
+	CPUHP_CPUIDLE_PSERIES_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
-- 
2.9.3

[toc] | [next] | [standalone]

#1467767

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2016-08-22 18:20 +0200
Message-ID<s9078-6Z-19@gated-at.bofh.it>
In reply to#1465237
On 08/18/2016 02:57 PM, Sebastian Andrzej Siewior wrote:
> Install the callbacks via the state machine.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 5811954809af..baecc4faf028 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -19,6 +19,7 @@ enum cpuhp_state {
>  	CPUHP_MM_WRITEBACK_DEAD,
>  	CPUHP_SOFTIRQ_DEAD,
>  	CPUHP_NET_MVNETA_DEAD,
> +	CPUHP_CPUIDLE_PSERIES_DEAD,

Can't we directly merge these into CPUHP_CPUIDLE_DEAD instead ? Or is it
planned to be done separately ?


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1467964

FromSebastian Andrzej Siewior <bigeasy@linutronix.de>
Date2016-08-22 21:10 +0200
Message-ID<s92LD-1R8-23@gated-at.bofh.it>
In reply to#1467767
On 2016-08-22 18:09:47 [+0200], Daniel Lezcano wrote:
> On 08/18/2016 02:57 PM, Sebastian Andrzej Siewior wrote:
> > Install the callbacks via the state machine.
> > 
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> > index 5811954809af..baecc4faf028 100644
> > --- a/include/linux/cpuhotplug.h
> > +++ b/include/linux/cpuhotplug.h
> > @@ -19,6 +19,7 @@ enum cpuhp_state {
> >  	CPUHP_MM_WRITEBACK_DEAD,
> >  	CPUHP_SOFTIRQ_DEAD,
> >  	CPUHP_NET_MVNETA_DEAD,
> > +	CPUHP_CPUIDLE_PSERIES_DEAD,
> 
> Can't we directly merge these into CPUHP_CPUIDLE_DEAD instead ? Or is it
> planned to be done separately ?

You mean CPUHP_CPUIDLE_DEAD instead of _PSERIES_DEAD and _POWERNV_DEAD?
We could do that but you would have to ensure that only one CPUIDLE
driver registers itself at a time and for those powerpc drivers it looks
like you could have two registered (not sure about ARM's little/big (if
you could have two of those later at run-time)).
For the ONLINE state we have dynamic allocation of IDs. If it is
possible to rework the code to use only ONLINE & PRE_DOWN instead of
DEAD then we wouldn't have this. I can't say at this point if we do
dynamic allocation of the DEAD IDs.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1468579

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2016-08-23 16:20 +0200
Message-ID<s9kIx-563-17@gated-at.bofh.it>
In reply to#1467964
On 08/22/2016 09:04 PM, Sebastian Andrzej Siewior wrote:
> On 2016-08-22 18:09:47 [+0200], Daniel Lezcano wrote:
>> On 08/18/2016 02:57 PM, Sebastian Andrzej Siewior wrote:
>>> Install the callbacks via the state machine.
>>>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: linux-pm@vger.kernel.org
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>> ---
>>> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
>>> index 5811954809af..baecc4faf028 100644
>>> --- a/include/linux/cpuhotplug.h
>>> +++ b/include/linux/cpuhotplug.h
>>> @@ -19,6 +19,7 @@ enum cpuhp_state {
>>>  	CPUHP_MM_WRITEBACK_DEAD,
>>>  	CPUHP_SOFTIRQ_DEAD,
>>>  	CPUHP_NET_MVNETA_DEAD,
>>> +	CPUHP_CPUIDLE_PSERIES_DEAD,
>>
>> Can't we directly merge these into CPUHP_CPUIDLE_DEAD instead ? Or is it
>> planned to be done separately ?
> 
> You mean CPUHP_CPUIDLE_DEAD instead of _PSERIES_DEAD and _POWERNV_DEAD?

Yes. If we can limit the number of duplicating enum for the same purpose
right now, it would be nice.

> We could do that but you would have to ensure that only one CPUIDLE
> driver registers itself at a time and for those powerpc drivers it looks
> like you could have two registered (not sure about ARM's little/big (if
> you could have two of those later at run-time)).

At the first glance, I don't think it is possible to register the cpu
hotplug callback twice because the cpuidle drivers are doing:

...
        retval = cpuidle_register(&pseries_idle_driver, NULL);
        if (retval) {
                printk(KERN_DEBUG "Registration of pseries driver
failed.\n");
                return retval;
        }

        register_cpu_notifier(&setup_hotplug_notifier);

So if a previous driver was already registered, cpuidle_register will
fail and register_cpu_notifier won't be hit.

There is the same scenario for intel_idle and processor_idle (acpi).

> For the ONLINE state we have dynamic allocation of IDs. If it is
> possible to rework the code to use only ONLINE & PRE_DOWN instead of
> DEAD then we wouldn't have this. I can't say at this point if we do
> dynamic allocation of the DEAD IDs.
> 
> Sebastian
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1468695

FromSebastian Andrzej Siewior <bigeasy@linutronix.de>
Date2016-08-23 18:40 +0200
Message-ID<s9mU2-6sX-19@gated-at.bofh.it>
In reply to#1468579
On 2016-08-23 16:16:12 [+0200], Daniel Lezcano wrote:
> Yes. If we can limit the number of duplicating enum for the same purpose
> right now, it would be nice.
Okay.

> > We could do that but you would have to ensure that only one CPUIDLE
> > driver registers itself at a time and for those powerpc drivers it looks
> > like you could have two registered (not sure about ARM's little/big (if
> > you could have two of those later at run-time)).
> 
> At the first glance, I don't think it is possible to register the cpu
> hotplug callback twice because the cpuidle drivers are doing:
…
> So if a previous driver was already registered, cpuidle_register will
> fail and register_cpu_notifier won't be hit.
> 
> There is the same scenario for intel_idle and processor_idle (acpi).

I tried to preserve everything as-is during the conversation. However if
you are explicitly asking for this and you are sure that it will work
then you can get it. No problem :)

Sebastian

[toc] | [prev] | [next] | [standalone]

#1469239 — [PATCH 10/16 v2] cpuidle: pseries: Convert to hotplug state machine

FromSebastian Andrzej Siewior <bigeasy@linutronix.de>
Date2016-08-24 11:10 +0200
Subject[PATCH 10/16 v2] cpuidle: pseries: Convert to hotplug state machine
Message-ID<s9Cm6-8m-9@gated-at.bofh.it>
In reply to#1468695
Install the callbacks via the state machine.

v1…v2: Use only CPUHP_CPUIDLE_DEAD (requested by Daniel Lezcano)

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/cpuidle/cpuidle-pseries.c | 53 ++++++++++++++++++---------------------
 include/linux/cpuhotplug.h        |  1 +
 2 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 07135e009d8b..abbce8f96d93 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -171,39 +171,29 @@ static struct cpuidle_state shared_states[] = {
 		.enter = &shared_cede_loop },
 };
 
-static int pseries_cpuidle_add_cpu_notifier(struct notifier_block *n,
-			unsigned long action, void *hcpu)
+static int pseries_cpuidle_cpu_online(unsigned int cpu)
 {
-	int hotcpu = (unsigned long)hcpu;
-	struct cpuidle_device *dev =
-				per_cpu(cpuidle_devices, hotcpu);
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
 
 	if (dev && cpuidle_get_driver()) {
-		switch (action) {
-		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_enable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		case CPU_DEAD:
-		case CPU_DEAD_FROZEN:
-			cpuidle_pause_and_lock();
-			cpuidle_disable_device(dev);
-			cpuidle_resume_and_unlock();
-			break;
-
-		default:
-			return NOTIFY_DONE;
-		}
+		cpuidle_pause_and_lock();
+		cpuidle_enable_device(dev);
+		cpuidle_resume_and_unlock();
 	}
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block setup_hotplug_notifier = {
-	.notifier_call = pseries_cpuidle_add_cpu_notifier,
-};
+static int pseries_cpuidle_cpu_dead(unsigned int cpu)
+{
+	struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
+
+	if (dev && cpuidle_get_driver()) {
+		cpuidle_pause_and_lock();
+		cpuidle_disable_device(dev);
+		cpuidle_resume_and_unlock();
+	}
+	return 0;
+}
 
 /*
  * pseries_cpuidle_driver_init()
@@ -273,7 +263,14 @@ static int __init pseries_processor_idle_init(void)
 		return retval;
 	}
 
-	register_cpu_notifier(&setup_hotplug_notifier);
+	retval = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+					   "CPUIDLE_PSERIES_ONLINE",
+					   pseries_cpuidle_cpu_online, NULL);
+	WARN_ON(retval < 0);
+	retval = cpuhp_setup_state_nocalls(CPUHP_CPUIDLE_DEAD,
+					   "CPUIDLE_PSERIES_DEAD", NULL,
+					   pseries_cpuidle_cpu_dead);
+	WARN_ON(retval < 0);
 	printk(KERN_DEBUG "pseries_idle_driver registered\n");
 	return 0;
 }
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 5811954809af..14b58a9d1450 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -19,6 +19,7 @@ enum cpuhp_state {
 	CPUHP_MM_WRITEBACK_DEAD,
 	CPUHP_SOFTIRQ_DEAD,
 	CPUHP_NET_MVNETA_DEAD,
+	CPUHP_CPUIDLE_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
-- 
2.9.3

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web