[RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 6342 bytes --]

Hi,

This is an experimantal patch aimed at the "unable to freeze processes under
load" problem.

On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
"dd if=/dev/hda of=/dev/null" test.

Please have a look.

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 include/linux/kmod.h    |    3 ++
 include/linux/suspend.h |    4 +++
 kernel/kmod.c           |   23 +++++++++++++++++
 kernel/power/process.c  |   62 +++++++++++++++++++++++++++++++++++++++++-------
 4 files changed, 83 insertions(+), 9 deletions(-)

Index: linux-2.6.16-rc1-mm4/kernel/power/process.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/process.c	2006-01-31 23:45:59.000000000 +0100
+++ linux-2.6.16-rc1-mm4/kernel/power/process.c	2006-02-01 01:12:14.000000000 +0100
@@ -12,12 +12,19 @@
 #include <linux/interrupt.h>
 #include <linux/suspend.h>
 #include <linux/module.h>
+#include <linux/kmod.h>
+#include <linux/mutex.h>
 
 /* 
  * Timeout for stopping processes
  */
 #define TIMEOUT	(6 * HZ)
 
+/* This is used to disable usermodehelper invocations while
+ * freeze_processes() is being executed
+ */
+int freezing_processes;
+DEFINE_MUTEX(freezer_lock);
 
 static inline int freezeable(struct task_struct * p)
 {
@@ -54,40 +61,77 @@ void refrigerator(void)
 	current->state = save;
 }
 
+static inline void freeze_process(struct task_struct *p)
+{
+	unsigned long flags;
+
+	freeze(p);
+	spin_lock_irqsave(&p->sighand->siglock, flags);
+	signal_wake_up(p, 0);
+	spin_unlock_irqrestore(&p->sighand->siglock, flags);
+}
+
 /* 0 = success, else # of processes that we failed to stop */
 int freeze_processes(void)
 {
-	int todo;
+	int todo, nr_kernel, nr_user, user_frozen;
 	unsigned long start_time;
 	struct task_struct *g, *p;
 	unsigned long flags;
 
+	mutex_lock(&freezer_lock);
+	freezing_processes = 1;
+	mutex_unlock(&freezer_lock);
+	while (atomic_read(&usermodehelper_waiting))
+		schedule();
+
 	printk( "Stopping tasks: " );
 	start_time = jiffies;
+	user_frozen = 0;
 	do {
-		todo = 0;
+		nr_kernel = 0;
+		nr_user = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			if (!freezeable(p))
 				continue;
 			if (frozen(p))
 				continue;
-
-			freeze(p);
-			spin_lock_irqsave(&p->sighand->siglock, flags);
-			signal_wake_up(p, 0);
-			spin_unlock_irqrestore(&p->sighand->siglock, flags);
-			todo++;
+			if (p->mm) {
+				/* The task is a user-space one. Freeze it */
+				printk("  freezing %s\n", p->comm);
+				freeze_process(p);
+				nr_user++;
+			} else {
+				/* Freeze only if the user space is frozen */
+				if (user_frozen) {
+					printk("  freezing [%s]\n", p->comm);
+					freeze_process(p);
+				}
+				nr_kernel++;
+			}
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
+		user_frozen = !nr_user;
+		todo = nr_user + nr_kernel;
 		yield();			/* Yield is okay here */
 		if (todo && time_after(jiffies, start_time + TIMEOUT)) {
 			printk( "\n" );
-			printk(KERN_ERR " stopping tasks timed out (%d tasks remaining)\n", todo );
+			printk(KERN_ERR " stopping tasks timed out "
+				"after %d seconds (%d tasks remaining):\n",
+				TIMEOUT / HZ, todo);
+			do_each_thread(g, p) {
+				if (freezeable(p) && !frozen(p))
+					printk(KERN_ERR "  %s\n", p->comm);
+			} while_each_thread(g, p);
 			break;
 		}
 	} while(todo);
 
+	mutex_lock(&freezer_lock);
+	freezing_processes = 0;
+	mutex_unlock(&freezer_lock);
+
 	/* This does not unfreeze processes that are already frozen
 	 * (we have slightly ugly calling convention in that respect,
 	 * and caller must call thaw_processes() if something fails),
Index: linux-2.6.16-rc1-mm4/kernel/kmod.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/kmod.c	2006-01-31 23:45:59.000000000 +0100
+++ linux-2.6.16-rc1-mm4/kernel/kmod.c	2006-02-01 00:20:10.000000000 +0100
@@ -36,6 +36,8 @@
 #include <linux/mount.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/suspend.h>
+#include <linux/mutex.h>
 #include <asm/uaccess.h>
 
 extern int max_threads;
@@ -216,6 +218,10 @@ static void __call_usermodehelper(void *
 		complete(sub_info->complete);
 }
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+atomic_t usermodehelper_waiting = ATOMIC_INIT(0);
+#endif
+
 /**
  * call_usermodehelper_keys - start a usermode application
  * @path: pathname for the application
@@ -249,11 +255,28 @@ int call_usermodehelper_keys(char *path,
 	if (!khelper_wq)
 		return -EBUSY;
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+	mutex_lock(&freezer_lock);
+	if (freezing_processes) {
+		mutex_unlock(&freezer_lock);
+		return 0;
+	}
+	if (wait)
+		atomic_inc(&usermodehelper_waiting);
+	mutex_unlock(&freezer_lock);
+#endif
+
 	if (path[0] == '\0')
 		return 0;
 
 	queue_work(khelper_wq, &work);
 	wait_for_completion(&done);
+
+#ifdef CONFIG_SOFTWARE_SUSPEND
+	if (wait)
+		atomic_dec(&usermodehelper_waiting);
+#endif
+
 	return sub_info.retval;
 }
 EXPORT_SYMBOL(call_usermodehelper_keys);
Index: linux-2.6.16-rc1-mm4/include/linux/suspend.h
===================================================================
--- linux-2.6.16-rc1-mm4.orig/include/linux/suspend.h	2006-01-31 23:45:59.000000000 +0100
+++ linux-2.6.16-rc1-mm4/include/linux/suspend.h	2006-02-01 00:20:20.000000000 +0100
@@ -40,6 +40,10 @@ extern void drain_local_pages(void);
 extern void mark_free_pages(struct zone *zone);
 
 #ifdef CONFIG_PM
+struct mutex;
+extern struct mutex freezer_lock;
+extern int freezing_processes;
+
 /* kernel/power/swsusp.c */
 extern int software_suspend(void);
 
Index: linux-2.6.16-rc1-mm4/include/linux/kmod.h
===================================================================
--- linux-2.6.16-rc1-mm4.orig/include/linux/kmod.h	2006-01-31 23:45:59.000000000 +0100
+++ linux-2.6.16-rc1-mm4/include/linux/kmod.h	2006-02-01 00:06:19.000000000 +0100
@@ -23,6 +23,7 @@
 #include <linux/config.h>
 #include <linux/errno.h>
 #include <linux/compiler.h>
+#include <asm/atomic.h>
 
 #define KMOD_PATH_LEN 256
 
@@ -36,6 +37,8 @@ static inline int request_module(const c
 
 #define try_then_request_module(x, mod...) ((x) ?: (request_module(mod), (x)))
 
+extern atomic_t usermodehelper_waiting;
+
 struct key;
 extern int call_usermodehelper_keys(char *path, char *argv[], char *envp[],
 				    struct key *session_keyring, int wait);

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

Hi!

> This is an experimantal patch aimed at the "unable to freeze processes under
> load" problem.
> 
> On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
> "dd if=/dev/hda of=/dev/null" test.

What filesystem? ext2 vs. ext3 is very different. I can fix ext2 by
just upping the timeout. ext3 is harder, because of the journalling
thread. Maybe I'll take the sledgehammer approach and just freeze all
the user tasks first... I tried to do the "small" approach and just
fix the journalling code...

I'm not sure what problem the usermodehelper code is trying to
solve... why are usermodehelpers special?
								Pavel


-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 1008 bytes --]

Hi.

On Wednesday 01 February 2006 20:55, Pavel Machek wrote:
> Hi!
>
> > This is an experimantal patch aimed at the "unable to freeze processes
> > under load" problem.
> >
> > On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
> > "dd if=/dev/hda of=/dev/null" test.
>
> What filesystem? ext2 vs. ext3 is very different. I can fix ext2 by
> just upping the timeout. ext3 is harder, because of the journalling
> thread. Maybe I'll take the sledgehammer approach and just freeze all
> the user tasks first... I tried to do the "small" approach and just
> fix the journalling code...
>
> I'm not sure what problem the usermodehelper code is trying to
> solve... why are usermodehelpers special?

This is related to the 'Hotplug events during sleep transition' thread from 
the PM list from December.

Regards,

Nigel


-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Rafael J. Wysocki]

Hi,

[Sorry, I was unable to respond earlier.]

On Wednesday 01 February 2006 11:55, Pavel Machek wrote:
> > This is an experimantal patch aimed at the "unable to freeze processes under
> > load" problem.
> > 
> > On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
> > "dd if=/dev/hda of=/dev/null" test.
> 
> What filesystem? ext2 vs. ext3 is very different.

Just the entire disk.

> I can fix ext2 by 
> just upping the timeout. ext3 is harder, because of the journalling
> thread. Maybe I'll take the sledgehammer approach and just freeze all
> the user tasks first... I tried to do the "small" approach and just
> fix the journalling code...
> 
> I'm not sure what problem the usermodehelper code is trying to
> solve... why are usermodehelpers special?

They may be waited for uninterruptilby, so we can get an unfreezable proces
if we freeze one of them.

Actually I think I made a mistake here, since call_usermodehelper_keys()
should return -EBUSY rather than 0 when freezing_processes is non-zero
(if we return 0, the caller gets confused, because it assumes the call has
happend and has been successful, which is wrong).

Greetings,
Rafael

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4038 bytes --]

Hi!

> This is an experimantal patch aimed at the "unable to freeze processes under
> load" problem.
> 
> On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
> "dd if=/dev/hda of=/dev/null" test.
> 
> Please have a look.

It makes it better (well, I used my own, simpler variant, but that
should not matter; patch is attached). I now can't reproduce hangs
with simple stress testing, but running kernel make alongside that
makes it hang sometimes. Example of non-frozen gcc:

gcc           D EEE06A70     0  1750   1749  1751
(NOTLB)
df85df38 00000046 bf878130 eee06a70 00004111 eee06a70 eee06a70
003d0900
       00000000 c0137cf5 df85c000 00000000 c058ada2 c012503e ef2c915c
ef2c9030
       c1c0b480 7c3b8500 003d0927 df85c000 00000a98 7c3b8500 003d0927
c0770800
Call Trace:
 [<c0137cf5>] attach_pid+0x25/0xb0
 [<c058ada2>] _write_unlock_irq+0x12/0x30
 [<c012503e>] copy_process+0xe5e/0x11b0
 [<c0588f74>] wait_for_completion+0x94/0xd0
 [<c0121690>] default_wake_function+0x0/0x10
 [<c01254d9>] do_fork+0x149/0x210
 [<c0101218>] sys_vfork+0x28/0x30
 [<c0103231>] syscall_call+0x7/0xb

...maybe solving this would solve journalling problems, too? It is
similar AFAICT.
								Pavel

diff --git a/kernel/power/disk.c b/kernel/power/disk.c
index e24446f..90d6c1a 100644
--- a/kernel/power/disk.c
+++ b/kernel/power/disk.c
@@ -87,7 +87,6 @@ static int prepare_processes(void)
 	int error;
 
 	pm_prepare_console();
-	sys_sync();
 	disable_nonboot_cpus();
 
 	if (freeze_processes()) {
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 02a1b3a..bd16e44 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -16,7 +16,7 @@
 /* 
  * Timeout for stopping processes
  */
-#define TIMEOUT	(6 * HZ)
+#define TIMEOUT	(60 * HZ)
 
 
 static inline int freezeable(struct task_struct * p)
@@ -54,32 +54,53 @@ void refrigerator(void)
 	current->state = save;
 }
 
+static void freeze_process(struct task_struct *p)
+{
+       unsigned long flags;
+
+       freeze(p);
+       spin_lock_irqsave(&p->sighand->siglock, flags);
+       signal_wake_up(p, 0);
+       spin_unlock_irqrestore(&p->sighand->siglock, flags);
+}
+
 /* 0 = success, else # of processes that we failed to stop */
 int freeze_processes(void)
 {
-	int todo;
+	int todo, user_frozen, nr_user;
 	unsigned long start_time;
 	struct task_struct *g, *p;
 	unsigned long flags;
 
 	printk( "Stopping tasks: " );
 	start_time = jiffies;
+	user_frozen = 0;
 	do {
-		todo = 0;
+		nr_user = todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			if (!freezeable(p))
 				continue;
 			if (frozen(p))
 				continue;
-
-			freeze(p);
-			spin_lock_irqsave(&p->sighand->siglock, flags);
-			signal_wake_up(p, 0);
-			spin_unlock_irqrestore(&p->sighand->siglock, flags);
-			todo++;
+			if (p->mm) {
+				/* The task is a user-space one. Freeze it */
+				freeze_process(p);
+				todo++;
+				nr_user++;
+			} else {
+				/* Freeze only if the user space is frozen */
+				if (user_frozen)
+					freeze_process(p);
+				todo++;
+			}
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
+		if (!user_frozen && !nr_user) {
+			printk("sync");
+			sys_sync();
+		}
+		user_frozen = !nr_user;
 		yield();			/* Yield is okay here */
 		if (todo && time_after(jiffies, start_time + TIMEOUT)) {
 			printk( "\n" );
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 41f6636..c9c293f 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -80,7 +80,7 @@ static int save_highmem_zone(struct zone
 		void *kaddr;
 		unsigned long pfn = zone_pfn + zone->zone_start_pfn;
 
-		if (!(pfn%1000))
+		if (!(pfn%10000))
 			printk(".");
 		if (!pfn_valid(pfn))
 			continue;
@@ -121,13 +121,14 @@ int save_highmem(void)
 	struct zone *zone;
 	int res = 0;
 
-	pr_debug("swsusp: Saving Highmem\n");
+	pr_debug("swsusp: Saving Highmem");
 	for_each_zone (zone) {
 		if (is_highmem(zone))
 			res = save_highmem_zone(zone);
 		if (res)
 			return res;
 	}
+	printk("\n");
 	return 0;
 }
 

-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 2322 bytes --]

Hi.

On Wednesday 01 February 2006 21:47, Pavel Machek wrote:
> Hi!
>
> > This is an experimantal patch aimed at the "unable to freeze processes
> > under load" problem.
> >
> > On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
> > "dd if=/dev/hda of=/dev/null" test.
> >
> > Please have a look.
>
> It makes it better (well, I used my own, simpler variant, but that
> should not matter; patch is attached). I now can't reproduce hangs
> with simple stress testing, but running kernel make alongside that
> makes it hang sometimes. Example of non-frozen gcc:
>
> gcc           D EEE06A70     0  1750   1749  1751
> (NOTLB)
> df85df38 00000046 bf878130 eee06a70 00004111 eee06a70 eee06a70
> 003d0900
>        00000000 c0137cf5 df85c000 00000000 c058ada2 c012503e ef2c915c
> ef2c9030
>        c1c0b480 7c3b8500 003d0927 df85c000 00000a98 7c3b8500 003d0927
> c0770800
> Call Trace:
>  [<c0137cf5>] attach_pid+0x25/0xb0
>  [<c058ada2>] _write_unlock_irq+0x12/0x30
>  [<c012503e>] copy_process+0xe5e/0x11b0
>  [<c0588f74>] wait_for_completion+0x94/0xd0
>  [<c0121690>] default_wake_function+0x0/0x10
>  [<c01254d9>] do_fork+0x149/0x210
>  [<c0101218>] sys_vfork+0x28/0x30
>  [<c0103231>] syscall_call+0x7/0xb
>
> ...maybe solving this would solve journalling problems, too? It is
> similar AFAICT.

What exactly is the journalling problem?

>
> diff --git a/kernel/power/disk.c b/kernel/power/disk.c
> index e24446f..90d6c1a 100644
> --- a/kernel/power/disk.c
> +++ b/kernel/power/disk.c
> @@ -87,7 +87,6 @@ static int prepare_processes(void)
>  	int error;
>
>  	pm_prepare_console();
> -	sys_sync();
>  	disable_nonboot_cpus();
>
>  	if (freeze_processes()) {

That will help speed up freezing, but it won't help the integrity of your data 
if you don't resume.

> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 02a1b3a..bd16e44 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -16,7 +16,7 @@
>  /*
>   * Timeout for stopping processes
>   */
> -#define TIMEOUT	(6 * HZ)
> +#define TIMEOUT	(60 * HZ)

You're kidding, right?

Regards,

Nigel
-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]

Hi!

> > > This is an experimantal patch aimed at the "unable to freeze processes
> > > under load" problem.
> > >
> > > On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
> > > "dd if=/dev/hda of=/dev/null" test.
> > >
> > > Please have a look.
> >
> > It makes it better (well, I used my own, simpler variant, but that
> > should not matter; patch is attached). I now can't reproduce hangs
> > with simple stress testing, but running kernel make alongside that
> > makes it hang sometimes. Example of non-frozen gcc:
> >
> > gcc           D EEE06A70     0  1750   1749  1751
> > (NOTLB)
> > df85df38 00000046 bf878130 eee06a70 00004111 eee06a70 eee06a70
> > 003d0900
> >        00000000 c0137cf5 df85c000 00000000 c058ada2 c012503e ef2c915c
> > ef2c9030
> >        c1c0b480 7c3b8500 003d0927 df85c000 00000a98 7c3b8500 003d0927
> > c0770800
> > Call Trace:
> >  [<c0137cf5>] attach_pid+0x25/0xb0
> >  [<c058ada2>] _write_unlock_irq+0x12/0x30
> >  [<c012503e>] copy_process+0xe5e/0x11b0
> >  [<c0588f74>] wait_for_completion+0x94/0xd0
> >  [<c0121690>] default_wake_function+0x0/0x10
> >  [<c01254d9>] do_fork+0x149/0x210
> >  [<c0101218>] sys_vfork+0x28/0x30
> >  [<c0103231>] syscall_call+0x7/0xb
> >
> > ...maybe solving this would solve journalling problems, too? It is
> > similar AFAICT.
> 
> What exactly is the journalling problem?

Hangs by freezing everything at same time only happen with journalling
filesystems; there kjournald needs to be running if we want user
threads to be stoppable.

> > @@ -87,7 +87,6 @@ static int prepare_processes(void)
> >  	int error;
> >
> >  	pm_prepare_console();
> > -	sys_sync();
> >  	disable_nonboot_cpus();
> >
> >  	if (freeze_processes()) {
> 
> That will help speed up freezing, but it won't help the integrity of your data 
> if you don't resume.

See the patch better; it is now done between freezing userspace and
kernel threads.

> >  /*
> >   * Timeout for stopping processes
> >   */
> > -#define TIMEOUT	(6 * HZ)
> > +#define TIMEOUT	(60 * HZ)
> 
> You're kidding, right?

sync takes long time... and 6 seconds were not enough to deliver
signals on highly-loaded ext2.
							Pavel

-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 3114 bytes --]

Hi.

On Wednesday 01 February 2006 22:49, Pavel Machek wrote:
> Hi!
>
> > > > This is an experimantal patch aimed at the "unable to freeze
> > > > processes under load" problem.
> > > >
> > > > On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives
> > > > the "dd if=/dev/hda of=/dev/null" test.
> > > >
> > > > Please have a look.
> > >
> > > It makes it better (well, I used my own, simpler variant, but that
> > > should not matter; patch is attached). I now can't reproduce hangs
> > > with simple stress testing, but running kernel make alongside that
> > > makes it hang sometimes. Example of non-frozen gcc:
> > >
> > > gcc           D EEE06A70     0  1750   1749  1751
> > > (NOTLB)
> > > df85df38 00000046 bf878130 eee06a70 00004111 eee06a70 eee06a70
> > > 003d0900
> > >        00000000 c0137cf5 df85c000 00000000 c058ada2 c012503e ef2c915c
> > > ef2c9030
> > >        c1c0b480 7c3b8500 003d0927 df85c000 00000a98 7c3b8500 003d0927
> > > c0770800
> > > Call Trace:
> > >  [<c0137cf5>] attach_pid+0x25/0xb0
> > >  [<c058ada2>] _write_unlock_irq+0x12/0x30
> > >  [<c012503e>] copy_process+0xe5e/0x11b0
> > >  [<c0588f74>] wait_for_completion+0x94/0xd0
> > >  [<c0121690>] default_wake_function+0x0/0x10
> > >  [<c01254d9>] do_fork+0x149/0x210
> > >  [<c0101218>] sys_vfork+0x28/0x30
> > >  [<c0103231>] syscall_call+0x7/0xb
> > >
> > > ...maybe solving this would solve journalling problems, too? It is
> > > similar AFAICT.
> >
> > What exactly is the journalling problem?
>
> Hangs by freezing everything at same time only happen with journalling
> filesystems; there kjournald needs to be running if we want user
> threads to be stoppable.

Ah. Right. That should be fixed by doing the kernelspace threads after the 
usespace ones (as I believe you're now doing?). I have seen XFS still 
submitting I/O after the sys_sync is finished (it apparently treats sys_sync 
as a weak and useless indication that it should think about considering 
flushing a buffer or two). That's why I'm now using bdev freezing instead of 
sys_sync.

> > > @@ -87,7 +87,6 @@ static int prepare_processes(void)
> > >  	int error;
> > >
> > >  	pm_prepare_console();
> > > -	sys_sync();
> > >  	disable_nonboot_cpus();
> > >
> > >  	if (freeze_processes()) {
> >
> > That will help speed up freezing, but it won't help the integrity of your
> > data if you don't resume.
>
> See the patch better; it is now done between freezing userspace and
> kernel threads.

Ah. So I see. Sorry.

> > >  /*
> > >   * Timeout for stopping processes
> > >   */
> > > -#define TIMEOUT	(6 * HZ)
> > > +#define TIMEOUT	(60 * HZ)
> >
> > You're kidding, right?
>
> sync takes long time... and 6 seconds were not enough to deliver
> signals on highly-loaded ext2.

I'm using a timeout per step. Perhaps you could try that approach? 1 minute is 
an awfully long time to wait if you do hang.

Regards,

Nigel
-- 
See our web page for Howtos, FAQs, the Wiki and mailing list info.
http://www.suspend2.net                IRC: #suspend2 on Freenode

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Rafael J. Wysocki]

Hi,

On Wednesday 01 February 2006 13:49, Pavel Machek wrote:
> > > > This is an experimantal patch aimed at the "unable to freeze processes
> > > > under load" problem.
> > > >
}-- snip --{
> > >  /*
> > >   * Timeout for stopping processes
> > >   */
> > > -#define TIMEOUT	(6 * HZ)
> > > +#define TIMEOUT	(60 * HZ)
> > 
> > You're kidding, right?
> 
> sync takes long time... and 6 seconds were not enough to deliver
> signals on highly-loaded ext2.

The appended modified version of the original patch solves the timeout vs sync
problem. ;-)

Seriously speaking I have incoroprated your changes (extended to the userland
interface) and made some fixes (the usermodehelper-related part is still
there, for completness).

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 include/linux/suspend.h |    6 +++
 kernel/kmod.c           |   19 ++++++++++++
 kernel/power/disk.c     |    1 
 kernel/power/process.c  |   74 ++++++++++++++++++++++++++++++++++++++----------
 kernel/power/user.c     |    1 
 5 files changed, 85 insertions(+), 16 deletions(-)

Index: linux-2.6.16-rc1-mm4/kernel/power/process.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/process.c
+++ linux-2.6.16-rc1-mm4/kernel/power/process.c
@@ -12,12 +12,20 @@
 #include <linux/interrupt.h>
 #include <linux/suspend.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/syscalls.h>
 
 /* 
  * Timeout for stopping processes
  */
 #define TIMEOUT	(6 * HZ)
 
+/* This is used to disable usermodehelper invocations while
+ * freeze_processes() is being executed
+ */
+DEFINE_MUTEX(freezer_lock);
+int freezing_processes;
+atomic_t usermodehelper_waiting = ATOMIC_INIT(0);
 
 static inline int freezeable(struct task_struct * p)
 {
@@ -54,48 +62,86 @@ void refrigerator(void)
 	current->state = save;
 }
 
+static inline void freeze_process(struct task_struct *p)
+{
+	unsigned long flags;
+
+	freeze(p);
+	spin_lock_irqsave(&p->sighand->siglock, flags);
+	signal_wake_up(p, 0);
+	spin_unlock_irqrestore(&p->sighand->siglock, flags);
+}
+
 /* 0 = success, else # of processes that we failed to stop */
 int freeze_processes(void)
 {
-	int todo;
+	int todo, nr_user, user_frozen;
 	unsigned long start_time;
 	struct task_struct *g, *p;
 	unsigned long flags;
 
+	mutex_lock(&freezer_lock);
+	freezing_processes = 1;
+	mutex_unlock(&freezer_lock);
+	while (atomic_read(&usermodehelper_waiting))
+		schedule();
+
 	printk( "Stopping tasks: " );
 	start_time = jiffies;
+	user_frozen = 0;
 	do {
-		todo = 0;
+		nr_user = todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			if (!freezeable(p))
 				continue;
 			if (frozen(p))
 				continue;
-
-			freeze(p);
-			spin_lock_irqsave(&p->sighand->siglock, flags);
-			signal_wake_up(p, 0);
-			spin_unlock_irqrestore(&p->sighand->siglock, flags);
-			todo++;
+			if (p->mm && !(p->flags & PF_BORROWED_MM)) {
+				/* The task is a user-space one.
+				 * Freeze it unless there's a vfork completion
+				 * pending
+				 */
+				if (!p->vfork_done)
+					freeze_process(p);
+				nr_user++;
+			} else {
+				/* Freeze only if the user space is frozen */
+				if (user_frozen)
+					freeze_process(p);
+				todo++;
+			}
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
+		todo += nr_user;
+		if (!user_frozen && !nr_user) {
+			sys_sync();
+			start_time = jiffies;
+		}
+		user_frozen = !nr_user;
 		yield();			/* Yield is okay here */
-		if (todo && time_after(jiffies, start_time + TIMEOUT)) {
-			printk( "\n" );
-			printk(KERN_ERR " stopping tasks timed out (%d tasks remaining)\n", todo );
+		if (todo && time_after(jiffies, start_time + TIMEOUT))
 			break;
-		}
 	} while(todo);
 
+	mutex_lock(&freezer_lock);
+	freezing_processes = 0;
+	mutex_unlock(&freezer_lock);
+
 	/* This does not unfreeze processes that are already frozen
 	 * (we have slightly ugly calling convention in that respect,
 	 * and caller must call thaw_processes() if something fails),
 	 * but it cleans up leftover PF_FREEZE requests.
 	 */
 	if (todo) {
+		printk( "\n" );
+		printk(KERN_ERR " stopping tasks timed out "
+			"after %d seconds (%d tasks remaining):\n",
+			TIMEOUT / HZ, todo);
 		read_lock(&tasklist_lock);
-		do_each_thread(g, p)
+		do_each_thread(g, p) {
+			if (freezeable(p) && !frozen(p))
+				printk(KERN_ERR "  %s\n", p->comm);
 			if (freezing(p)) {
 				pr_debug("  clean up: %s\n", p->comm);
 				p->flags &= ~PF_FREEZE;
@@ -103,7 +149,7 @@ int freeze_processes(void)
 				recalc_sigpending_tsk(p);
 				spin_unlock_irqrestore(&p->sighand->siglock, flags);
 			}
-		while_each_thread(g, p);
+		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 		return todo;
 	}
Index: linux-2.6.16-rc1-mm4/kernel/kmod.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/kmod.c
+++ linux-2.6.16-rc1-mm4/kernel/kmod.c
@@ -36,6 +36,8 @@
 #include <linux/mount.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/suspend.h>
+#include <linux/mutex.h>
 #include <asm/uaccess.h>
 
 extern int max_threads;
@@ -249,11 +251,28 @@ int call_usermodehelper_keys(char *path,
 	if (!khelper_wq)
 		return -EBUSY;
 
+#ifdef CONFIG_PM
+	mutex_lock(&freezer_lock);
+	if (freezing_processes) {
+		mutex_unlock(&freezer_lock);
+		return -EBUSY;
+	}
+	if (wait)
+		atomic_inc(&usermodehelper_waiting);
+	mutex_unlock(&freezer_lock);
+#endif
+
 	if (path[0] == '\0')
 		return 0;
 
 	queue_work(khelper_wq, &work);
 	wait_for_completion(&done);
+
+#ifdef CONFIG_PM
+	if (wait)
+		atomic_dec(&usermodehelper_waiting);
+#endif
+
 	return sub_info.retval;
 }
 EXPORT_SYMBOL(call_usermodehelper_keys);
Index: linux-2.6.16-rc1-mm4/include/linux/suspend.h
===================================================================
--- linux-2.6.16-rc1-mm4.orig/include/linux/suspend.h
+++ linux-2.6.16-rc1-mm4/include/linux/suspend.h
@@ -9,6 +9,7 @@
 #include <linux/config.h>
 #include <linux/init.h>
 #include <linux/pm.h>
+#include <asm/atomic.h>
 
 /* page backup entry */
 typedef struct pbe {
@@ -40,6 +41,11 @@ extern void drain_local_pages(void);
 extern void mark_free_pages(struct zone *zone);
 
 #ifdef CONFIG_PM
+struct mutex;
+extern struct mutex freezer_lock;
+extern int freezing_processes;
+extern atomic_t usermodehelper_waiting;
+
 /* kernel/power/swsusp.c */
 extern int software_suspend(void);
 
Index: linux-2.6.16-rc1-mm4/kernel/power/disk.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/disk.c
+++ linux-2.6.16-rc1-mm4/kernel/power/disk.c
@@ -73,7 +73,6 @@ static int prepare_processes(void)
 	int error;
 
 	pm_prepare_console();
-	sys_sync();
 	disable_nonboot_cpus();
 
 	if (freeze_processes()) {
Index: linux-2.6.16-rc1-mm4/kernel/power/user.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/user.c
+++ linux-2.6.16-rc1-mm4/kernel/power/user.c
@@ -137,7 +137,6 @@ static int snapshot_ioctl(struct inode *
 	case SNAPSHOT_FREEZE:
 		if (data->frozen)
 			break;
-		sys_sync();
 		down(&pm_sem);
 		pm_prepare_console();
 		disable_nonboot_cpus();

Re: Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Rafael J. Wysocki]

Hi,

On Thursday 02 February 2006 00:57, Rafael J. Wysocki wrote:
> On Wednesday 01 February 2006 13:49, Pavel Machek wrote:
> > > > > This is an experimantal patch aimed at the "unable to freeze processes
> > > > > under load" problem.
> > > > >
> }-- snip --{
> > > >  /*
> > > >   * Timeout for stopping processes
> > > >   */
> > > > -#define TIMEOUT	(6 * HZ)
> > > > +#define TIMEOUT	(60 * HZ)
> > > 
> > > You're kidding, right?
> > 
> > sync takes long time... and 6 seconds were not enough to deliver
> > signals on highly-loaded ext2.
> 
> The appended modified version of the original patch solves the timeout vs sync
> problem. ;-)
> 
> Seriously speaking I have incoroprated your changes (extended to the userland
> interface) and made some fixes (the usermodehelper-related part is still
> there, for completness).
}-- snip --{  
> +	mutex_lock(&freezer_lock);
> +	freezing_processes = 1;
> +	mutex_unlock(&freezer_lock);
> +	while (atomic_read(&usermodehelper_waiting))
> +		schedule();

That requires a timeout in case we have a user mode helper in the D state.
The corrected patch is appended.

BTW, it contains a change that may help solve the unfreezeable gcc problem
that has appeared in your tests.  Could you please try it or tell me what I
should do to reproduce the problem?

Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 include/linux/suspend.h |    6 +++
 kernel/kmod.c           |   19 +++++++++++
 kernel/power/disk.c     |    1 
 kernel/power/process.c  |   81 +++++++++++++++++++++++++++++++++++++++---------
 kernel/power/user.c     |    1 
 5 files changed, 92 insertions(+), 16 deletions(-)

Index: linux-2.6.16-rc1-mm4/kernel/power/process.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/process.c
+++ linux-2.6.16-rc1-mm4/kernel/power/process.c
@@ -12,12 +12,20 @@
 #include <linux/interrupt.h>
 #include <linux/suspend.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/syscalls.h>
 
 /* 
  * Timeout for stopping processes
  */
 #define TIMEOUT	(6 * HZ)
 
+/* This is used to disable usermodehelper invocations while
+ * freeze_processes() is being executed
+ */
+DEFINE_MUTEX(freezer_lock);
+int freezing_processes;
+atomic_t usermodehelper_waiting = ATOMIC_INIT(0);
 
 static inline int freezeable(struct task_struct * p)
 {
@@ -54,48 +62,93 @@ void refrigerator(void)
 	current->state = save;
 }
 
+static inline void freeze_process(struct task_struct *p)
+{
+	unsigned long flags;
+
+	freeze(p);
+	spin_lock_irqsave(&p->sighand->siglock, flags);
+	signal_wake_up(p, 0);
+	spin_unlock_irqrestore(&p->sighand->siglock, flags);
+}
+
 /* 0 = success, else # of processes that we failed to stop */
 int freeze_processes(void)
 {
-	int todo;
+	int todo, nr_user, user_frozen;
 	unsigned long start_time;
 	struct task_struct *g, *p;
 	unsigned long flags;
 
+	start_time = jiffies;
+	mutex_lock(&freezer_lock);
+	freezing_processes = 1;
+	mutex_unlock(&freezer_lock);
+	while (atomic_read(&usermodehelper_waiting)) {
+		if (time_after(jiffies, start_time + TIMEOUT)) {
+			printk(KERN_ERR "Unable to freeze tasks because of "
+				"active user mode helpers\n");
+			return -EBUSY;
+		}
+		schedule();
+	}
+
 	printk( "Stopping tasks: " );
 	start_time = jiffies;
+	user_frozen = 0;
 	do {
-		todo = 0;
+		nr_user = todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			if (!freezeable(p))
 				continue;
 			if (frozen(p))
 				continue;
-
-			freeze(p);
-			spin_lock_irqsave(&p->sighand->siglock, flags);
-			signal_wake_up(p, 0);
-			spin_unlock_irqrestore(&p->sighand->siglock, flags);
-			todo++;
+			if (p->mm && !(p->flags & PF_BORROWED_MM)) {
+				/* The task is a user-space one.
+				 * Freeze it unless there's a vfork completion
+				 * pending
+				 */
+				if (!p->vfork_done)
+					freeze_process(p);
+				nr_user++;
+			} else {
+				/* Freeze only if the user space is frozen */
+				if (user_frozen)
+					freeze_process(p);
+				todo++;
+			}
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
+		todo += nr_user;
+		if (!user_frozen && !nr_user) {
+			sys_sync();
+			start_time = jiffies;
+		}
+		user_frozen = !nr_user;
 		yield();			/* Yield is okay here */
-		if (todo && time_after(jiffies, start_time + TIMEOUT)) {
-			printk( "\n" );
-			printk(KERN_ERR " stopping tasks timed out (%d tasks remaining)\n", todo );
+		if (todo && time_after(jiffies, start_time + TIMEOUT))
 			break;
-		}
 	} while(todo);
 
+	mutex_lock(&freezer_lock);
+	freezing_processes = 0;
+	mutex_unlock(&freezer_lock);
+
 	/* This does not unfreeze processes that are already frozen
 	 * (we have slightly ugly calling convention in that respect,
 	 * and caller must call thaw_processes() if something fails),
 	 * but it cleans up leftover PF_FREEZE requests.
 	 */
 	if (todo) {
+		printk( "\n" );
+		printk(KERN_ERR " stopping tasks timed out "
+			"after %d seconds (%d tasks remaining):\n",
+			TIMEOUT / HZ, todo);
 		read_lock(&tasklist_lock);
-		do_each_thread(g, p)
+		do_each_thread(g, p) {
+			if (freezeable(p) && !frozen(p))
+				printk(KERN_ERR "  %s\n", p->comm);
 			if (freezing(p)) {
 				pr_debug("  clean up: %s\n", p->comm);
 				p->flags &= ~PF_FREEZE;
@@ -103,7 +156,7 @@ int freeze_processes(void)
 				recalc_sigpending_tsk(p);
 				spin_unlock_irqrestore(&p->sighand->siglock, flags);
 			}
-		while_each_thread(g, p);
+		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 		return todo;
 	}
Index: linux-2.6.16-rc1-mm4/kernel/kmod.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/kmod.c
+++ linux-2.6.16-rc1-mm4/kernel/kmod.c
@@ -36,6 +36,8 @@
 #include <linux/mount.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/suspend.h>
+#include <linux/mutex.h>
 #include <asm/uaccess.h>
 
 extern int max_threads;
@@ -249,11 +251,28 @@ int call_usermodehelper_keys(char *path,
 	if (!khelper_wq)
 		return -EBUSY;
 
+#ifdef CONFIG_PM
+	mutex_lock(&freezer_lock);
+	if (freezing_processes) {
+		mutex_unlock(&freezer_lock);
+		return -EBUSY;
+	}
+	if (wait)
+		atomic_inc(&usermodehelper_waiting);
+	mutex_unlock(&freezer_lock);
+#endif
+
 	if (path[0] == '\0')
 		return 0;
 
 	queue_work(khelper_wq, &work);
 	wait_for_completion(&done);
+
+#ifdef CONFIG_PM
+	if (wait)
+		atomic_dec(&usermodehelper_waiting);
+#endif
+
 	return sub_info.retval;
 }
 EXPORT_SYMBOL(call_usermodehelper_keys);
Index: linux-2.6.16-rc1-mm4/include/linux/suspend.h
===================================================================
--- linux-2.6.16-rc1-mm4.orig/include/linux/suspend.h
+++ linux-2.6.16-rc1-mm4/include/linux/suspend.h
@@ -9,6 +9,7 @@
 #include <linux/config.h>
 #include <linux/init.h>
 #include <linux/pm.h>
+#include <asm/atomic.h>
 
 /* page backup entry */
 typedef struct pbe {
@@ -40,6 +41,11 @@ extern void drain_local_pages(void);
 extern void mark_free_pages(struct zone *zone);
 
 #ifdef CONFIG_PM
+struct mutex;
+extern struct mutex freezer_lock;
+extern int freezing_processes;
+extern atomic_t usermodehelper_waiting;
+
 /* kernel/power/swsusp.c */
 extern int software_suspend(void);
 
Index: linux-2.6.16-rc1-mm4/kernel/power/disk.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/disk.c
+++ linux-2.6.16-rc1-mm4/kernel/power/disk.c
@@ -73,7 +73,6 @@ static int prepare_processes(void)
 	int error;
 
 	pm_prepare_console();
-	sys_sync();
 	disable_nonboot_cpus();
 
 	if (freeze_processes()) {
Index: linux-2.6.16-rc1-mm4/kernel/power/user.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/user.c
+++ linux-2.6.16-rc1-mm4/kernel/power/user.c
@@ -137,7 +137,6 @@ static int snapshot_ioctl(struct inode *
 	case SNAPSHOT_FREEZE:
 		if (data->frozen)
 			break;
-		sys_sync();
 		down(&pm_sem);
 		pm_prepare_console();
 		disable_nonboot_cpus();

Re: Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

Hi!

> That requires a timeout in case we have a user mode helper in the D state.
> The corrected patch is appended.
> 
> BTW, it contains a change that may help solve the unfreezeable gcc problem
> that has appeared in your tests.  Could you please try it or tell me what I
> should do to reproduce the problem?

I'm away from real macine just now... I could reproduce it with
Nigel's "stress ..." command, then trying to build kernel.

								Pavel

-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Rafael J. Wysocki]

Hi,

On Thursday 02 February 2006 16:08, Pavel Machek wrote:
> > That requires a timeout in case we have a user mode helper in the D state.
> > The corrected patch is appended.
> > 
> > BTW, it contains a change that may help solve the unfreezeable gcc problem
> > that has appeared in your tests.  Could you please try it or tell me what I
> > should do to reproduce the problem?
> 
> I'm away from real macine just now... I could reproduce it with
> Nigel's "stress ..." command, then trying to build kernel.

OK, I did the following:
1) run "swapoff -a"
2) run kernel make on one vt,
3) run "stress -d 5 --hdd-bytes 100M -i 5 -c 5" on another vt,
4) run "for f in 1 2 3 4 5 6 7 8 9 10; do echo disk > /sys/power/state ; sleep 5; done" on the 3rd vt.

Appended is the version of the patch that has freezed processes in 10 attempts
out of 10 (please note the "if (!freezing(p))" in freeze_process() ;-)).

Still freezing the userspace processes may take more that 15 secs under such
a load on my box, so the timeout is set to 20 sec (probably overkill for any
sane real-life situation).

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 include/linux/suspend.h |    6 +++
 kernel/kmod.c           |   19 ++++++++++
 kernel/power/disk.c     |    1 
 kernel/power/process.c  |   85 +++++++++++++++++++++++++++++++++++++++---------
 kernel/power/user.c     |    1 
 5 files changed, 95 insertions(+), 17 deletions(-)

Index: linux-2.6.16-rc1-mm4/kernel/power/process.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/process.c
+++ linux-2.6.16-rc1-mm4/kernel/power/process.c
@@ -12,12 +12,20 @@
 #include <linux/interrupt.h>
 #include <linux/suspend.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/syscalls.h>
 
 /* 
  * Timeout for stopping processes
  */
-#define TIMEOUT	(6 * HZ)
+#define TIMEOUT	(20 * HZ)
 
+/* This is used to disable usermodehelper invocations while
+ * freeze_processes() is being executed
+ */
+DEFINE_MUTEX(freezer_lock);
+int freezing_processes;
+atomic_t usermodehelper_waiting = ATOMIC_INIT(0);
 
 static inline int freezeable(struct task_struct * p)
 {
@@ -54,48 +62,95 @@ void refrigerator(void)
 	current->state = save;
 }
 
+static inline void freeze_process(struct task_struct *p)
+{
+	unsigned long flags;
+
+	if (!freezing(p)) {
+		freeze(p);
+		spin_lock_irqsave(&p->sighand->siglock, flags);
+		signal_wake_up(p, 0);
+		spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	}
+}
+
 /* 0 = success, else # of processes that we failed to stop */
 int freeze_processes(void)
 {
-	int todo;
+	int todo, nr_user, user_frozen;
 	unsigned long start_time;
 	struct task_struct *g, *p;
 	unsigned long flags;
 
+	start_time = jiffies;
+	mutex_lock(&freezer_lock);
+	freezing_processes = 1;
+	mutex_unlock(&freezer_lock);
+	while (atomic_read(&usermodehelper_waiting)) {
+		if (time_after(jiffies, start_time + TIMEOUT)) {
+			printk(KERN_ERR "Unable to freeze tasks because of "
+				"active user mode helpers\n");
+			return -EBUSY;
+		}
+		schedule();
+	}
+
 	printk( "Stopping tasks: " );
 	start_time = jiffies;
+	user_frozen = 0;
 	do {
-		todo = 0;
+		nr_user = todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			if (!freezeable(p))
 				continue;
 			if (frozen(p))
 				continue;
-
-			freeze(p);
-			spin_lock_irqsave(&p->sighand->siglock, flags);
-			signal_wake_up(p, 0);
-			spin_unlock_irqrestore(&p->sighand->siglock, flags);
-			todo++;
+			if (p->mm && !(p->flags & PF_BORROWED_MM)) {
+				/* The task is a user-space one.
+				 * Freeze it unless there's a vfork completion
+				 * pending
+				 */
+				if (!p->vfork_done)
+					freeze_process(p);
+				nr_user++;
+			} else {
+				/* Freeze only if the user space is frozen */
+				if (user_frozen)
+					freeze_process(p);
+				todo++;
+			}
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
+		todo += nr_user;
+		if (!user_frozen && !nr_user) {
+			sys_sync();
+			start_time = jiffies;
+		}
+		user_frozen = !nr_user;
 		yield();			/* Yield is okay here */
-		if (todo && time_after(jiffies, start_time + TIMEOUT)) {
-			printk( "\n" );
-			printk(KERN_ERR " stopping tasks timed out (%d tasks remaining)\n", todo );
+		if (todo && time_after(jiffies, start_time + TIMEOUT))
 			break;
-		}
 	} while(todo);
 
+	mutex_lock(&freezer_lock);
+	freezing_processes = 0;
+	mutex_unlock(&freezer_lock);
+
 	/* This does not unfreeze processes that are already frozen
 	 * (we have slightly ugly calling convention in that respect,
 	 * and caller must call thaw_processes() if something fails),
 	 * but it cleans up leftover PF_FREEZE requests.
 	 */
 	if (todo) {
+		printk( "\n" );
+		printk(KERN_ERR " stopping tasks timed out "
+			"after %d seconds (%d tasks remaining):\n",
+			TIMEOUT / HZ, todo);
 		read_lock(&tasklist_lock);
-		do_each_thread(g, p)
+		do_each_thread(g, p) {
+			if (freezeable(p) && !frozen(p))
+				printk(KERN_ERR "  %s\n", p->comm);
 			if (freezing(p)) {
 				pr_debug("  clean up: %s\n", p->comm);
 				p->flags &= ~PF_FREEZE;
@@ -103,7 +158,7 @@ int freeze_processes(void)
 				recalc_sigpending_tsk(p);
 				spin_unlock_irqrestore(&p->sighand->siglock, flags);
 			}
-		while_each_thread(g, p);
+		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 		return todo;
 	}
Index: linux-2.6.16-rc1-mm4/kernel/kmod.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/kmod.c
+++ linux-2.6.16-rc1-mm4/kernel/kmod.c
@@ -36,6 +36,8 @@
 #include <linux/mount.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/suspend.h>
+#include <linux/mutex.h>
 #include <asm/uaccess.h>
 
 extern int max_threads;
@@ -249,11 +251,28 @@ int call_usermodehelper_keys(char *path,
 	if (!khelper_wq)
 		return -EBUSY;
 
+#ifdef CONFIG_PM
+	mutex_lock(&freezer_lock);
+	if (freezing_processes) {
+		mutex_unlock(&freezer_lock);
+		return -EBUSY;
+	}
+	if (wait)
+		atomic_inc(&usermodehelper_waiting);
+	mutex_unlock(&freezer_lock);
+#endif
+
 	if (path[0] == '\0')
 		return 0;
 
 	queue_work(khelper_wq, &work);
 	wait_for_completion(&done);
+
+#ifdef CONFIG_PM
+	if (wait)
+		atomic_dec(&usermodehelper_waiting);
+#endif
+
 	return sub_info.retval;
 }
 EXPORT_SYMBOL(call_usermodehelper_keys);
Index: linux-2.6.16-rc1-mm4/include/linux/suspend.h
===================================================================
--- linux-2.6.16-rc1-mm4.orig/include/linux/suspend.h
+++ linux-2.6.16-rc1-mm4/include/linux/suspend.h
@@ -9,6 +9,7 @@
 #include <linux/config.h>
 #include <linux/init.h>
 #include <linux/pm.h>
+#include <asm/atomic.h>
 
 /* page backup entry */
 typedef struct pbe {
@@ -40,6 +41,11 @@ extern void drain_local_pages(void);
 extern void mark_free_pages(struct zone *zone);
 
 #ifdef CONFIG_PM
+struct mutex;
+extern struct mutex freezer_lock;
+extern int freezing_processes;
+extern atomic_t usermodehelper_waiting;
+
 /* kernel/power/swsusp.c */
 extern int software_suspend(void);
 
Index: linux-2.6.16-rc1-mm4/kernel/power/disk.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/disk.c
+++ linux-2.6.16-rc1-mm4/kernel/power/disk.c
@@ -73,7 +73,6 @@ static int prepare_processes(void)
 	int error;
 
 	pm_prepare_console();
-	sys_sync();
 	disable_nonboot_cpus();
 
 	if (freeze_processes()) {
Index: linux-2.6.16-rc1-mm4/kernel/power/user.c
===================================================================
--- linux-2.6.16-rc1-mm4.orig/kernel/power/user.c
+++ linux-2.6.16-rc1-mm4/kernel/power/user.c
@@ -137,7 +137,6 @@ static int snapshot_ioctl(struct inode *
 	case SNAPSHOT_FREEZE:
 		if (data->frozen)
 			break;
-		sys_sync();
 		down(&pm_sem);
 		pm_prepare_console();
 		disable_nonboot_cpus();

Re: Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

Hi!

> > > That requires a timeout in case we have a user mode helper in the D state.
> > > The corrected patch is appended.
> > > 
> > > BTW, it contains a change that may help solve the unfreezeable gcc problem
> > > that has appeared in your tests.  Could you please try it or tell me what I
> > > should do to reproduce the problem?
> > 
> > I'm away from real macine just now... I could reproduce it with
> > Nigel's "stress ..." command, then trying to build kernel.
> 
> OK, I did the following:
> 1) run "swapoff -a"
> 2) run kernel make on one vt,
> 3) run "stress -d 5 --hdd-bytes 100M -i 5 -c 5" on another vt,
> 4) run "for f in 1 2 3 4 5 6 7 8 9 10; do echo disk > /sys/power/state ; sleep 5; done" on the 3rd vt.
> 
> Appended is the version of the patch that has freezed processes in 10 attempts
> out of 10 (please note the "if (!freezing(p))" in freeze_process() ;-)).
> 
> Still freezing the userspace processes may take more that 15 secs under such
> a load on my box, so the timeout is set to 20 sec (probably overkill for any
> sane real-life situation).

You have my ACK on freezer parts, but please reserve usermode helper
parts for separate patch. Is there simple way to demonstrate usermode
helper problem?
								Pavel
-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Rafael J. Wysocki]

Hi,

On Saturday 04 February 2006 22:26, Pavel Machek wrote:
> > > > That requires a timeout in case we have a user mode helper in the D state.
> > > > The corrected patch is appended.
> > > > 
> > > > BTW, it contains a change that may help solve the unfreezeable gcc problem
> > > > that has appeared in your tests.  Could you please try it or tell me what I
> > > > should do to reproduce the problem?
> > > 
> > > I'm away from real macine just now... I could reproduce it with
> > > Nigel's "stress ..." command, then trying to build kernel.
> > 
> > OK, I did the following:
> > 1) run "swapoff -a"
> > 2) run kernel make on one vt,
> > 3) run "stress -d 5 --hdd-bytes 100M -i 5 -c 5" on another vt,
> > 4) run "for f in 1 2 3 4 5 6 7 8 9 10; do echo disk > /sys/power/state ; sleep 5; done" on the 3rd vt.
> > 
> > Appended is the version of the patch that has freezed processes in 10 attempts
> > out of 10 (please note the "if (!freezing(p))" in freeze_process() ;-)).
> > 
> > Still freezing the userspace processes may take more that 15 secs under such
> > a load on my box, so the timeout is set to 20 sec (probably overkill for any
> > sane real-life situation).
> 
> You have my ACK on freezer parts, but please reserve usermode helper
> parts for separate patch.

OK, I'll remove the usermodehelper-related part for now.  If we have a test
case, I'll think of it again.

> Is there simple way to demonstrate usermode helper problem?

Not that I know of.  Probably I am overly paranoid wrt that.

Greetings,
Rafael

Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

[Rafael J. Wysocki]

Hi,

On Wednesday 01 February 2006 12:47, Pavel Machek wrote:
> > This is an experimantal patch aimed at the "unable to freeze processes under
> > load" problem.
> > 
> > On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
> > "dd if=/dev/hda of=/dev/null" test.
> > 
> > Please have a look.
> 
> It makes it better (well, I used my own, simpler variant, but that
> should not matter; patch is attached). I now can't reproduce hangs
> with simple stress testing,

This means the direction is right ...

> but running kernel make alongside that 
> makes it hang sometimes.

... but there still is a race here.

> Example of non-frozen gcc: 
> 
> gcc           D EEE06A70     0  1750   1749  1751
> (NOTLB)
> df85df38 00000046 bf878130 eee06a70 00004111 eee06a70 eee06a70
> 003d0900
>        00000000 c0137cf5 df85c000 00000000 c058ada2 c012503e ef2c915c
> ef2c9030
>        c1c0b480 7c3b8500 003d0927 df85c000 00000a98 7c3b8500 003d0927
> c0770800
> Call Trace:
>  [<c0137cf5>] attach_pid+0x25/0xb0
>  [<c058ada2>] _write_unlock_irq+0x12/0x30
>  [<c012503e>] copy_process+0xe5e/0x11b0
>  [<c0588f74>] wait_for_completion+0x94/0xd0
>  [<c0121690>] default_wake_function+0x0/0x10
>  [<c01254d9>] do_fork+0x149/0x210
>  [<c0101218>] sys_vfork+0x28/0x30
>  [<c0103231>] syscall_call+0x7/0xb

[I'm trying to understand this trace, but with not much success, so far.
Apparently sys_vfork() calls do_fork() which calls copy_process(),
which calls attach_pid().  But what default_wake_function() and
wait_for_completion() are doing here?  And where it got stuck, actually?]

I think the problem is related to the kernel make using fork() heavily.
Namely, if the parent process uses CLONE_VFORK, it will wait for the
vfork completion.  Now if the child process is frozen _before_
it completes the vfork completion, the parent will be unfreezeable.

> ...maybe solving this would solve journalling problems, too? It is
> similar AFAICT.

There's a chance, I think. ;-)

> diff --git a/kernel/power/disk.c b/kernel/power/disk.c
> index e24446f..90d6c1a 100644
> --- a/kernel/power/disk.c
> +++ b/kernel/power/disk.c
> @@ -87,7 +87,6 @@ static int prepare_processes(void)
>  	int error;
>  
>  	pm_prepare_console();
> -	sys_sync();
>  	disable_nonboot_cpus();
>  
>  	if (freeze_processes()) {
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 02a1b3a..bd16e44 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -16,7 +16,7 @@
>  /* 
>   * Timeout for stopping processes
>   */
> -#define TIMEOUT	(6 * HZ)
> +#define TIMEOUT	(60 * HZ)
>  
>  
>  static inline int freezeable(struct task_struct * p)
> @@ -54,32 +54,53 @@ void refrigerator(void)
>  	current->state = save;
>  }
>  
> +static void freeze_process(struct task_struct *p)
> +{
> +       unsigned long flags;
> +
> +       freeze(p);
> +       spin_lock_irqsave(&p->sighand->siglock, flags);
> +       signal_wake_up(p, 0);
> +       spin_unlock_irqrestore(&p->sighand->siglock, flags);
> +}
> +
>  /* 0 = success, else # of processes that we failed to stop */
>  int freeze_processes(void)
>  {
> -	int todo;
> +	int todo, user_frozen, nr_user;
>  	unsigned long start_time;
>  	struct task_struct *g, *p;
>  	unsigned long flags;
>  
>  	printk( "Stopping tasks: " );
>  	start_time = jiffies;
> +	user_frozen = 0;
>  	do {
> -		todo = 0;
> +		nr_user = todo = 0;
>  		read_lock(&tasklist_lock);
>  		do_each_thread(g, p) {
>  			if (!freezeable(p))
>  				continue;
>  			if (frozen(p))
>  				continue;
> -
> -			freeze(p);
> -			spin_lock_irqsave(&p->sighand->siglock, flags);
> -			signal_wake_up(p, 0);
> -			spin_unlock_irqrestore(&p->sighand->siglock, flags);
> -			todo++;
> +			if (p->mm) {

This one is too simplistic, it appears.  We should lock the task to check its mm
and there may be kernel threads with non-null mm (they have the
PF_BORROWED_MM flag set though, so we have to check this too).

> +				/* The task is a user-space one. Freeze it */
> +				freeze_process(p);
> +				todo++;

I think you can drop this ...

> +				nr_user++;
> +			} else {
> +				/* Freeze only if the user space is frozen */
> +				if (user_frozen)
> +					freeze_process(p);
> +				todo++;
> +			}
>  		} while_each_thread(g, p);
>  		read_unlock(&tasklist_lock);

... if you do

 +		todo += nr_user;
> +		if (!user_frozen && !nr_user) {
> +			printk("sync");
> +			sys_sync();
> +		}
> +		user_frozen = !nr_user;
>  		yield();			/* Yield is okay here */
>  		if (todo && time_after(jiffies, start_time + TIMEOUT)) {
>  			printk( "\n" );
}-- debugging stuff snipped --{

Greetings,
Rafael