* [PATCH -mm 0/2] PM: Hibernation and suspend notifiers @ 2007-06-04 21:25 Rafael J. Wysocki 2007-06-04 21:27 ` [PATCH -mm 1/2] PM: Introduce hibernation " Rafael J. Wysocki 2007-06-04 21:28 ` [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend Rafael J. Wysocki 0 siblings, 2 replies; 10+ messages in thread From: Rafael J. Wysocki @ 2007-06-04 21:25 UTC (permalink / raw) To: Andrew Morton; +Cc: LKML, Nigel Cunningham, Pavel Machek Hi, The first of the following two patches introduces hibernation and suspend notifiers that can be used by subsystems to perform tasks related to suspend and/or hibernation that cannot be done from device drivers' .suspend() and .resume() callbacks. The second one uses the notifiers to disable the user mode helper functionality before a hibernation/suspend. Greetings, Rafael -- "Premature optimization is the root of all evil." - Donald Knuth ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH -mm 1/2] PM: Introduce hibernation and suspend notifiers 2007-06-04 21:25 [PATCH -mm 0/2] PM: Hibernation and suspend notifiers Rafael J. Wysocki @ 2007-06-04 21:27 ` Rafael J. Wysocki 2007-06-04 21:28 ` [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend Rafael J. Wysocki 1 sibling, 0 replies; 10+ messages in thread From: Rafael J. Wysocki @ 2007-06-04 21:27 UTC (permalink / raw) To: Andrew Morton; +Cc: LKML, Nigel Cunningham, Pavel Machek From: Rafael J. Wysocki <rjw@sisk.pl> Make it possible to register hibernation and suspend notifiers, so that subsystems can perform hibernation-related or suspend-related operations that should not be carried out by device drivers' .suspend() and .resume() routines. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> Acked-by: Pavel Machek <pavel@ucw.cz> --- Documentation/power/notifiers.txt | 50 ++++++++++++++++++++++++++++++++++++++ include/linux/notifier.h | 6 ++++ include/linux/suspend.h | 37 +++++++++++++++++++++++++--- kernel/power/disk.c | 16 +++++++++--- kernel/power/main.c | 9 ++++++ kernel/power/power.h | 10 +++++++ kernel/power/user.c | 11 ++++++-- 7 files changed, 129 insertions(+), 10 deletions(-) Index: linux-2.6.22-rc3/include/linux/suspend.h =================================================================== --- linux-2.6.22-rc3.orig/include/linux/suspend.h 2007-05-31 00:00:38.000000000 +0200 +++ linux-2.6.22-rc3/include/linux/suspend.h 2007-06-01 22:55:01.000000000 +0200 @@ -54,7 +54,8 @@ struct hibernation_ops { void (*restore_cleanup)(void); }; -#if defined(CONFIG_PM) && defined(CONFIG_SOFTWARE_SUSPEND) +#ifdef CONFIG_PM +#ifdef CONFIG_SOFTWARE_SUSPEND /* kernel/power/snapshot.c */ extern void __register_nosave_region(unsigned long b, unsigned long e, int km); static inline void register_nosave_region(unsigned long b, unsigned long e) @@ -72,7 +73,7 @@ extern unsigned long get_safe_page(gfp_t extern void hibernation_set_ops(struct hibernation_ops *ops); extern int hibernate(void); -#else +#else /* CONFIG_SOFTWARE_SUSPEND */ static inline void register_nosave_region(unsigned long b, unsigned long e) {} static inline void register_nosave_region_late(unsigned long b, unsigned long e) {} static inline int swsusp_page_is_forbidden(struct page *p) { return 0; } @@ -81,7 +82,7 @@ static inline void swsusp_unset_page_fre static inline void hibernation_set_ops(struct hibernation_ops *ops) {} static inline int hibernate(void) { return -ENOSYS; } -#endif /* defined(CONFIG_PM) && defined(CONFIG_SOFTWARE_SUSPEND) */ +#endif /* CONFIG_SOFTWARE_SUSPEND */ void save_processor_state(void); void restore_processor_state(void); @@ -89,4 +90,34 @@ struct saved_context; void __save_processor_state(struct saved_context *ctxt); void __restore_processor_state(struct saved_context *ctxt); +/* kernel/power/main.c */ +extern struct blocking_notifier_head pm_chain_head; + +static inline int register_pm_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&pm_chain_head, nb); +} + +static inline int unregister_pm_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&pm_chain_head, nb); +} + +#define pm_notifier(fn, pri) { \ + static struct notifier_block fn##_nb = \ + { .notifier_call = fn, .priority = pri }; \ + register_pm_notifier(&fn##_nb); \ +} +#else /* CONFIG_PM */ +static inline int register_pm_notifier(struct notifier_block *nb) { + return 0; +} + +static inline int unregister_pm_notifier(struct notifier_block *nb) { + return 0; +} + +#define pm_notifier(fn, pri) do { (void)(fn); } while (0) +#endif /* CONFIG_PM */ + #endif /* _LINUX_SWSUSP_H */ Index: linux-2.6.22-rc3/kernel/power/power.h =================================================================== --- linux-2.6.22-rc3.orig/kernel/power/power.h 2007-05-31 00:00:38.000000000 +0200 +++ linux-2.6.22-rc3/kernel/power/power.h 2007-06-01 22:55:01.000000000 +0200 @@ -173,5 +173,15 @@ extern void swsusp_close(void); extern int suspend_enter(suspend_state_t state); struct timeval; +/* kernel/power/swsusp.c */ extern void swsusp_show_speed(struct timeval *, struct timeval *, unsigned int, char *); + +/* kernel/power/main.c */ +extern struct blocking_notifier_head pm_chain_head; + +static inline int pm_notifier_call_chain(unsigned long val) +{ + return (blocking_notifier_call_chain(&pm_chain_head, val, NULL) + == NOTIFY_BAD) ? -EINVAL : 0; +} Index: linux-2.6.22-rc3/include/linux/notifier.h =================================================================== --- linux-2.6.22-rc3.orig/include/linux/notifier.h 2007-05-31 00:00:38.000000000 +0200 +++ linux-2.6.22-rc3/include/linux/notifier.h 2007-06-01 23:01:38.000000000 +0200 @@ -209,5 +209,11 @@ extern int __srcu_notifier_call_chain(st #define CPU_DOWN_FAILED_FROZEN (CPU_DOWN_FAILED | CPU_TASKS_FROZEN) #define CPU_DEAD_FROZEN (CPU_DEAD | CPU_TASKS_FROZEN) +/* Hibernation and suspend events */ +#define PM_HIBERNATION_PREPARE 0x0001 /* Going to hibernate */ +#define PM_POST_HIBERNATION 0x0002 /* Hibernation finished */ +#define PM_SUSPEND_PREPARE 0x0003 /* Going to suspend the system */ +#define PM_POST_SUSPEND 0x0004 /* Suspend finished */ + #endif /* __KERNEL__ */ #endif /* _LINUX_NOTIFIER_H */ Index: linux-2.6.22-rc3/kernel/power/disk.c =================================================================== --- linux-2.6.22-rc3.orig/kernel/power/disk.c 2007-05-31 00:00:38.000000000 +0200 +++ linux-2.6.22-rc3/kernel/power/disk.c 2007-06-01 23:05:11.000000000 +0200 @@ -281,9 +281,16 @@ int hibernate(void) { int error; + mutex_lock(&pm_mutex); /* The snapshot device should not be opened while we're running */ - if (!atomic_add_unless(&snapshot_device_available, -1, 0)) - return -EBUSY; + if (!atomic_add_unless(&snapshot_device_available, -1, 0)) { + error = -EBUSY; + goto Unlock; + } + + error = pm_notifier_call_chain(PM_HIBERNATION_PREPARE); + if (error) + goto Exit; /* Allocate memory management structures */ error = create_basic_memory_bitmaps(); @@ -294,7 +301,6 @@ int hibernate(void) if (error) goto Finish; - mutex_lock(&pm_mutex); if (hibernation_mode == HIBERNATION_TESTPROC) { printk("swsusp debug: Waiting for 5 seconds.\n"); mdelay(5000); @@ -316,12 +322,14 @@ int hibernate(void) swsusp_free(); } Thaw: - mutex_unlock(&pm_mutex); unprepare_processes(); Finish: free_basic_memory_bitmaps(); Exit: + pm_notifier_call_chain(PM_POST_HIBERNATION); atomic_inc(&snapshot_device_available); + Unlock: + mutex_unlock(&pm_mutex); return error; } Index: linux-2.6.22-rc3/kernel/power/main.c =================================================================== --- linux-2.6.22-rc3.orig/kernel/power/main.c 2007-05-31 00:00:38.000000000 +0200 +++ linux-2.6.22-rc3/kernel/power/main.c 2007-06-01 23:08:43.000000000 +0200 @@ -24,6 +24,8 @@ #include "power.h" +BLOCKING_NOTIFIER_HEAD(pm_chain_head); + /*This is just an arbitrary number */ #define FREE_PAGE_NUMBER (100) @@ -82,6 +84,10 @@ static int suspend_prepare(suspend_state if (!pm_ops || !pm_ops->enter) return -EPERM; + error = pm_notifier_call_chain(PM_SUSPEND_PREPARE); + if (error) + goto Finish; + pm_prepare_console(); if (freeze_processes()) { @@ -124,6 +130,8 @@ static int suspend_prepare(suspend_state Thaw: thaw_processes(); pm_restore_console(); + Finish: + pm_notifier_call_chain(PM_POST_SUSPEND); return error; } @@ -175,6 +183,7 @@ static void suspend_finish(suspend_state resume_console(); thaw_processes(); pm_restore_console(); + pm_notifier_call_chain(PM_POST_SUSPEND); } Index: linux-2.6.22-rc3/kernel/power/user.c =================================================================== --- linux-2.6.22-rc3.orig/kernel/power/user.c 2007-05-31 00:00:38.000000000 +0200 +++ linux-2.6.22-rc3/kernel/power/user.c 2007-06-01 23:15:58.000000000 +0200 @@ -149,10 +149,14 @@ static int snapshot_ioctl(struct inode * if (data->frozen) break; mutex_lock(&pm_mutex); - if (freeze_processes()) { - thaw_processes(); - error = -EBUSY; + error = pm_notifier_call_chain(PM_HIBERNATION_PREPARE); + if (!error) { + error = freeze_processes(); + if (error) + thaw_processes(); } + if (error) + pm_notifier_call_chain(PM_POST_HIBERNATION); mutex_unlock(&pm_mutex); if (!error) data->frozen = 1; @@ -163,6 +167,7 @@ static int snapshot_ioctl(struct inode * break; mutex_lock(&pm_mutex); thaw_processes(); + pm_notifier_call_chain(PM_POST_HIBERNATION); mutex_unlock(&pm_mutex); data->frozen = 0; break; Index: linux-2.6.22-rc3/Documentation/power/notifiers.txt =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ linux-2.6.22-rc3/Documentation/power/notifiers.txt 2007-06-01 23:14:59.000000000 +0200 @@ -0,0 +1,50 @@ +Suspend notifiers + (C) 2007 Rafael J. Wysocki <rjw@sisk.pl>, GPL + +There are some operations that device drivers may want to carry out in their +.suspend() routines, but shouldn't, because they can cause the hibernation or +suspend to fail. For example, a driver may want to allocate a substantial amount +of memory (like 50 MB) in .suspend(), but that shouldn't be done after the +swsusp's memory shrinker has run. + +Also, there may be some operations, that subsystems want to carry out before a +hibernation/suspend or after a restore/resume, requiring the system to be fully +functional, so the drivers' .suspend() and .resume() routines are not suitable +for this purpose. For example, device drivers may want to upload firmware to +their devices after a restore from a hibernation image, but they cannot do it by +calling request_firmware() from their .resume() routines (user land processes +are frozen at this point). The solution may be to load the firmware into +memory before processes are frozen and upload it from there in the .resume() +routine. Of course, a hibernation notifier may be used for this purpose. + +The subsystems that have such needs can register suspend notifiers that will be +called upon the following events by the suspend core: + +PM_HIBERNATION_PREPARE The system is going to hibernate or suspend, tasks will + be frozen immediately. + +PM_POST_HIBERNATION The system memory state has been restored from a + hibernation image or an error occured during the + hibernation. Device drivers' .resume() callbacks have + been executed and tasks have been thawed. + +PM_SUSPEND_PREPARE The system is preparing for a suspend. + +PM_POST_SUSPEND The system has just resumed or an error occured during + the suspend. Device drivers' .resume() callbacks have + been executed and tasks have been thawed. + +It is generally assumed that whatever the notifiers do for +PM_HIBERNATION_PREPARE, should be undone for PM_POST_HIBERNATION. Analogously, +operations performed for PM_SUSPEND_PREPARE should be reversed for +PM_POST_SUSPEND. Additionally, all of the notifiers are called for +PM_POST_HIBERNATION if one of them fails for PM_HIBERNATION_PREPARE, and +all of the notifiers are called for PM_POST_SUSPEND if one of them fails for +PM_SUSPEND_PREPARE. + +The hibernation and suspend notifiers are called with pm_mutex held. They are +defined in the usual way, but their last argument is meaningless (it is always +NULL). To register and/or unregister a suspend notifier use the functions +register_pm_notifier() and unregister_pm_notifier(), respectively, defined in +include/linux/suspend.h . If you don't need to unregister the notifier, you can +also use the pm_notifier() macro defined in include/linux/suspend.h . ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend 2007-06-04 21:25 [PATCH -mm 0/2] PM: Hibernation and suspend notifiers Rafael J. Wysocki 2007-06-04 21:27 ` [PATCH -mm 1/2] PM: Introduce hibernation " Rafael J. Wysocki @ 2007-06-04 21:28 ` Rafael J. Wysocki 2007-06-04 21:33 ` Nigel Cunningham 2007-06-15 13:08 ` Uli Luckas 1 sibling, 2 replies; 10+ messages in thread From: Rafael J. Wysocki @ 2007-06-04 21:28 UTC (permalink / raw) To: Andrew Morton; +Cc: LKML, Nigel Cunningham, Pavel Machek From: Rafael J. Wysocki <rjw@sisk.pl> Use a hibernation and suspend notifier to disable the user mode helper before a hibernation/suspend and enable it after the operation. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> Acked-by: Pavel Machek <pavel@ucw.cz> --- kernel/kmod.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) Index: linux-2.6.22-rc3/kernel/kmod.c =================================================================== --- linux-2.6.22-rc3.orig/kernel/kmod.c 2007-05-31 00:00:37.000000000 +0200 +++ linux-2.6.22-rc3/kernel/kmod.c 2007-06-02 00:01:47.000000000 +0200 @@ -33,6 +33,8 @@ #include <linux/kernel.h> #include <linux/init.h> #include <linux/resource.h> +#include <linux/notifier.h> +#include <linux/suspend.h> #include <asm/uaccess.h> extern int max_threads; @@ -46,6 +48,14 @@ static struct workqueue_struct *khelper_ */ char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe"; +/* + * If set, both call_usermodehelper_keys() and call_usermodehelper_pipe() exit + * immediately returning -EBUSY. Used for preventing user land processes from + * being created after the user land has been frozen during a system-wide + * hibernation or suspend operation. + */ +static int usermodehelper_disabled; + /** * request_module - try to load a kernel module * @fmt: printf style format string for the name of the module @@ -251,6 +261,24 @@ static void __call_usermodehelper(struct complete(sub_info->complete); } +static int usermodehelper_pm_callback(struct notifier_block *nfb, + unsigned long action, + void *ignored) +{ + switch (action) { + case PM_HIBERNATION_PREPARE: + case PM_SUSPEND_PREPARE: + usermodehelper_disabled = 1; + return NOTIFY_OK; + case PM_POST_HIBERNATION: + case PM_POST_SUSPEND: + usermodehelper_disabled = 0; + return NOTIFY_OK; + } + + return NOTIFY_DONE; +} + /** * call_usermodehelper_keys - start a usermode application * @path: pathname for the application @@ -276,7 +304,7 @@ int call_usermodehelper_keys(char *path, struct subprocess_info *sub_info; int retval; - if (!khelper_wq) + if (!khelper_wq || usermodehelper_disabled) return -EBUSY; if (path[0] == '\0') @@ -319,7 +347,7 @@ int call_usermodehelper_pipe(char *path, }; struct file *f; - if (!khelper_wq) + if (!khelper_wq || usermodehelper_disabled) return -EBUSY; if (path[0] == '\0') @@ -347,4 +375,5 @@ void __init usermodehelper_init(void) { khelper_wq = create_singlethread_workqueue("khelper"); BUG_ON(!khelper_wq); + pm_notifier(usermodehelper_pm_callback, 0); } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend 2007-06-04 21:28 ` [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend Rafael J. Wysocki @ 2007-06-04 21:33 ` Nigel Cunningham 2007-06-15 13:08 ` Uli Luckas 1 sibling, 0 replies; 10+ messages in thread From: Nigel Cunningham @ 2007-06-04 21:33 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Andrew Morton, LKML, Pavel Machek [-- Attachment #1: Type: text/plain, Size: 27 bytes --] Ack. Regards, Nigel [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend 2007-06-04 21:28 ` [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend Rafael J. Wysocki 2007-06-04 21:33 ` Nigel Cunningham @ 2007-06-15 13:08 ` Uli Luckas 2007-06-15 21:36 ` Rafael J. Wysocki 1 sibling, 1 reply; 10+ messages in thread From: Uli Luckas @ 2007-06-15 13:08 UTC (permalink / raw) To: LKML, LKML; +Cc: Rafael J. Wysocki On Monday, 4. June 2007, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > Use a hibernation and suspend notifier to disable the user mode helper > before a hibernation/suspend and enable it after the operation. > Hi Rafael, I have a couple of questions, regarding this patch ... > - if (!khelper_wq) > + if (!khelper_wq || usermodehelper_disabled) > return -EBUSY; 1) how about not returning -EBUSY here, if (wait == UMH_NO_WAIT)? 2) how does your patch prevent wait_for_completion(&done) to hang during freezing if usermodehelper_pm_callback is called _after_ the above check? Regards Uli ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend 2007-06-15 13:08 ` Uli Luckas @ 2007-06-15 21:36 ` Rafael J. Wysocki 2007-06-17 19:24 ` Rafael J. Wysocki 0 siblings, 1 reply; 10+ messages in thread From: Rafael J. Wysocki @ 2007-06-15 21:36 UTC (permalink / raw) To: Uli Luckas; +Cc: LKML, Pavel Machek On Friday, 15 June 2007 15:08, Uli Luckas wrote: > On Monday, 4. June 2007, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > Use a hibernation and suspend notifier to disable the user mode helper > > before a hibernation/suspend and enable it after the operation. > > > Hi Rafael, > I have a couple of questions, regarding this patch ... > > - if (!khelper_wq) > > + if (!khelper_wq || usermodehelper_disabled) > > return -EBUSY; > 1) how about not returning -EBUSY here, if (wait == UMH_NO_WAIT)? Yes, this would make sense. > 2) how does your patch prevent wait_for_completion(&done) to hang during > freezing if usermodehelper_pm_callback is called _after_ the above check? It doesn't. Once the helper is running, we can't distinguish it from any other user land process. Still, it narrows the window quite a bit. Greetings, Rafael -- "Premature optimization is the root of all evil." - Donald Knuth ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend 2007-06-15 21:36 ` Rafael J. Wysocki @ 2007-06-17 19:24 ` Rafael J. Wysocki 2007-06-18 10:51 ` Uli Luckas 0 siblings, 1 reply; 10+ messages in thread From: Rafael J. Wysocki @ 2007-06-17 19:24 UTC (permalink / raw) To: Uli Luckas; +Cc: LKML, Pavel Machek On Friday, 15 June 2007 23:36, Rafael J. Wysocki wrote: > On Friday, 15 June 2007 15:08, Uli Luckas wrote: > > On Monday, 4. June 2007, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > > > Use a hibernation and suspend notifier to disable the user mode helper > > > before a hibernation/suspend and enable it after the operation. > > > > > Hi Rafael, > > I have a couple of questions, regarding this patch ... > > > - if (!khelper_wq) > > > + if (!khelper_wq || usermodehelper_disabled) > > > return -EBUSY; > > 1) how about not returning -EBUSY here, if (wait == UMH_NO_WAIT)? > > Yes, this would make sense. I know why I didn't do that from the start: my patch was against -rc4 and UMH_NO_WAIT is mm-only. > > 2) how does your patch prevent wait_for_completion(&done) to hang during > > freezing if usermodehelper_pm_callback is called _after_ the above check? > > It doesn't. Once the helper is running, we can't distinguish it from any other > user land process. > > Still, it narrows the window quite a bit. Okay, I think we can help it a bit. Please tell me what you think of the following patch (on top of 2.6.22-rc4-mm2). Greetings, Rafael Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- kernel/kmod.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) Index: linux-2.6.22-rc4-mm2/kernel/kmod.c =================================================================== --- linux-2.6.22-rc4-mm2.orig/kernel/kmod.c +++ linux-2.6.22-rc4-mm2/kernel/kmod.c @@ -49,6 +49,15 @@ static struct workqueue_struct *khelper_ */ static int usermodehelper_disabled; +/* Number of helpers running */ +static atomic_t running_helpers = ATOMIC_INIT(0); + +/* + * Time to wait for running_helpers to become zero before the setting of + * usermodehelper_disabled in usermodehelper_pm_callback() fails + */ +#define RUNNING_HELPERS_TIMEOUT (5 * HZ) + #ifdef CONFIG_KMOD /* @@ -279,11 +288,20 @@ static int usermodehelper_pm_callback(st unsigned long action, void *ignored) { + DECLARE_WAIT_QUEUE_HEAD_ONSTACK(waiting); + switch (action) { case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + wait_event_timeout(waiting, atomic_read(&running_helpers) == 0, + RUNNING_HELPERS_TIMEOUT); + if (atomic_read(&running_helpers) == 0) { + return NOTIFY_OK; + } else { + usermodehelper_disabled = 0; + return NOTIFY_BAD; + } case PM_POST_HIBERNATION: case PM_POST_SUSPEND: usermodehelper_disabled = 0; @@ -397,12 +415,13 @@ int call_usermodehelper_exec(struct subp DECLARE_COMPLETION_ONSTACK(done); int retval; + atomic_inc(&running_helpers); if (sub_info->path[0] == '\0') { retval = 0; goto out; } - if (!khelper_wq || usermodehelper_disabled) { + if (!khelper_wq || (wait != UMH_NO_WAIT && usermodehelper_disabled)) { retval = -EBUSY; goto out; } @@ -418,6 +437,7 @@ int call_usermodehelper_exec(struct subp out: call_usermodehelper_freeinfo(sub_info); + atomic_dec(&running_helpers); return retval; } EXPORT_SYMBOL(call_usermodehelper_exec); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend 2007-06-17 19:24 ` Rafael J. Wysocki @ 2007-06-18 10:51 ` Uli Luckas 2007-06-18 15:15 ` Rafael J. Wysocki 0 siblings, 1 reply; 10+ messages in thread From: Uli Luckas @ 2007-06-18 10:51 UTC (permalink / raw) To: LKML; +Cc: Rafael J. Wysocki On Sunday, 17. June 2007, you wrote: > On Friday, 15 June 2007 23:36, Rafael J. Wysocki wrote: > > On Friday, 15 June 2007 15:08, Uli Luckas wrote: > > > On Monday, 4. June 2007, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > > > > > Use a hibernation and suspend notifier to disable the user mode > > > > helper before a hibernation/suspend and enable it after the > > > > operation. > > > > > > Hi Rafael, > > > I have a couple of questions, regarding this patch ... > > > 2) how does your patch prevent wait_for_completion(&done) to hang > > > during freezing if usermodehelper_pm_callback is called _after_ the > > > above check? > > > > It doesn't. Once the helper is running, we can't distinguish it from any > > other user land process. > > > > Still, it narrows the window quite a bit. > > Okay, I think we can help it a bit. Please tell me what you think of the > following patch (on top of 2.6.22-rc4-mm2). > Hi Rafael, Thanks for your work. I haven't found the time to actually test your patch but in general it looks like a valid aproach. I think there is one problem though. You need to have your wait queue woken up when you update (atomic_dec) running_helpers. Otherwise you end up always waiting the full RUNNING_HELPERS_TIMEOUT. > Index: linux-2.6.22-rc4-mm2/kernel/kmod.c > =================================================================== > --- linux-2.6.22-rc4-mm2.orig/kernel/kmod.c > +++ linux-2.6.22-rc4-mm2/kernel/kmod.c > @@ -49,6 +49,15 @@ static struct workqueue_struct *khelper_ > */ > static int usermodehelper_disabled; > > +/* Number of helpers running */ > +static atomic_t running_helpers = ATOMIC_INIT(0); > + > +/* > + * Time to wait for running_helpers to become zero before the setting of > + * usermodehelper_disabled in usermodehelper_pm_callback() fails > + */ > +#define RUNNING_HELPERS_TIMEOUT (5 * HZ) > + > #ifdef CONFIG_KMOD > > /* > @@ -279,11 +288,20 @@ static int usermodehelper_pm_callback(st > unsigned long action, > void *ignored) > { > + DECLARE_WAIT_QUEUE_HEAD_ONSTACK(waiting); > + > switch (action) { > case PM_HIBERNATION_PREPARE: > case PM_SUSPEND_PREPARE: > usermodehelper_disabled = 1; > - return NOTIFY_OK; > + wait_event_timeout(waiting, atomic_read(&running_helpers) == 0, > + RUNNING_HELPERS_TIMEOUT); > + if (atomic_read(&running_helpers) == 0) { > + return NOTIFY_OK; > + } else { > + usermodehelper_disabled = 0; > + return NOTIFY_BAD; > + } > case PM_POST_HIBERNATION: > case PM_POST_SUSPEND: > usermodehelper_disabled = 0; > @@ -397,12 +415,13 @@ int call_usermodehelper_exec(struct subp > DECLARE_COMPLETION_ONSTACK(done); > int retval; > > + atomic_inc(&running_helpers); > if (sub_info->path[0] == '\0') { > retval = 0; > goto out; > } > > - if (!khelper_wq || usermodehelper_disabled) { > + if (!khelper_wq || (wait != UMH_NO_WAIT && usermodehelper_disabled)) { > retval = -EBUSY; > goto out; > } > @@ -418,6 +437,7 @@ int call_usermodehelper_exec(struct subp > > out: > call_usermodehelper_freeinfo(sub_info); > + atomic_dec(&running_helpers); > return retval; > } > EXPORT_SYMBOL(call_usermodehelper_exec); -- ------- ROAD ...the handyPC Company - - - ) ) ) Uli Luckas Software Development ROAD GmbH Bennigsenstr. 14 | 12159 Berlin | Germany fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69 url: www.road.de Amtsgericht Charlottenburg: HRB 96688 B Managing directors: Hans-Peter Constien, Hubertus von Streit ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend 2007-06-18 10:51 ` Uli Luckas @ 2007-06-18 15:15 ` Rafael J. Wysocki 2007-06-18 18:23 ` Uli Luckas 0 siblings, 1 reply; 10+ messages in thread From: Rafael J. Wysocki @ 2007-06-18 15:15 UTC (permalink / raw) To: Uli Luckas; +Cc: LKML, Pavel Machek On Monday, 18 June 2007 12:51, Uli Luckas wrote: > On Sunday, 17. June 2007, you wrote: > > On Friday, 15 June 2007 23:36, Rafael J. Wysocki wrote: > > > On Friday, 15 June 2007 15:08, Uli Luckas wrote: > > > > On Monday, 4. June 2007, Rafael J. Wysocki wrote: > > > > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > > > > > > > Use a hibernation and suspend notifier to disable the user mode > > > > > helper before a hibernation/suspend and enable it after the > > > > > operation. > > > > > > > > Hi Rafael, > > > > I have a couple of questions, regarding this patch ... > > > > > 2) how does your patch prevent wait_for_completion(&done) to hang > > > > during freezing if usermodehelper_pm_callback is called _after_ the > > > > above check? > > > > > > It doesn't. Once the helper is running, we can't distinguish it from any > > > other user land process. > > > > > > Still, it narrows the window quite a bit. > > > > Okay, I think we can help it a bit. Please tell me what you think of the > > following patch (on top of 2.6.22-rc4-mm2). > > > Hi Rafael, > Thanks for your work. I haven't found the time to actually test your patch but > in general it looks like a valid aproach. > I think there is one problem though. You need to have your wait queue woken up > when you update (atomic_dec) running_helpers. Otherwise you end up always > waiting the full RUNNING_HELPERS_TIMEOUT. Yeah, right. I tend to forget about obvious things. :-( Besides, I think that the _pm_callback need not be compiled if CONFIG_PM is unset. If you can, please have a look at the next version of the patch (appended). Greetings, Rafael Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- kernel/kmod.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 10 deletions(-) Index: linux-2.6.22-rc4-mm2/kernel/kmod.c =================================================================== --- linux-2.6.22-rc4-mm2.orig/kernel/kmod.c +++ linux-2.6.22-rc4-mm2/kernel/kmod.c @@ -41,14 +41,6 @@ extern int max_threads; static struct workqueue_struct *khelper_wq; -/* - * If set, both call_usermodehelper_keys() and call_usermodehelper_pipe() exit - * immediately returning -EBUSY. Used for preventing user land processes from - * being created after the user land has been frozen during a system-wide - * hibernation or suspend operation. - */ -static int usermodehelper_disabled; - #ifdef CONFIG_KMOD /* @@ -275,6 +267,30 @@ static void __call_usermodehelper(struct } } +#ifdef CONFIG_PM +/* + * If set and if call_usermodehelper_exec() is supposed to wait, it will exit + * immediately returning -EBUSY (used for preventing user land processes from + * being created after the user land has been frozen during a system-wide + * hibernation or suspend operation). + */ +static int usermodehelper_disabled; + +/* Number of helpers running */ +static atomic_t running_helpers = ATOMIC_INIT(0); + +/* + * Wait queue head used by usermodehelper_pm_callback() to wait for all running + * helpers to finish. + */ +static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq); + +/* + * Time to wait for running_helpers to become zero before the setting of + * usermodehelper_disabled in usermodehelper_pm_callback() fails + */ +#define RUNNING_HELPERS_TIMEOUT (5 * HZ) + static int usermodehelper_pm_callback(struct notifier_block *nfb, unsigned long action, void *ignored) @@ -283,7 +299,15 @@ static int usermodehelper_pm_callback(st case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: usermodehelper_disabled = 1; - return NOTIFY_OK; + wait_event_timeout(running_helpers_waitq, + atomic_read(&running_helpers) == 0, + RUNNING_HELPERS_TIMEOUT); + if (atomic_read(&running_helpers) == 0) { + return NOTIFY_OK; + } else { + usermodehelper_disabled = 0; + return NOTIFY_BAD; + } case PM_POST_HIBERNATION: case PM_POST_SUSPEND: usermodehelper_disabled = 0; @@ -293,6 +317,36 @@ static int usermodehelper_pm_callback(st return NOTIFY_DONE; } +static void new_helper(void) +{ + atomic_inc(&running_helpers); +} + +static void helper_finished(void) +{ + if (atomic_dec_and_test(&running_helpers)) + wake_up(&running_helpers_waitq); +} + +#else /* CONFIG_PM */ +#define usermodehelper_disabled 0 + +static inline int usermodehelper_pm_callback(struct notifier_block *nfb, + unsigned long action, + void *ignored) +{ + return 0; +} + +static inline void new_helper(void) +{ +} + +static inline void helper_finished(void) +{ +} +#endif /* CONFIG_PM */ + /** * call_usermodehelper_setup - prepare to call a usermode helper * @path - path to usermode executable @@ -397,12 +451,13 @@ int call_usermodehelper_exec(struct subp DECLARE_COMPLETION_ONSTACK(done); int retval; + new_helper(); if (sub_info->path[0] == '\0') { retval = 0; goto out; } - if (!khelper_wq || usermodehelper_disabled) { + if (!khelper_wq || (wait != UMH_NO_WAIT && usermodehelper_disabled)) { retval = -EBUSY; goto out; } @@ -418,6 +473,7 @@ int call_usermodehelper_exec(struct subp out: call_usermodehelper_freeinfo(sub_info); + helper_finished(); return retval; } EXPORT_SYMBOL(call_usermodehelper_exec); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend 2007-06-18 15:15 ` Rafael J. Wysocki @ 2007-06-18 18:23 ` Uli Luckas 0 siblings, 0 replies; 10+ messages in thread From: Uli Luckas @ 2007-06-18 18:23 UTC (permalink / raw) To: LKML; +Cc: Rafael J. Wysocki On Monday, 18. June 2007, you wrote: > On Monday, 18 June 2007 12:51, Uli Luckas wrote: > > On Sunday, 17. June 2007, you wrote: > > > On Friday, 15 June 2007 23:36, Rafael J. Wysocki wrote: > > > > On Friday, 15 June 2007 15:08, Uli Luckas wrote: > > > > > On Monday, 4. June 2007, Rafael J. Wysocki wrote: > > > > > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > > > > > > > > > Use a hibernation and suspend notifier to disable the user mode > > > > > > helper before a hibernation/suspend and enable it after the > > > > > > operation. > > > > > > > > > > Hi Rafael, > > > > > I have a couple of questions, regarding this patch ... > > > > > > > > > > 2) how does your patch prevent wait_for_completion(&done) to hang > > > > > during freezing if usermodehelper_pm_callback is called _after_ the > > > > > above check? > > > > > > > > It doesn't. Once the helper is running, we can't distinguish it from > > > > any other user land process. > > > > > > > > Still, it narrows the window quite a bit. > > > > > > Okay, I think we can help it a bit. Please tell me what you think of > > > the following patch (on top of 2.6.22-rc4-mm2). > > > > Hi Rafael, > > Thanks for your work. I haven't found the time to actually test your > > patch but in general it looks like a valid aproach. > > I think there is one problem though. You need to have your wait queue > > woken up when you update (atomic_dec) running_helpers. Otherwise you end > > up always waiting the full RUNNING_HELPERS_TIMEOUT. > > Yeah, right. I tend to forget about obvious things. :-( > Great. As far as I can see, this patch should now fix the situation except under heavy load where it would abort the attempted suspend. I can live with that and it is a whole lot better than it was before - Thanks. > Besides, I think that the _pm_callback need not be compiled if CONFIG_PM is > unset. > Should work but is probably beneath what you intended to do. I'll comment further down... > If you can, please have a look at the next version of the patch > (appended). > One more topic to keep in mind for the future. None of the callers of call_usermode_helper* expect a return value of EBUSY nor do they handle it correctly. If auto loading of a module for example fails, a permanent error is usually assumed and no retry attempts are made. > Greetings, > Rafael > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > --- > kernel/kmod.c | 76 > ++++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, > 66 insertions(+), 10 deletions(-) > > Index: linux-2.6.22-rc4-mm2/kernel/kmod.c > =================================================================== > --- linux-2.6.22-rc4-mm2.orig/kernel/kmod.c > +++ linux-2.6.22-rc4-mm2/kernel/kmod.c > @@ -41,14 +41,6 @@ extern int max_threads; > > static struct workqueue_struct *khelper_wq; > > -/* > - * If set, both call_usermodehelper_keys() and call_usermodehelper_pipe() > exit - * immediately returning -EBUSY. Used for preventing user land > processes from - * being created after the user land has been frozen during > a system-wide - * hibernation or suspend operation. > - */ > -static int usermodehelper_disabled; > - > #ifdef CONFIG_KMOD > > /* > @@ -275,6 +267,30 @@ static void __call_usermodehelper(struct > } > } > > +#ifdef CONFIG_PM > +/* > + * If set and if call_usermodehelper_exec() is supposed to wait, it will > exit + * immediately returning -EBUSY (used for preventing user land > processes from + * being created after the user land has been frozen during > a system-wide + * hibernation or suspend operation). > + */ > +static int usermodehelper_disabled; > + > +/* Number of helpers running */ > +static atomic_t running_helpers = ATOMIC_INIT(0); > + > +/* > + * Wait queue head used by usermodehelper_pm_callback() to wait for all > running + * helpers to finish. > + */ > +static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq); > + > +/* > + * Time to wait for running_helpers to become zero before the setting of > + * usermodehelper_disabled in usermodehelper_pm_callback() fails > + */ > +#define RUNNING_HELPERS_TIMEOUT (5 * HZ) > + > static int usermodehelper_pm_callback(struct notifier_block *nfb, > unsigned long action, > void *ignored) > @@ -283,7 +299,15 @@ static int usermodehelper_pm_callback(st > case PM_HIBERNATION_PREPARE: > case PM_SUSPEND_PREPARE: > usermodehelper_disabled = 1; > - return NOTIFY_OK; > + wait_event_timeout(running_helpers_waitq, > + atomic_read(&running_helpers) == 0, > + RUNNING_HELPERS_TIMEOUT); > + if (atomic_read(&running_helpers) == 0) { > + return NOTIFY_OK; > + } else { > + usermodehelper_disabled = 0; > + return NOTIFY_BAD; > + } > case PM_POST_HIBERNATION: > case PM_POST_SUSPEND: > usermodehelper_disabled = 0; > @@ -293,6 +317,36 @@ static int usermodehelper_pm_callback(st > return NOTIFY_DONE; > } > > +static void new_helper(void) > +{ > + atomic_inc(&running_helpers); > +} > + > +static void helper_finished(void) > +{ > + if (atomic_dec_and_test(&running_helpers)) > + wake_up(&running_helpers_waitq); > +} > + > +#else /* CONFIG_PM */ > +#define usermodehelper_disabled 0 > + > +static inline int usermodehelper_pm_callback(struct notifier_block *nfb, ^^^^^^ This kind of implies, the compiler might optimize this function away. Further down you call "pm_notifier(usermodehelper_pm_callback, 0);", thereby passing a pointer to usermodehelper_pm_callback which can't be inlined nor optimized away. If instead you had an inline function register_usermodehelper_pm_callback which called pm_notifier if CONFIG_PM was defined and did nothing otherwise... > + unsigned long action, > + void *ignored) > +{ > + return 0; > +} > + > +static inline void new_helper(void) > +{ > +} > + > +static inline void helper_finished(void) > +{ > +} > +#endif /* CONFIG_PM */ > + > /** > * call_usermodehelper_setup - prepare to call a usermode helper > * @path - path to usermode executable > @@ -397,12 +451,13 @@ int call_usermodehelper_exec(struct subp > DECLARE_COMPLETION_ONSTACK(done); > int retval; > > + new_helper(); > if (sub_info->path[0] == '\0') { > retval = 0; > goto out; > } > > - if (!khelper_wq || usermodehelper_disabled) { > + if (!khelper_wq || (wait != UMH_NO_WAIT && usermodehelper_disabled)) { > retval = -EBUSY; > goto out; > } > @@ -418,6 +473,7 @@ int call_usermodehelper_exec(struct subp > > out: > call_usermodehelper_freeinfo(sub_info); > + helper_finished(); > return retval; > } > EXPORT_SYMBOL(call_usermodehelper_exec); -- ------- ROAD ...the handyPC Company - - - ) ) ) Uli Luckas Software Development ROAD GmbH Bennigsenstr. 14 | 12159 Berlin | Germany fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69 url: www.road.de Amtsgericht Charlottenburg: HRB 96688 B Managing directors: Hans-Peter Constien, Hubertus von Streit ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-06-18 18:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-06-04 21:25 [PATCH -mm 0/2] PM: Hibernation and suspend notifiers Rafael J. Wysocki 2007-06-04 21:27 ` [PATCH -mm 1/2] PM: Introduce hibernation " Rafael J. Wysocki 2007-06-04 21:28 ` [PATCH -mm 2/2] PM: Disable usermode helper before hibernation and suspend Rafael J. Wysocki 2007-06-04 21:33 ` Nigel Cunningham 2007-06-15 13:08 ` Uli Luckas 2007-06-15 21:36 ` Rafael J. Wysocki 2007-06-17 19:24 ` Rafael J. Wysocki 2007-06-18 10:51 ` Uli Luckas 2007-06-18 15:15 ` Rafael J. Wysocki 2007-06-18 18:23 ` Uli Luckas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).