[PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Brian King]

Rather than calling disable_nonboot_cpus and enable_nonboot_cpus
directly, wrapper the calls in a weak arch_suspend_disable_nonboot_cpus
and arch_suspend_enable_nonboot_cpus that can be overridden by
architectures that require different handling of suspending processors
at suspend time than these functions provide. This is needed to enable
suspend/resume on IBM Power servers.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 include/linux/suspend.h |   18 ++++++++++++++++++
 kernel/power/suspend.c  |   14 ++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff -puN kernel/power/suspend.c~suspend_arch_smp kernel/power/suspend.c
--- linux-2.6/kernel/power/suspend.c~suspend_arch_smp	2010-02-17 09:46:59.000000000 -0600
+++ linux-2.6-bjking1/kernel/power/suspend.c	2010-02-17 09:46:59.000000000 -0600
@@ -116,6 +116,16 @@ void __attribute__ ((weak)) arch_suspend
 	local_irq_enable();
 }
 
+int __attribute__ ((weak)) arch_suspend_disable_nonboot_cpus(void)
+{
+	return disable_nonboot_cpus();
+}
+
+void __attribute__ ((weak)) arch_suspend_enable_nonboot_cpus(void)
+{
+	return enable_nonboot_cpus();
+}
+
 /**
  *	suspend_enter - enter the desired system sleep state.
  *	@state:		state to enter
@@ -147,7 +157,7 @@ static int suspend_enter(suspend_state_t
 	if (suspend_test(TEST_PLATFORM))
 		goto Platform_wake;
 
-	error = disable_nonboot_cpus();
+	error = arch_suspend_disable_nonboot_cpus();
 	if (error || suspend_test(TEST_CPUS))
 		goto Enable_cpus;
 
@@ -165,7 +175,7 @@ static int suspend_enter(suspend_state_t
 	BUG_ON(irqs_disabled());
 
  Enable_cpus:
-	enable_nonboot_cpus();
+	arch_suspend_enable_nonboot_cpus();
 
  Platform_wake:
 	if (suspend_ops->wake)
diff -puN include/linux/suspend.h~suspend_arch_smp include/linux/suspend.h
--- linux-2.6/include/linux/suspend.h~suspend_arch_smp	2010-02-17 09:46:59.000000000 -0600
+++ linux-2.6-bjking1/include/linux/suspend.h	2010-02-17 09:46:59.000000000 -0600
@@ -141,6 +141,24 @@ extern void arch_suspend_disable_irqs(vo
  */
 extern void arch_suspend_enable_irqs(void);
 
+/**
+ * arch_suspend_disable_nonboot_cpus - disable non boot for suspend
+ *
+ * Disables non boot CPUs (in the default case). This is a weak symbol in the common
+ * code and thus allows architectures to override it if more needs to be
+ * done. Not called for suspend to disk.
+ */
+extern int arch_suspend_disable_nonboot_cpus(void);
+
+/**
+ * arch_suspend_enable_nonboot_cpus - enable non boot after suspend
+ *
+ * Enables non boot CPUs (in the default case). This is a weak symbol in the common
+ * code and thus allows architectures to override it if more needs to be
+ * done. Not called for suspend to disk.
+ */
+extern void arch_suspend_enable_nonboot_cpus(void);
+
 extern int pm_suspend(suspend_state_t state);
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
_

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Brian King]

On 02/21/2010 01:18 PM, Pavel Machek wrote:
> On Sun 2010-02-21 10:32:02, Brian King wrote:
>>
>> Rather than calling disable_nonboot_cpus and enable_nonboot_cpus
>> directly, wrapper the calls in a weak arch_suspend_disable_nonboot_cpus
>> and arch_suspend_enable_nonboot_cpus that can be overridden by
>> architectures that require different handling of suspending processors
>> at suspend time than these functions provide. This is needed to enable
>> suspend/resume on IBM Power servers.
> 
> This needs *way* better explanation. Like: why do power servers
> require special handling? What else is disable_nonboot_cpus used for?

disable_nonboot_cpus ends up calling _cpu_down, which is also used in
the CPU hotplug path.

The issue is essentially that on Power servers, the state that a CPU
needs to be in when hotplug removing it is different than the state it
needs to be in for suspend.

Here is the way that suspend works on Power servers. For each CPU, we
must make a call to the hypervisor (H_JOIN). For all but the last CPU,
this call does not return. For the last CPU, it will return with a
response indicating it is the last hardware thread. At that point, we
need to make a different call (ibm,suspend-me) to the hypervisor to
indicate we are suspending the partition. When the partition resumes,
the CPU thread that made the ibm,suspend-me call will wake up. It then
makes a call to the hypevisor (H_PROD) for each other CPU to wake up the
other CPUs.

So, for Power, the arch_suspend_disable_nonboot_cpus implementation will
call H_JOIN for all other CPUs. Then the suspend enter call just calls
ibm,suspend-me to suspend the partition. On resume,
arch_suspend_enable_nonboot_cpus sends the H_PROD to each CPU, waking it up.

Adding these arch hooks seemed like the cleanest way to accomplish all this.

Thanks,

Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Brian King]

On 02/21/2010 04:08 PM, Rafael J. Wysocki wrote:
> I'm not a big fan of __attribute__ ((weak)), though.  While we already use that
> in the suspend code, I'm not particularly comfortable with it.
> 
> Have you considered any alternative approaches?

I suppose another option would be to implement this similar to how
arch_free_page and arch_alloc_page do. Something like this:

#ifndef CONFIG_HAVE_ARCH_SUSPEND_CPUS
static inline int arch_suspend_disable_nonboot_cpus(void)
{
	return disable_nonboot_cpus();
}

static inline void arch_suspend_enable_nonboot_cpus(void)
{
	return enable_nonboot_cpus()'
}
#else
extern int arch_suspend_disable_nonboot_cpus(void);
extern void arch_suspend_enable_nonboot_cpus(void);
#endif

I figured I would just be consistent with arch_suspend_disable_irqs /
arch_suspend_enable_irqs.

-Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Brian King]

On 02/21/2010 04:27 PM, Rafael J. Wysocki wrote:
> On Sunday 21 February 2010, Brian King wrote:
>> On 02/21/2010 04:08 PM, Rafael J. Wysocki wrote:
>>> I'm not a big fan of __attribute__ ((weak)), though.  While we already use that
>>> in the suspend code, I'm not particularly comfortable with it.
>>>
>>> Have you considered any alternative approaches?
>>
>> I suppose another option would be to implement this similar to how
>> arch_free_page and arch_alloc_page do. Something like this:
>>
>> #ifndef CONFIG_HAVE_ARCH_SUSPEND_CPUS
>> static inline int arch_suspend_disable_nonboot_cpus(void)
>> {
>> 	return disable_nonboot_cpus();
>> }
>>
>> static inline void arch_suspend_enable_nonboot_cpus(void)
>> {
>> 	return enable_nonboot_cpus()'
>> }
>> #else
>> extern int arch_suspend_disable_nonboot_cpus(void);
>> extern void arch_suspend_enable_nonboot_cpus(void);
>> #endif
>>
>> I figured I would just be consistent with arch_suspend_disable_irqs /
>> arch_suspend_enable_irqs.
> 
> I just think that doing arch_suspend_[enable|disable]_irqs() this way was
> a mistake.

Do you prefer the example above? I can send an updated patch. If not,
any other suggestions you might have as to the way you would like this
done would be greatly appreciated.

Thanks,

Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Brian King]

On 02/21/2010 04:37 PM, Rafael J. Wysocki wrote:
> On Sunday 21 February 2010, Brian King wrote:
>> On 02/21/2010 04:27 PM, Rafael J. Wysocki wrote:
>>> On Sunday 21 February 2010, Brian King wrote:
>>>> On 02/21/2010 04:08 PM, Rafael J. Wysocki wrote:
>>>>> I'm not a big fan of __attribute__ ((weak)), though.  While we already use that
>>>>> in the suspend code, I'm not particularly comfortable with it.
>>>>>
>>>>> Have you considered any alternative approaches?
>>>>
>>>> I suppose another option would be to implement this similar to how
>>>> arch_free_page and arch_alloc_page do. Something like this:
>>>>
>>>> #ifndef CONFIG_HAVE_ARCH_SUSPEND_CPUS
>>>> static inline int arch_suspend_disable_nonboot_cpus(void)
>>>> {
>>>> 	return disable_nonboot_cpus();
>>>> }
>>>>
>>>> static inline void arch_suspend_enable_nonboot_cpus(void)
>>>> {
>>>> 	return enable_nonboot_cpus()'
>>>> }
>>>> #else
>>>> extern int arch_suspend_disable_nonboot_cpus(void);
>>>> extern void arch_suspend_enable_nonboot_cpus(void);
>>>> #endif
>>>>
>>>> I figured I would just be consistent with arch_suspend_disable_irqs /
>>>> arch_suspend_enable_irqs.
>>>
>>> I just think that doing arch_suspend_[enable|disable]_irqs() this way was
>>> a mistake.
>>
>> Do you prefer the example above? I can send an updated patch. If not,
>> any other suggestions you might have as to the way you would like this
>> done would be greatly appreciated.
> 
> disable_nonboot_cpus() is also called by kernel_power_off().  Is that fine
> with your architecture?

Yes. We only need a different behavior for the suspend/resume path.
Here is an alternative implementation of the patch. My test machine is
currently unavailable, so it is not yet been tested. How does this one look?

Thanks,

Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Rather than calling disable_nonboot_cpus and enable_nonboot_cpus
directly, wrapper the calls in arch_suspend_disable_nonboot_cpus
and arch_suspend_enable_nonboot_cpus that can be overridden by
architectures that require different handling of suspending processors
at suspend time than these functions provide. This is needed to enable
suspend/resume on IBM Power servers.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 include/linux/suspend.h |   27 +++++++++++++++++++++++++++
 kernel/power/suspend.c  |    4 ++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff -puN include/linux/suspend.h~suspend_new_arch_smp include/linux/suspend.h
--- linux-2.6/include/linux/suspend.h~suspend_new_arch_smp	2010-02-21 16:32:04.000000000 -0600
+++ linux-2.6-bjking1/include/linux/suspend.h	2010-02-21 16:45:36.000000000 -0600
@@ -141,6 +141,33 @@ extern void arch_suspend_disable_irqs(vo
  */
 extern void arch_suspend_enable_irqs(void);
 
+#ifdef CONFIG_HAVE_ARCH_SUSPEND_CPUS
+extern int arch_suspend_disable_nonboot_cpus(void);
+extern void arch_suspend_enable_nonboot_cpus(void);
+#else
+/**
+ * arch_suspend_disable_nonboot_cpus - disable non boot for suspend
+ *
+ * Disables non boot CPUs (in the default case). Architectures can
+ * override this if more needs to be done. Not called for suspend to disk.
+ */
+static inline int arch_suspend_disable_nonboot_cpus(void)
+{
+	return disable_nonboot_cpus();
+}
+
+/**
+ * arch_suspend_enable_nonboot_cpus - enable non boot after suspend
+ *
+ * Enables non boot CPUs (in the default case). Architectures can
+ * override this if more needs to be done. Not called for suspend to disk.
+ */
+static inline void arch_suspend_enable_nonboot_cpus(void)
+{
+	enable_nonboot_cpus();
+}
+#endif
+
 extern int pm_suspend(suspend_state_t state);
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
diff -puN kernel/power/suspend.c~suspend_new_arch_smp kernel/power/suspend.c
--- linux-2.6/kernel/power/suspend.c~suspend_new_arch_smp	2010-02-21 16:34:00.000000000 -0600
+++ linux-2.6-bjking1/kernel/power/suspend.c	2010-02-21 16:34:35.000000000 -0600
@@ -147,7 +147,7 @@ static int suspend_enter(suspend_state_t
 	if (suspend_test(TEST_PLATFORM))
 		goto Platform_wake;
 
-	error = disable_nonboot_cpus();
+	error = arch_suspend_disable_nonboot_cpus();
 	if (error || suspend_test(TEST_CPUS))
 		goto Enable_cpus;
 
@@ -165,7 +165,7 @@ static int suspend_enter(suspend_state_t
 	BUG_ON(irqs_disabled());
 
  Enable_cpus:
-	enable_nonboot_cpus();
+	arch_suspend_enable_nonboot_cpus();
 
  Platform_wake:
 	if (suspend_ops->wake)
_

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Rafael J. Wysocki]

On Sunday 21 February 2010, Brian King wrote:
> On 02/21/2010 04:37 PM, Rafael J. Wysocki wrote:
> > On Sunday 21 February 2010, Brian King wrote:
> >> On 02/21/2010 04:27 PM, Rafael J. Wysocki wrote:
> >>> On Sunday 21 February 2010, Brian King wrote:
> >>>> On 02/21/2010 04:08 PM, Rafael J. Wysocki wrote:
> >>>>> I'm not a big fan of __attribute__ ((weak)), though.  While we already use that
> >>>>> in the suspend code, I'm not particularly comfortable with it.
> >>>>>
> >>>>> Have you considered any alternative approaches?
> >>>>
> >>>> I suppose another option would be to implement this similar to how
> >>>> arch_free_page and arch_alloc_page do. Something like this:
> >>>>
> >>>> #ifndef CONFIG_HAVE_ARCH_SUSPEND_CPUS
> >>>> static inline int arch_suspend_disable_nonboot_cpus(void)
> >>>> {
> >>>> 	return disable_nonboot_cpus();
> >>>> }
> >>>>
> >>>> static inline void arch_suspend_enable_nonboot_cpus(void)
> >>>> {
> >>>> 	return enable_nonboot_cpus()'
> >>>> }
> >>>> #else
> >>>> extern int arch_suspend_disable_nonboot_cpus(void);
> >>>> extern void arch_suspend_enable_nonboot_cpus(void);
> >>>> #endif
> >>>>
> >>>> I figured I would just be consistent with arch_suspend_disable_irqs /
> >>>> arch_suspend_enable_irqs.
> >>>
> >>> I just think that doing arch_suspend_[enable|disable]_irqs() this way was
> >>> a mistake.
> >>
> >> Do you prefer the example above? I can send an updated patch. If not,
> >> any other suggestions you might have as to the way you would like this
> >> done would be greatly appreciated.
> > 
> > disable_nonboot_cpus() is also called by kernel_power_off().  Is that fine
> > with your architecture?
> 
> Yes. We only need a different behavior for the suspend/resume path.

OK

> Here is an alternative implementation of the patch. My test machine is
> currently unavailable, so it is not yet been tested. How does this one look?

Well, I'd like to do that cleanly from the start.

Now, the problem is that PM_SLEEP_SMP selects HOTPLUG_CPU, because
that's necessary for the other architectures to make SMP suspend work, but it's
not necessary on your architecture.  Moreover, you don't need to compile
enable_nonboot_cpus() at all.

I'm not sure how to untangle it at the moment, but I think it should be
untangled.

Preferably, on architectures that need HOTPLUG_CPU for the SMP resume to work
PM_SLEEP_SMP should depend on it instead of selecting it, but on your
architectures they may be independent from each other.

Rafael

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Brian King]

On 02/22/2010 01:14 PM, Rafael J. Wysocki wrote:
> On Sunday 21 February 2010, Brian King wrote:
>> On 02/21/2010 04:37 PM, Rafael J. Wysocki wrote:
>>> On Sunday 21 February 2010, Brian King wrote:
>>>> On 02/21/2010 04:27 PM, Rafael J. Wysocki wrote:
>>>>> On Sunday 21 February 2010, Brian King wrote:
>>>>>> On 02/21/2010 04:08 PM, Rafael J. Wysocki wrote:
>>>>>>> I'm not a big fan of __attribute__ ((weak)), though.  While we already use that
>>>>>>> in the suspend code, I'm not particularly comfortable with it.
>>>>>>>
>>>>>>> Have you considered any alternative approaches?
>>>>>>
>>>>>> I suppose another option would be to implement this similar to how
>>>>>> arch_free_page and arch_alloc_page do. Something like this:
>>>>>>
>>>>>> #ifndef CONFIG_HAVE_ARCH_SUSPEND_CPUS
>>>>>> static inline int arch_suspend_disable_nonboot_cpus(void)
>>>>>> {
>>>>>> 	return disable_nonboot_cpus();
>>>>>> }
>>>>>>
>>>>>> static inline void arch_suspend_enable_nonboot_cpus(void)
>>>>>> {
>>>>>> 	return enable_nonboot_cpus()'
>>>>>> }
>>>>>> #else
>>>>>> extern int arch_suspend_disable_nonboot_cpus(void);
>>>>>> extern void arch_suspend_enable_nonboot_cpus(void);
>>>>>> #endif
>>>>>>
>>>>>> I figured I would just be consistent with arch_suspend_disable_irqs /
>>>>>> arch_suspend_enable_irqs.
>>>>>
>>>>> I just think that doing arch_suspend_[enable|disable]_irqs() this way was
>>>>> a mistake.
>>>>
>>>> Do you prefer the example above? I can send an updated patch. If not,
>>>> any other suggestions you might have as to the way you would like this
>>>> done would be greatly appreciated.
>>>
>>> disable_nonboot_cpus() is also called by kernel_power_off().  Is that fine
>>> with your architecture?
>>
>> Yes. We only need a different behavior for the suspend/resume path.
> 
> OK
> 
>> Here is an alternative implementation of the patch. My test machine is
>> currently unavailable, so it is not yet been tested. How does this one look?
> 
> Well, I'd like to do that cleanly from the start.
> 
> Now, the problem is that PM_SLEEP_SMP selects HOTPLUG_CPU, because
> that's necessary for the other architectures to make SMP suspend work, but it's
> not necessary on your architecture.  Moreover, you don't need to compile
> enable_nonboot_cpus() at all.

At least for the architecture I am enabling this support for (PPC_PSERIES), upon looking closer, it looks like PM_SLEEP_SMP was never defined, so enable_nonboot_cpus and disable_nonboot_cpus were always nooped before, which I didn't previously realize. We probably want to retain this behavior.

So perhaps a better way to solve this might be to change the Kconfig rules so that PM_SLEEP_SMP is not defined for PPC_PSERIES and then use ->prepare_late to put the other CPUs in H_JOIN state and ->wake to send H_PROD to them to wake them up. In that case, I suppose a simple patch like the one below would suffice.


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 kernel/power/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff -puN kernel/power/Kconfig~suspend_no_pm_sleep_smp kernel/power/Kconfig
--- linux-2.6/kernel/power/Kconfig~suspend_no_pm_sleep_smp	2010-02-22 17:05:25.000000000 -0600
+++ linux-2.6-bjking1/kernel/power/Kconfig	2010-02-22 17:07:00.000000000 -0600
@@ -77,6 +77,7 @@ config PM_SLEEP_SMP
 	depends on SMP
 	depends on ARCH_SUSPEND_POSSIBLE || ARCH_HIBERNATION_POSSIBLE
 	depends on PM_SLEEP
+	depends on !PPC_PSERIES
 	select HOTPLUG_CPU
 	default y
 
_

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Pavel Machek]

Hi!

> > OK
> > 
> >> Here is an alternative implementation of the patch. My test machine is
> >> currently unavailable, so it is not yet been tested. How does this one look?
> > 
> > Well, I'd like to do that cleanly from the start.
> > 
> > Now, the problem is that PM_SLEEP_SMP selects HOTPLUG_CPU, because
> > that's necessary for the other architectures to make SMP suspend work, but it's
> > not necessary on your architecture.  Moreover, you don't need to compile
> > enable_nonboot_cpus() at all.

>  At least for the architecture I am enabling this support for
> (PPC_PSERIES), upon looking closer, it looks like PM_SLEEP_SMP was
> never defined, so enable_nonboot_cpus and disable_nonboot_cpus were
> always nooped before, which I didn't previously realize. We probably
> want to retain this behavior.  >

(Please wrap at column 80)

This patch is already way better than the original one, but... Why do
you want enable/disable_nonboot_cpus to be noped out?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Brian King]

On 02/23/2010 09:43 AM, Pavel Machek wrote:
> Hi!
> 
>>> OK
>>>
>>>> Here is an alternative implementation of the patch. My test machine is
>>>> currently unavailable, so it is not yet been tested. How does this one look?
>>>
>>> Well, I'd like to do that cleanly from the start.
>>>
>>> Now, the problem is that PM_SLEEP_SMP selects HOTPLUG_CPU, because
>>> that's necessary for the other architectures to make SMP suspend work, but it's
>>> not necessary on your architecture.  Moreover, you don't need to compile
>>> enable_nonboot_cpus() at all.
> 
>>  At least for the architecture I am enabling this support for
>> (PPC_PSERIES), upon looking closer, it looks like PM_SLEEP_SMP was
>> never defined, so enable_nonboot_cpus and disable_nonboot_cpus were
>> always nooped before, which I didn't previously realize. We probably
>> want to retain this behavior.  >
> 
> (Please wrap at column 80)
> 
> This patch is already way better than the original one, but... Why do
> you want enable/disable_nonboot_cpus to be noped out?
> 									Pavel

Today for PPC_PSERIES, PM_SLEEP_SMP is never defined, so for all the
current code paths that call enable/disable_nonboot_cpus (power off,
kexec), these functions are noops. I don't want to change that behavior.
I figured I can just use the prepare_late and wake pm functions to do
the work I need to do. Let me know if you think this is a reasonable
approach and I'll be happy to resend the patch with an appropriate
subject line and description.

Thanks,

Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 1/1] PM: Add arch_suspend_disable_nonboot_cpus

[Pavel Machek]

> > (Please wrap at column 80)
> > 
> > This patch is already way better than the original one, but... Why do
> > you want enable/disable_nonboot_cpus to be noped out?
> 
> Today for PPC_PSERIES, PM_SLEEP_SMP is never defined, so for all the
> current code paths that call enable/disable_nonboot_cpus (power off,
> kexec), these functions are noops. I don't want to change that
> behavior.

Why not?

> I figured I can just use the prepare_late and wake pm functions to do
> the work I need to do. Let me know if you think this is a reasonable
> approach and I'll be happy to resend the patch with an appropriate
> subject line and description.

No, I do not think that's reasonable approach. Please just implement
enable/disable_nonboot_cpus; that's the clean fix.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html