Disable CPU hotplug during suspend
Subsystems (3)
PATCH RESEND 1/1 Disable CPU hotplug during suspend
The current suspend code has to be run on one CPU, so we use the CPU hotplug to take the non-boot CPUs offline on SMP machines. However, we should also make sure that these CPUs will not be enabled by someone else after we have disabled them.
The functions disable_nonboot_cpus() and enable_nonboot_cpus() are moved to kernel/cpu.c, because they now refer to some stuff in there that should better be static. Also it's better if disable_nonboot_cpus() returns an error instead of panicking if something goes wrong, and enable_nonboot_cpus() has no reason to panic(), because the CPUs may have been enabled by the userland before it tries to take them online.
--- include/linux/cpu.h | 8 +++ include/linux/suspend.h | 8 --- kernel/cpu.c | 104 ++++++++++++++++++++++++++++++++++++++++++------ kernel/power/Makefile | 2 kernel/power/disk.c | 7 ++- kernel/power/main.c | 10 +--- kernel/power/smp.c | 62 ---------------------------- kernel/power/user.c | 14 ++++-- 8 files changed, 118 insertions(+), 97 deletions(-)
include/linux/suspend.h +0 -8
@@ -57,14 +57,6 @@ static inline int software_suspend(void) }#endif /* CONFIG_PM */ -#ifdef CONFIG_SUSPEND_SMP -extern void disable_nonboot_cpus (void ); -extern void enable_nonboot_cpus (void ); -#else -static inline void disable_nonboot_cpus (void ) {} -static inline void enable_nonboot_cpus (void ) {} -#endif -void save_processor_state (void );void restore_processor_state (void );struct saved_context ;
kernel/power/smp.c deleted +0 -62
62 lines not on this page · raw message
kernel/power/disk.c +6 -1
@@ -18,6 +18,7 @@#include <linux/fs.h> #include <linux/mount.h> #include <linux/pm.h> +#include <linux/cpu.h> #include "power.h"
@@ -72,7 +73,10 @@ static int prepare_processes(void)int error;pm_prepare_console (); -disable_nonboot_cpus (); + + error =disable_nonboot_cpus (); +if (error) + goto enable_cpus;if (freeze_processes ()) { error = -EBUSY ;
@@ -84,6 +88,7 @@ static int prepare_processes(void)return 0 ; thaw:thaw_processes (); +enable_cpus:enable_nonboot_cpus ();pm_restore_console ();return error;
kernel/power/main.c +4 -6
@@ -16,6 +16,7 @@#include <linux/init.h> #include <linux/pm.h> #include <linux/console.h> +#include <linux/cpu.h> #include "power.h"
@@ -51,7 +52,7 @@ void pm_set_ops(struct pm_ops * ops)static int suspend_prepare (suspend_state_t state) { -int error =0 ; +int error;unsigned int free_pages;if (!pm_ops || !pm_ops->enter)
@@ -63,12 +64,9 @@ static int suspend_prepare(suspend_statepm_prepare_console (); -disable_nonboot_cpus (); - -if (num_online_cpus () !=1 ) { - error = -EPERM ; + error =disable_nonboot_cpus (); +if (error) goto Enable_cpu; - }if (freeze_processes ()) { error = -EAGAIN ;
kernel/power/user.c +9 -5
@@ -19,6 +19,7 @@#include <linux/swapops.h> #include <linux/pm.h> #include <linux/fs.h> +#include <linux/cpu.h> #include <asm/uaccess.h>
@@ -139,12 +140,15 @@ static int snapshot_ioctl(struct inode *if (data->frozen)break ;down (&pm_sem); -disable_nonboot_cpus (); -if (freeze_processes ()) { -thaw_processes (); -enable_nonboot_cpus (); - error = -EBUSY ; + error =disable_nonboot_cpus (); +if (!error) { + error =freeze_processes (); +if (error) { +thaw_processes (); + error = -EBUSY ; + } } +enable_nonboot_cpus ();up (&pm_sem);if (!error) data->frozen =1 ;
include/linux/cpu.h +8 -0
@@ -89,4 +89,12 @@ int cpu_down(unsigned int cpu);static inline int cpu_is_offline (int cpu) {return 0 ; }#endif +#ifdef CONFIG_SUSPEND_SMP +extern int disable_nonboot_cpus (void ); +extern void enable_nonboot_cpus (void ); +#else +static inline int disable_nonboot_cpus (void ) {return 0 ; } +static inline void enable_nonboot_cpus (void ) {} +#endif +#endif /* _LINUX_CPU_H_ */
kernel/cpu.c +91 -13
143 lines not on this page · raw message
kernel/power/Makefile +0 -2
@@ -7,6 +7,4 @@ obj-y := main.o process.o console.o obj-$(CONFIG_PM_LEGACY) += pm.o obj-$(CONFIG_SOFTWARE_SUSPEND) += swsusp.o disk.o snapshot.o swap.o user.o -obj-$(CONFIG_SUSPEND_SMP) += smp.o - obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o
Hi Rafael-
A couple of minor comments:
Quoted 14 lines
kernel/cpu.c +10
-0+int cpu_down (unsigned int cpu) +{ +int err =0 ; + +mutex_lock (&cpu_add_remove_lock); +if (cpu_hotplug_disabled) + err = -EPERM ; +else + err =__cpu_down (cpu); +mutex_unlock (&cpu_add_remove_lock);return err; }
kernel/cpu.c int __devinit cpu_up(unsigned int cpu) · line 209
Quoted 7 lines
kernel/cpu.c +4
-0@@ -191,6 +203,11 @@ int __devinit cpu_up(unsigned int cpu)void *hcpu = (void *)(long )cpu;mutex_lock (&cpu_add_remove_lock); +if (cpu_hotplug_disabled) { + ret = -EPERM ; + goto out; + }
I think -EBUSY would be more appropriate than -EPERM, perhaps?
Quoted 6 lines
kernel/cpu.c +6
-0+#ifdef CONFIG_SUSPEND_SMP +static cpumask_t frozen_cpus; + +int disable_nonboot_cpus (void ) +{ +int cpu, error =0 ;
kernel/cpu.c out: · line 259
Quoted 10 lines
kernel/cpu.c +10
-0+ +/* We take all of the non-boot CPUs down in one shot to avoid races +* with the userspace trying to use the CPU hotplug at the same time +*/ +mutex_lock (&cpu_add_remove_lock); +cpus_clear (frozen_cpus); +printk ("Disabling non-boot CPUs ...\n" ); +for_each_online_cpu (cpu) { +if (cpu ==0 ) +continue ;
Assuming cpu 0 is online is not okay in generic code. This should be something like:
int cpu, first_cpu, error = 0;
/* We take all of the non-boot CPUs down in one shot to avoid races
* with the userspace trying to use the CPU hotplug at the same time
*/
mutex_lock(&cpu_add_remove_lock);
cpus_clear(frozen_cpus);
first_cpu = first_cpu(cpu_online_mask);
printk("Disabling non-boot CPUs ...\n");
for_each_online_cpu(cpu) {
if (cpu == first_cpu)
continue;kernel/cpu.c out: · line 272
Quoted 13 lines
kernel/cpu.c +13
-0+ error =__cpu_down (cpu); +if (!error) { +cpu_set (cpu, frozen_cpus); +printk ("CPU%d is down\n" , cpu); + }else { +printk (KERN_ERR "Error taking CPU%d down: %d\n" , + cpu, error); +break ; + } + } +if (!error) { +BUG_ON (num_online_cpus () >1 ); +BUG_ON (raw_smp_processor_id () !=0 );
Same problem here.
Otherwise, I think the patch looks okay.
Hi Nathan,
Quoted 21 lines · Nathan Lynch
Hi Rafael-
A couple of minor comments:
Quoted 16 lines
kernel/cpu.c +10
-0+int cpu_down (unsigned int cpu) +{ +int err =0 ; + +mutex_lock (&cpu_add_remove_lock); +if (cpu_hotplug_disabled) + err = -EPERM ; +else + err =__cpu_down (cpu); +mutex_unlock (&cpu_add_remove_lock);return err; }@@ -191,6 +203,11 @@ int __devinit cpu_up(unsigned int cpu)void *hcpu = (void *)(long )cpu;
Quoted 7 lines
kernel/cpu.c int __devinit cpu_up(unsigned int cpu) · line 209
Quoted 5 lines
kernel/cpu.c +4
-0mutex_lock (&cpu_add_remove_lock); +if (cpu_hotplug_disabled) { + ret = -EPERM ; + goto out; + }I think
-EBUSYwould be more appropriate than-EPERM, perhaps?
Sure, why not.
Quoted 10 lines
Quoted 10 lines
kernel/cpu.c +10
-0+#ifdef CONFIG_SUSPEND_SMP +static cpumask_t frozen_cpus; + +int disable_nonboot_cpus (void ) +{ +int cpu, error =0 ; + +/* We take all of the non-boot CPUs down in one shot to avoid races +* with the userspace trying to use the CPU hotplug at the same time +*/
Quoted 8 lines
kernel/cpu.c out: · line 259
Quoted 6 lines
kernel/cpu.c +6
-0+mutex_lock (&cpu_add_remove_lock); +cpus_clear (frozen_cpus); +printk ("Disabling non-boot CPUs ...\n" ); +for_each_online_cpu (cpu) { +if (cpu ==0 ) +continue ;Assuming cpu 0 is online is not okay in generic code.
Absolutely. Thanks for pointing this out.
Quoted 14 lines
This should be something like:
int cpu, first_cpu, error = 0; /* We take all of the non-boot CPUs down in one shot to avoid races * with the userspace trying to use the CPU hotplug at the same time */ mutex_lock(&cpu_add_remove_lock); cpus_clear(frozen_cpus); first_cpu = first_cpu(cpu_online_mask); printk("Disabling non-boot CPUs ...\n"); for_each_online_cpu(cpu) { if (cpu == first_cpu) continue;
I'm not quite sure if we can finish with CPU0 offline. Perhaps it's better to check if CPU0 is online and bring it up if not and then continue or return an error if that fails?
Rafael
Quoted 5 lines · Rafael J. Wysocki
Hi Nathan,
Quoted 2 lines · Nathan Lynch
Hi Rafael-
]--snip--[
Quoted 3 lines
I'm not quite sure if we can finish with CPU0 offline. Perhaps it's better to check if CPU0 is online and bring it up if not and then continue or return an error if that fails?
I've tried to implement this idea in the appended patch (I'm omitting the changes outside of kernel/cpu.c as they are not relevant here).
Please have a look.
Rafael
kernel/cpu.c +117 -20
@@ -21,6 +21,11 @@ static DEFINE_MUTEX(cpu_bitmask_lock);static __cpuinitdata BLOCKING_NOTIFIER_HEAD (cpu_chain); +/* If set, cpu_up and cpu_down will return -EBUSY and do nothing. +* Should always be manipulated under cpu_add_remove_lock +*/ +static int cpu_hotplug_disabled; +#ifdef CONFIG_HOTPLUG_CPU /* Crappy recursive lock-takers in cpufreq! Complain loudly about idiots */
@@ -108,30 +113,25 @@ static int take_cpu_down(void *unused)return 0 ; } -int cpu_down (unsigned int cpu) +/* Requires cpu_add_remove_lock to be held */ +static int _cpu_down (unsigned int cpu) {int err;struct task_struct *p;cpumask_t old_allowed, tmp; -mutex_lock (&cpu_add_remove_lock); -if (num_online_cpus () ==1 ) { - err = -EBUSY ; - goto out; - } +if (num_online_cpus () ==1 ) +return -EBUSY ; -if (!cpu_online (cpu)) { - err = -EINVAL ; - goto out; - } +if (!cpu_online (cpu)) +return -EINVAL ; err =blocking_notifier_call_chain (&cpu_chain,CPU_DOWN_PREPARE , (void *)(long )cpu);if (err ==NOTIFY_BAD ) {printk ("%s: attempt to take down CPU %u failed\n" , __FUNCTION__, cpu); - err = -EINVAL ; - goto out; +return -EINVAL ; }/* Ensure that we are not runnable on dying cpu */
@@ -179,22 +179,31 @@ out_thread: err =kthread_stop (p); out_allowed:set_cpus_allowed (current, old_allowed); -out: + return err; +} + +int cpu_down (unsigned int cpu) +{ +int err =0 ; + +mutex_lock (&cpu_add_remove_lock); +if (cpu_hotplug_disabled) + err = -EBUSY ; +else + err =_cpu_down (cpu); +mutex_unlock (&cpu_add_remove_lock);return err; }#endif /*CONFIG_HOTPLUG_CPU*/ -int __devinit cpu_up (unsigned int cpu) +static int __devinit _cpu_up (unsigned int cpu) {int ret;void *hcpu = (void *)(long )cpu; -mutex_lock (&cpu_add_remove_lock); -if (cpu_online (cpu) || !cpu_present (cpu)) { - ret = -EINVAL ; - goto out; - } +if (cpu_online (cpu) || !cpu_present (cpu)) +return -EINVAL ; ret =blocking_notifier_call_chain (&cpu_chain,CPU_UP_PREPARE , hcpu);if (ret ==NOTIFY_BAD ) {
@@ -219,7 +228,95 @@ out_notify:if (ret !=0 )blocking_notifier_call_chain (&cpu_chain,CPU_UP_CANCELED , hcpu); + +return ret; +} + +int __devinit cpu_up (unsigned int cpu) +{ +int err =0 ; + +mutex_lock (&cpu_add_remove_lock); +if (cpu_hotplug_disabled) + err = -EBUSY ; +else + err =_cpu_up (cpu); + +mutex_unlock (&cpu_add_remove_lock); +return err; +} + +#ifdef CONFIG_SUSPEND_SMP +static cpumask_t frozen_cpus; + +int disable_nonboot_cpus (void ) +{ +int cpu, first_cpu, error; + +mutex_lock (&cpu_add_remove_lock); + first_cpu =first_cpu (cpu_present_map); +if (!cpu_online (first_cpu)) { + error =_cpu_up (first_cpu); +if (error) { +printk (KERN_ERR "Could not bring CPU%d up.\n" , + first_cpu); + goto out; + } + } + error =set_cpus_allowed (current,cpumask_of_cpu (first_cpu)); +if (error) { +printk (KERN_ERR "Could not run on CPU%d\n" , first_cpu); + goto out; + } +/* We take down all of the non-boot CPUs in one shot to avoid races +* with the userspace trying to use the CPU hotplug at the same time +*/ +cpus_clear (frozen_cpus); +printk ("Disabling non-boot CPUs ...\n" ); +for_each_online_cpu (cpu) { +if (cpu == first_cpu) +continue ; + error =_cpu_down (cpu); +if (!error) { +cpu_set (cpu, frozen_cpus); +printk ("CPU%d is down\n" , cpu); + }else { +printk (KERN_ERR "Error taking CPU%d down: %d\n" , + cpu, error); +break ; + } + } +if (!error) { +BUG_ON (num_online_cpus () >1 ); +/* Make sure the CPUs won't be enabled by someone else */ + cpu_hotplug_disabled =1 ; + }else { +printk (KERN_ERR "Non-boot CPUs are not disabled" ); + } out:mutex_unlock (&cpu_add_remove_lock); -return ret; +return error; +} + +void enable_nonboot_cpus (void ) +{ +int cpu, error; + +/* Allow everyone to use the CPU hotplug again */ +mutex_lock (&cpu_add_remove_lock); + cpu_hotplug_disabled =0 ; +mutex_unlock (&cpu_add_remove_lock); + +printk ("Enabling non-boot CPUs ...\n" ); +for_each_cpu_mask (cpu, frozen_cpus) { + error =cpu_up (cpu); +if (!error) { +printk ("CPU%d is up\n" , cpu); +continue ; + } +printk (KERN_WARNING "Error taking CPU%d up: %d\n" , + cpu, error); + } +cpus_clear (frozen_cpus); } +#endif
Quoted 38 lines · Rafael J. Wysocki
Quoted 10 lines · Nathan Lynch
Quoted 10 lines
kernel/cpu.c +10
-0+#ifdef CONFIG_SUSPEND_SMP +static cpumask_t frozen_cpus; + +int disable_nonboot_cpus (void ) +{ +int cpu, error =0 ; + +/* We take all of the non-boot CPUs down in one shot to avoid races +* with the userspace trying to use the CPU hotplug at the same time +*/ Quoted 8 lines
kernel/cpu.c out: · line 259
Quoted 6 lines
kernel/cpu.c +6
-0+mutex_lock (&cpu_add_remove_lock); +cpus_clear (frozen_cpus); +printk ("Disabling non-boot CPUs ...\n" ); +for_each_online_cpu (cpu) { +if (cpu ==0 ) +continue ;Assuming cpu 0 is online is not okay in generic code.
Absolutely. Thanks for pointing this out.
Quoted 14 lines
This should be something like:
int cpu, first_cpu, error = 0; /* We take all of the non-boot CPUs down in one shot to avoid races * with the userspace trying to use the CPU hotplug at the same time */ mutex_lock(&cpu_add_remove_lock); cpus_clear(frozen_cpus); first_cpu = first_cpu(cpu_online_mask); printk("Disabling non-boot CPUs ...\n"); for_each_online_cpu(cpu) { if (cpu == first_cpu) continue;
Quoted 3 lines
I'm not quite sure if we can finish with CPU0 offline. Perhaps it's better to check if CPU0 is online and bring it up if not and then continue or return an error if that fails?
You can't assume that cpu 0 is even present in generic code. :-)
But maybe I'm misunderstanding the motivation for using cpu 0 here. I had assumed it was because on i386 (and others?) the BSP can't be offlined. Is there some other reason?
Quoted 39 lines · Nathan Lynch
Quoted 38 lines · Rafael J. Wysocki
Quoted 10 lines · Nathan Lynch
Quoted 10 lines
kernel/cpu.c +10
-0+#ifdef CONFIG_SUSPEND_SMP +static cpumask_t frozen_cpus; + +int disable_nonboot_cpus (void ) +{ +int cpu, error =0 ; + +/* We take all of the non-boot CPUs down in one shot to avoid races +* with the userspace trying to use the CPU hotplug at the same time +*/ Quoted 8 lines
kernel/cpu.c out: · line 259
Quoted 6 lines
kernel/cpu.c +6
-0+mutex_lock (&cpu_add_remove_lock); +cpus_clear (frozen_cpus); +printk ("Disabling non-boot CPUs ...\n" ); +for_each_online_cpu (cpu) { +if (cpu ==0 ) +continue ;Assuming cpu 0 is online is not okay in generic code.
Absolutely. Thanks for pointing this out.
Quoted 14 lines
This should be something like:
int cpu, first_cpu, error = 0; /* We take all of the non-boot CPUs down in one shot to avoid races * with the userspace trying to use the CPU hotplug at the same time */ mutex_lock(&cpu_add_remove_lock); cpus_clear(frozen_cpus); first_cpu = first_cpu(cpu_online_mask); printk("Disabling non-boot CPUs ...\n"); for_each_online_cpu(cpu) { if (cpu == first_cpu) continue;
Quoted 5 lines
Quoted 3 lines
I'm not quite sure if we can finish with CPU0 offline. Perhaps it's better to check if CPU0 is online and bring it up if not and then continue or return an error if that fails?
You can't assume that cpu 0 is even present in generic code. :-)
Yes. I should have said "the first present CPU" instead of "CPU0".
Quoted 3 lines
But maybe I'm misunderstanding the motivation for using cpu 0 here. I had assumed it was because on i386 (and others?) the BSP can't be offlined. Is there some other reason?
Yes.
First, the arch-dependent suspend code assumes implicitly that it will be running on the BSP, so some strange things may happen if it doesn't.
Second, we have to make sure that this function will always leaves the same CPU online. It's a bit difficult to explain, but I'll do my best. Suppose that disable_nonboot_cpus() exits running on CPU1, assuming it's possible. Then the system memory state saved in the suspend image will reflect this situation. Now the resume code will almost certainly run on the BSP (say it's CPU0), but when the system memory is restored from the suspend image the kernel will think it's running on CPU1.
In the last patch I send yesterday I made disable_nonboot_cpus() check if the first present CPU, first_cpu(cpu_present_map), is online, try to bring it up if not and migrate itself to it before the loop over all online CPUs is run.
I think that's general enough.
Quoted 19 lines · Rafael J. Wysocki
Quoted 3 lines · Nathan Lynch
But maybe I'm misunderstanding the motivation for using cpu 0 here. I had assumed it was because on i386 (and others?) the BSP can't be offlined. Is there some other reason?
Yes.
First, the arch-dependent suspend code assumes implicitly that it will be running on the BSP, so some strange things may happen if it doesn't.
Second, we have to make sure that this function will always leaves the same CPU online. It's a bit difficult to explain, but I'll do my best. Suppose that
disable_nonboot_cpus()exits running on CPU1, assuming it's possible. Then the system memory state saved in the suspend image will reflect this situation. Now the resume code will almost certainly run on the BSP (say it's CPU0), but when the system memory is restored from the suspend image the kernel will think it's running on CPU1.
Quoted 5 lines
In the last patch I send yesterday I made
disable_nonboot_cpus()check if the first present CPU,first_cpu(cpu_present_map), is online, try to bring it up if not and migrate itself to it before the loop over all online CPUs is run.I think that's general enough.
I see, thanks for the explanation.
It doesn't look like SMP swsusp would work reliably on platforms where there's a possibility of the cpu maps in the resume and saved images not matching (e.g. ppc64 logical partitions, where cpu 0 could be removed before suspending). But I guess that's largely a theoretical concern at this time. ;)
Rafael J. Wysocki
Acked-by Pavel MachekQuoted 26 lines · Nathan Lynch
Quoted 19 lines · Rafael J. Wysocki
Quoted 3 lines · Nathan Lynch
But maybe I'm misunderstanding the motivation for using cpu 0 here. I had assumed it was because on i386 (and others?) the BSP can't be offlined. Is there some other reason?
Yes.
First, the arch-dependent suspend code assumes implicitly that it will be running on the BSP, so some strange things may happen if it doesn't.
Second, we have to make sure that this function will always leaves the same CPU online. It's a bit difficult to explain, but I'll do my best. Suppose that
disable_nonboot_cpus()exits running on CPU1, assuming it's possible. Then the system memory state saved in the suspend image will reflect this situation. Now the resume code will almost certainly run on the BSP (say it's CPU0), but when the system memory is restored from the suspend image the kernel will think it's running on CPU1.Quoted 5 lines
In the last patch I send yesterday I made
disable_nonboot_cpus()check if the first present CPU,first_cpu(cpu_present_map), is online, try to bring it up if not and migrate itself to it before the loop over all online CPUs is run.I think that's general enough.
Quoted 7 lines
I see, thanks for the explanation.
It doesn't look like SMP swsusp would work reliably on platforms where there's a possibility of the cpu maps in the resume and saved images not matching (e.g. ppc64 logical partitions, where cpu 0 could be removed before suspending). But I guess that's largely a theoretical concern at this time. ;)
I think so. :-)
Appended is the full patch with corrections resulting from our discussion here.
Andrew, could you please replace disable-cpu-hotplug-during-suspend.patch with this one?
---
The current suspend code has to be run on one CPU, so we use the CPU hotplug to take the non-boot CPUs offline on SMP machines. However, we should also make sure that these CPUs will not be enabled by someone else after we have disabled them.
The functions disable_nonboot_cpus() and enable_nonboot_cpus() are moved to kernel/cpu.c, because they now refer to some stuff in there that should better be static. Also it's better if disable_nonboot_cpus() returns an error instead of panicking if something goes wrong, and enable_nonboot_cpus() has no reason to panic(), because the CPUs may have been enabled by the userland before it tries to take them online.
--- include/linux/cpu.h | 8 ++ include/linux/suspend.h | 8 -- kernel/cpu.c | 138 +++++++++++++++++++++++++++++++++++++++++------- kernel/power/Makefile | 2 kernel/power/disk.c | 7 ++ kernel/power/main.c | 10 +-- kernel/power/smp.c | 62 --------------------- kernel/power/user.c | 14 +++- 8 files changed, 145 insertions(+), 104 deletions(-)
include/linux/suspend.h +0 -8
@@ -57,14 +57,6 @@ static inline int software_suspend(void) }#endif /* CONFIG_PM */ -#ifdef CONFIG_SUSPEND_SMP -extern void disable_nonboot_cpus (void ); -extern void enable_nonboot_cpus (void ); -#else -static inline void disable_nonboot_cpus (void ) {} -static inline void enable_nonboot_cpus (void ) {} -#endif -void save_processor_state (void );void restore_processor_state (void );struct saved_context ;
kernel/power/smp.c deleted +0 -62
62 lines not on this page · raw message
kernel/power/disk.c +6 -1
@@ -18,6 +18,7 @@#include <linux/fs.h> #include <linux/mount.h> #include <linux/pm.h> +#include <linux/cpu.h> #include "power.h"
@@ -72,7 +73,10 @@ static int prepare_processes(void)int error;pm_prepare_console (); -disable_nonboot_cpus (); + + error =disable_nonboot_cpus (); +if (error) + goto enable_cpus;if (freeze_processes ()) { error = -EBUSY ;
@@ -84,6 +88,7 @@ static int prepare_processes(void)return 0 ; thaw:thaw_processes (); +enable_cpus:enable_nonboot_cpus ();pm_restore_console ();return error;
kernel/power/main.c +4 -6
@@ -16,6 +16,7 @@#include <linux/init.h> #include <linux/pm.h> #include <linux/console.h> +#include <linux/cpu.h> #include "power.h"
@@ -51,7 +52,7 @@ void pm_set_ops(struct pm_ops * ops)static int suspend_prepare (suspend_state_t state) { -int error =0 ; +int error;unsigned int free_pages;if (!pm_ops || !pm_ops->enter)
@@ -63,12 +64,9 @@ static int suspend_prepare(suspend_statepm_prepare_console (); -disable_nonboot_cpus (); - -if (num_online_cpus () !=1 ) { - error = -EPERM ; + error =disable_nonboot_cpus (); +if (error) goto Enable_cpu; - }if (freeze_processes ()) { error = -EAGAIN ;
kernel/power/user.c +9 -5
@@ -19,6 +19,7 @@#include <linux/swapops.h> #include <linux/pm.h> #include <linux/fs.h> +#include <linux/cpu.h> #include <asm/uaccess.h>
@@ -139,12 +140,15 @@ static int snapshot_ioctl(struct inode *if (data->frozen)break ;down (&pm_sem); -disable_nonboot_cpus (); -if (freeze_processes ()) { -thaw_processes (); -enable_nonboot_cpus (); - error = -EBUSY ; + error =disable_nonboot_cpus (); +if (!error) { + error =freeze_processes (); +if (error) { +thaw_processes (); + error = -EBUSY ; + } } +enable_nonboot_cpus ();up (&pm_sem);if (!error) data->frozen =1 ;
include/linux/cpu.h +8 -0
@@ -89,4 +89,12 @@ int cpu_down(unsigned int cpu);static inline int cpu_is_offline (int cpu) {return 0 ; }#endif +#ifdef CONFIG_SUSPEND_SMP +extern int disable_nonboot_cpus (void ); +extern void enable_nonboot_cpus (void ); +#else +static inline int disable_nonboot_cpus (void ) {return 0 ; } +static inline void enable_nonboot_cpus (void ) {} +#endif +#endif /* _LINUX_CPU_H_ */
kernel/cpu.c +118 -20
183 lines not on this page · raw message
kernel/power/Makefile +0 -2
@@ -7,6 +7,4 @@ obj-y := main.o process.o console.o obj-$(CONFIG_PM_LEGACY) += pm.o obj-$(CONFIG_SOFTWARE_SUSPEND) += swsusp.o disk.o snapshot.o swap.o user.o -obj-$(CONFIG_SUSPEND_SMP) += smp.o - obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o
kernel/cpu.c out: · line 259
Quoted 13 lines · On Fri, 28 Jul 2006 10:15:29 +0200 "Rafael J. Wysocki" <[email protected]> wrote:
int
disable_nonboot_cpus(void)kernel/cpu.c +12
-0+{ +int cpu, error =0 ; + +/* We take all of the non-boot CPUs down in one shot to avoid races +* with the userspace trying to use the CPU hotplug at the same time +*/ +mutex_lock (&cpu_add_remove_lock); +cpus_clear (frozen_cpus); +printk ("Disabling non-boot CPUs ...\n" ); +for_each_online_cpu (cpu) { +if (cpu ==0 ) +continue ;
This is presumably only called on cpu 0, yes?
How can we guarantee that, given that preemption is enabled?
What happens if cpu 0 isn't online?
Quoted 19 lines · Andrew Morton
kernel/cpu.c out: · line 259
Quoted 13 lines · On Fri, 28 Jul 2006 10:15:29 +0200 "Rafael J. Wysocki" <[email protected]> wrote:
int
disable_nonboot_cpus(void)kernel/cpu.c +12
-0+{ +int cpu, error =0 ; + +/* We take all of the non-boot CPUs down in one shot to avoid races +* with the userspace trying to use the CPU hotplug at the same time +*/ +mutex_lock (&cpu_add_remove_lock); +cpus_clear (frozen_cpus); +printk ("Disabling non-boot CPUs ...\n" ); +for_each_online_cpu (cpu) { +if (cpu ==0 ) +continue ;This is presumably only called on cpu 0, yes?
Quoted 1 line
How can we guarantee that, given that preemption is enabled?
If cpu 0 is online, it will end up running on it when all of the other cpus are down.
Quoted 1 line
What happens if cpu 0 isn't online?
Fortunately, on x86_64 and i386 it cannot be offline (I'm not sure about ppc, though), but of course in general we shouldn't assume that it's online or even present here.
I'm discussing the issue with Nathan right now.