2005-03-23 20:45:59

by Pavel Machek

[permalink] [raw]
Subject: smp/swsusp done right

Hi!

This is against -mm kernel; it is smp swsusp done right, and it
actually works for me. Unlike previous hacks, it uses cpu hotplug
infrastructure. Disable CONFIG_MTRR before you try this...

Test this if you can, and report any problems. If not enough people
scream, this is going to -mm.
Pavel

--- clean-mm/drivers/pci/pci.c 2005-03-21 11:39:32.000000000 +0100
+++ linux-mm/drivers/pci/pci.c 2005-03-22 01:41:48.000000000 +0100
@@ -376,11 +376,13 @@
if (!pci_find_capability(dev, PCI_CAP_ID_PM))
return PCI_D0;

+#if 0
if (platform_pci_choose_state) {
ret = platform_pci_choose_state(dev, state);
if (ret >= 0)
state = ret;
}
+#endif
switch (state) {
case 0: return PCI_D0;
case 3: return PCI_D3hot;
--- clean-mm/kernel/power/Kconfig 2005-01-22 21:24:53.000000000 +0100
+++ linux-mm/kernel/power/Kconfig 2005-03-23 11:40:14.000000000 +0100
@@ -28,7 +28,7 @@

config SOFTWARE_SUSPEND
bool "Software Suspend (EXPERIMENTAL)"
- depends on EXPERIMENTAL && PM && SWAP
+ depends on EXPERIMENTAL && PM && SWAP && (HOTPLUG_CPU || !SMP)
---help---
Enable the possibility of suspending the machine.
It doesn't need APM.
--- clean-mm/kernel/power/smp.c 2005-03-19 00:32:32.000000000 +0100
+++ linux-mm/kernel/power/smp.c 2005-03-23 15:38:30.000000000 +0100
@@ -7,79 +7,53 @@
* This file is released under the GPLv2.
*/

-#undef DEBUG
-
#include <linux/smp_lock.h>
#include <linux/interrupt.h>
#include <linux/suspend.h>
#include <linux/module.h>
#include <asm/atomic.h>
#include <asm/tlbflush.h>
+#include <asm/cpu.h>

-static atomic_t cpu_counter, freeze;
-
-
-static void smp_pause(void * data)
-{
- struct saved_context ctxt;
- __save_processor_state(&ctxt);
- printk("Sleeping in:\n");
- dump_stack();
- atomic_inc(&cpu_counter);
- while (atomic_read(&freeze)) {
- /* FIXME: restore takes place at random piece inside this.
- This should probably be written in assembly, and
- preserve general-purpose registers, too
-
- What about stack? We may need to move to new stack here.
-
- This should better be ran with interrupts disabled.
- */
- cpu_relax();
- barrier();
- }
- atomic_dec(&cpu_counter);
- __restore_processor_state(&ctxt);
-}
-
-static cpumask_t oldmask;
+cpumask_t frozen_cpus;

void disable_nonboot_cpus(void)
{
- printk("Freezing CPUs (at %d)", smp_processor_id());
- oldmask = current->cpus_allowed;
- set_cpus_allowed(current, cpumask_of_cpu(0));
- current->state = TASK_INTERRUPTIBLE;
- schedule_timeout(HZ);
- printk("...");
- BUG_ON(smp_processor_id() != 0);
-
- /* FIXME: for this to work, all the CPUs must be running
- * "idle" thread (or we deadlock). Is that guaranteed? */
+ int cpu, error;

- atomic_set(&cpu_counter, 0);
- atomic_set(&freeze, 1);
- smp_call_function(smp_pause, NULL, 0, 0);
- while (atomic_read(&cpu_counter) < (num_online_cpus() - 1)) {
- cpu_relax();
- barrier();
+ error = 0;
+ cpus_clear(frozen_cpus);
+ printk("Freezing cpus ...\n");
+ for_each_online_cpu(cpu) {
+ if (cpu == 0)
+ continue;
+ error = cpu_down(cpu);
+ if (!error) {
+ cpu_set(cpu, frozen_cpus);
+ printk("CPU%d is down\n", cpu);
+ continue;
+ }
+ printk("Error taking cpu %d down: %d\n", cpu, error);
}
- printk("ok\n");
+ BUG_ON(smp_processor_id() != 0);
+ if (error)
+ panic("cpus not sleeping");
}

void enable_nonboot_cpus(void)
{
- printk("Restarting CPUs");
- atomic_set(&freeze, 0);
- while (atomic_read(&cpu_counter)) {
- cpu_relax();
- barrier();
- }
- printk("...");
- set_cpus_allowed(current, oldmask);
- schedule();
- printk("ok\n");
+ int cpu, error;

+ printk("Thawing cpus ...\n");
+ for_each_cpu_mask(cpu, frozen_cpus) {
+ if (cpu == 0)
+ continue;
+ error = cpu_up(cpu);
+ if (!error) {
+ printk("CPU%d is up\n", cpu);
+ continue;
+ }
+ printk("Error taking cpu %d up: %d\n", cpu, error);
+ panic("Not enough cpus");
+ }
}
-
-
--- clean-mm/kernel/power/swsusp.c 2005-03-21 11:39:33.000000000 +0100
+++ linux-mm/kernel/power/swsusp.c 2005-03-23 15:34:53.000000000 +0100
@@ -1194,8 +1194,11 @@
return "version";
if (strcmp(swsusp_info.uts.machine,system_utsname.machine))
return "machine";
+#if 0
+ /* We can't use number of CPUs when we use hotplug to remove them ;-))) */
if(swsusp_info.cpus != num_online_cpus())
return "number of cpus";
+#endif
return NULL;
}


--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


2005-03-30 10:42:40

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: smp/swsusp done right

Hi,

On Wednesday, 23 of March 2005 21:40, Pavel Machek wrote:
> Hi!
>
> This is against -mm kernel; it is smp swsusp done right, and it
> actually works for me. Unlike previous hacks, it uses cpu hotplug
> infrastructure. Disable CONFIG_MTRR before you try this...
>
> Test this if you can, and report any problems. If not enough people
> scream, this is going to -mm.
> Pavel
>
> --- clean-mm/drivers/pci/pci.c 2005-03-21 11:39:32.000000000 +0100
> +++ linux-mm/drivers/pci/pci.c 2005-03-22 01:41:48.000000000 +0100
> @@ -376,11 +376,13 @@
> if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> return PCI_D0;
>
> +#if 0
> if (platform_pci_choose_state) {
> ret = platform_pci_choose_state(dev, state);
> if (ret >= 0)
> state = ret;
> }
> +#endif
> switch (state) {
> case 0: return PCI_D0;
> case 3: return PCI_D3hot;

You probably don't want the above change to go in the final patch?


> --- clean-mm/kernel/power/Kconfig 2005-01-22 21:24:53.000000000 +0100
> +++ linux-mm/kernel/power/Kconfig 2005-03-23 11:40:14.000000000 +0100
> @@ -28,7 +28,7 @@
>
> config SOFTWARE_SUSPEND
> bool "Software Suspend (EXPERIMENTAL)"
> - depends on EXPERIMENTAL && PM && SWAP
> + depends on EXPERIMENTAL && PM && SWAP && (HOTPLUG_CPU || !SMP)
> ---help---
> Enable the possibility of suspending the machine.
> It doesn't need APM.
> --- clean-mm/kernel/power/smp.c 2005-03-19 00:32:32.000000000 +0100
> +++ linux-mm/kernel/power/smp.c 2005-03-23 15:38:30.000000000 +0100
> @@ -7,79 +7,53 @@
> * This file is released under the GPLv2.
> */
>
> -#undef DEBUG
> -
> #include <linux/smp_lock.h>
> #include <linux/interrupt.h>
> #include <linux/suspend.h>
> #include <linux/module.h>
> #include <asm/atomic.h>
> #include <asm/tlbflush.h>
> +#include <asm/cpu.h>
>
> -static atomic_t cpu_counter, freeze;
> -
> -
> -static void smp_pause(void * data)
> -{
> - struct saved_context ctxt;
> - __save_processor_state(&ctxt);
> - printk("Sleeping in:\n");
> - dump_stack();
> - atomic_inc(&cpu_counter);
> - while (atomic_read(&freeze)) {
> - /* FIXME: restore takes place at random piece inside this.
> - This should probably be written in assembly, and
> - preserve general-purpose registers, too
> -
> - What about stack? We may need to move to new stack here.
> -
> - This should better be ran with interrupts disabled.
> - */
> - cpu_relax();
> - barrier();
> - }
> - atomic_dec(&cpu_counter);
> - __restore_processor_state(&ctxt);
> -}
> -
> -static cpumask_t oldmask;
> +cpumask_t frozen_cpus;
>
> void disable_nonboot_cpus(void)
> {
> - printk("Freezing CPUs (at %d)", smp_processor_id());
> - oldmask = current->cpus_allowed;
> - set_cpus_allowed(current, cpumask_of_cpu(0));
> - current->state = TASK_INTERRUPTIBLE;
> - schedule_timeout(HZ);
> - printk("...");
> - BUG_ON(smp_processor_id() != 0);
> -
> - /* FIXME: for this to work, all the CPUs must be running
> - * "idle" thread (or we deadlock). Is that guaranteed? */
> + int cpu, error;
>
> - atomic_set(&cpu_counter, 0);
> - atomic_set(&freeze, 1);
> - smp_call_function(smp_pause, NULL, 0, 0);
> - while (atomic_read(&cpu_counter) < (num_online_cpus() - 1)) {
> - cpu_relax();
> - barrier();
> + error = 0;
> + cpus_clear(frozen_cpus);
> + printk("Freezing cpus ...\n");
> + for_each_online_cpu(cpu) {
> + if (cpu == 0)
> + continue;
> + error = cpu_down(cpu);
> + if (!error) {
> + cpu_set(cpu, frozen_cpus);
> + printk("CPU%d is down\n", cpu);
> + continue;
> + }
> + printk("Error taking cpu %d down: %d\n", cpu, error);
> }
> - printk("ok\n");
> + BUG_ON(smp_processor_id() != 0);
> + if (error)
> + panic("cpus not sleeping");
> }

I'm not sure whether we should panic() here. It may be better to make
suspend fail and print a "please reboot immediately" message for the user.
In that case, the user may be able to reboot without loosing data ...


> void enable_nonboot_cpus(void)
> {
> - printk("Restarting CPUs");
> - atomic_set(&freeze, 0);
> - while (atomic_read(&cpu_counter)) {
> - cpu_relax();
> - barrier();
> - }
> - printk("...");
> - set_cpus_allowed(current, oldmask);
> - schedule();
> - printk("ok\n");
> + int cpu, error;
>
> + printk("Thawing cpus ...\n");
> + for_each_cpu_mask(cpu, frozen_cpus) {
> + if (cpu == 0)
> + continue;
> + error = cpu_up(cpu);
> + if (!error) {
> + printk("CPU%d is up\n", cpu);
> + continue;
> + }
> + printk("Error taking cpu %d up: %d\n", cpu, error);
> + panic("Not enough cpus");
> + }
> }
> -
> -
> --- clean-mm/kernel/power/swsusp.c 2005-03-21 11:39:33.000000000 +0100
> +++ linux-mm/kernel/power/swsusp.c 2005-03-23 15:34:53.000000000 +0100
> @@ -1194,8 +1194,11 @@
> return "version";
> if (strcmp(swsusp_info.uts.machine,system_utsname.machine))
> return "machine";
> +#if 0
> + /* We can't use number of CPUs when we use hotplug to remove them ;-))) */
> if(swsusp_info.cpus != num_online_cpus())
> return "number of cpus";
> +#endif
> return NULL;
> }
>
>
> --

I'll test it when I get the CPU hotplug on x86-64 done.

Greets,
Rafael


--
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
-- Lewis Carroll "Alice's Adventures in Wonderland"

2005-03-30 11:33:15

by Pavel Machek

[permalink] [raw]
Subject: Re: smp/swsusp done right

Hi!

> > This is against -mm kernel; it is smp swsusp done right, and it
> > actually works for me. Unlike previous hacks, it uses cpu hotplug
> > infrastructure. Disable CONFIG_MTRR before you try this...
> >
> > Test this if you can, and report any problems. If not enough people
> > scream, this is going to -mm.
> > Pavel
> >
> > --- clean-mm/drivers/pci/pci.c 2005-03-21 11:39:32.000000000 +0100
> > +++ linux-mm/drivers/pci/pci.c 2005-03-22 01:41:48.000000000 +0100
> > @@ -376,11 +376,13 @@
> > if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > return PCI_D0;
> >
> > +#if 0
> > if (platform_pci_choose_state) {
> > ret = platform_pci_choose_state(dev, state);
> > if (ret >= 0)
> > state = ret;
> > }
> > +#endif
> > switch (state) {
> > case 0: return PCI_D0;
> > case 3: return PCI_D3hot;
>
> You probably don't want the above change to go in the final patch?

No, not in final patch.

> > - atomic_set(&cpu_counter, 0);
> > - atomic_set(&freeze, 1);
> > - smp_call_function(smp_pause, NULL, 0, 0);
> > - while (atomic_read(&cpu_counter) < (num_online_cpus() - 1)) {
> > - cpu_relax();
> > - barrier();
> > + error = 0;
> > + cpus_clear(frozen_cpus);
> > + printk("Freezing cpus ...\n");
> > + for_each_online_cpu(cpu) {
> > + if (cpu == 0)
> > + continue;
> > + error = cpu_down(cpu);
> > + if (!error) {
> > + cpu_set(cpu, frozen_cpus);
> > + printk("CPU%d is down\n", cpu);
> > + continue;
> > + }
> > + printk("Error taking cpu %d down: %d\n", cpu, error);
> > }
> > - printk("ok\n");
> > + BUG_ON(smp_processor_id() != 0);
> > + if (error)
> > + panic("cpus not sleeping");
> > }
>
> I'm not sure whether we should panic() here. It may be better to make
> suspend fail and print a "please reboot immediately" message for the user.
> In that case, the user may be able to reboot without loosing data
> - ...

I guess I could just fail the suspend, but I want to see the messages
for now. (And I do not think it will ever trigger).
Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

2005-03-30 21:16:14

by Zwane Mwaikambo

[permalink] [raw]
Subject: Re: smp/swsusp done right

On Wed, 23 Mar 2005, Pavel Machek wrote:

> This is against -mm kernel; it is smp swsusp done right, and it
> actually works for me. Unlike previous hacks, it uses cpu hotplug
> infrastructure. Disable CONFIG_MTRR before you try this...
>
> Test this if you can, and report any problems. If not enough people
> scream, this is going to -mm.

Yay! Thanks for getting that done Pavel =)

Zwane

2005-03-30 21:25:05

by Pavel Machek

[permalink] [raw]
Subject: Re: smp/swsusp done right

Hi!

> > This is against -mm kernel; it is smp swsusp done right, and it
> > actually works for me. Unlike previous hacks, it uses cpu hotplug
> > infrastructure. Disable CONFIG_MTRR before you try this...
> >
> > Test this if you can, and report any problems. If not enough people
> > scream, this is going to -mm.
>
> Yay! Thanks for getting that done Pavel =)

Well, I guess it is thank you -- I got rid of ugly FIXME that would
involve arch-dependend assembly to be solved properly.

... hmm, can play_dead handle all the memory being overwritten? Also
it should probably save and restore registers including MTRRs.

...so I more moved ugly FIXME to better place. Oh well, at least it
uses common infrastructure now. People at Intel doing suspend-to-RAM
on smp systems will need to solve it properly, anyway, because they
need to go to real mode and back.

Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

2005-03-30 21:42:58

by Zwane Mwaikambo

[permalink] [raw]
Subject: Re: smp/swsusp done right

On Wed, 30 Mar 2005, Pavel Machek wrote:

> Hi!
>
> > > This is against -mm kernel; it is smp swsusp done right, and it
> > > actually works for me. Unlike previous hacks, it uses cpu hotplug
> > > infrastructure. Disable CONFIG_MTRR before you try this...
> > >
> > > Test this if you can, and report any problems. If not enough people
> > > scream, this is going to -mm.
> >
> > Yay! Thanks for getting that done Pavel =)
>
> Well, I guess it is thank you -- I got rid of ugly FIXME that would
> involve arch-dependend assembly to be solved properly.
>
> ... hmm, can play_dead handle all the memory being overwritten? Also
> it should probably save and restore registers including MTRRs.

play_dead currently can't handle memory being overwritten, i guess you'd
probably want to put the hook to kill the processor for real there. We
probably only have to make sure that MTRRs are restored on resume, which
should be taken care of by the current code since ACPI hotplug probably
requires it too.

> ...so I more moved ugly FIXME to better place. Oh well, at least it
> uses common infrastructure now. People at Intel doing suspend-to-RAM
> on smp systems will need to solve it properly, anyway, because they
> need to go to real mode and back.

Sounds like a perfect candidate =)

Thanks,
Zwane