[Suspend2][ 0/2] Freezer Upgrade

[Suspend2][ 0/2] Freezer Upgrade

[Nigel Cunningham]

Patches to upgrade the freezer and disable the updating of the
load average while we're suspended. The former is primarily
quieting debugging information, adding support for thawing
kernel threads without thawing userspace and freezing bdevs.
The later is needed because suspending is very cpu intensive,
and the load average will be high post-resume if we don't do
this. Having a high load average will in turn cause some
processes such as mtas to delay their work until the number
drops.

[Suspend2][ 1/2] [Suspend2] Disable load updating during suspending.

[Nigel Cunningham]

Suspend2 uses the cpu very intensively, with the result that the load
average can be quite high when a cycle has just completed. This in turn can
cause problems with mail delivery and other activities that suspend
activities when the load average gets too high. To avoid this, we suspend
updates of the load average while the freezer is on.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/timer.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index 9e49dee..44a17fc 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -34,6 +34,7 @@
 #include <linux/cpu.h>
 #include <linux/syscalls.h>
 #include <linux/delay.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -869,6 +870,16 @@ static inline void calc_load(unsigned lo
 	count -= ticks;
 	if (count < 0) {
 		count += LOAD_FREQ;
+
+		/* Suspend2 does a lot of work (pagecache I/O) before
+		 * and after the atomic copy. If we let the load average
+		 * be updated while suspending, it will be very high post
+		 * resume. Processes such as some MTAs that stop work
+		 * while the average is high will be unnecessarily disrupted.
+		 */
+		if (freezer_is_on())
+			return;
+
 		active_tasks = count_active_tasks();
 		CALC_LOAD(avenrun[0], EXP_1, active_tasks);
 		CALC_LOAD(avenrun[1], EXP_5, active_tasks);

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 2/2] [Suspend2] Freezer upgrade.

[Nigel Cunningham]

This patch represents the Suspend2 upgrades to the freezer implementation.
Due to recent changes in the vanilla version, I should be able to greatly
reduce the size of this patch. TODO.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 include/linux/freezer.h |   29 +++++++++++
 include/linux/sched.h   |    4 +
 include/linux/suspend.h |    5 ++
 kernel/kmod.c           |    4 +
 kernel/power/disk.c     |    5 +-
 kernel/power/main.c     |    5 +-
 kernel/power/process.c  |  127 ++++++++++++++++++++++++++++++++++++++++-------
 kernel/power/swsusp.c   |    5 ++
 kernel/power/user.c     |    7 +--
 9 files changed, 164 insertions(+), 27 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
new file mode 100644
index 0000000..43ef3b2
--- /dev/null
+++ b/include/linux/freezer.h
@@ -0,0 +1,29 @@
+/* Freezer declarations */
+
+#define FREEZER_ON 0
+#define ABORT_FREEZING 1
+#define FREEZING_COMPLETE 2
+
+#define FREEZER_KERNEL_THREADS 0
+#define FREEZER_ALL_THREADS 1
+
+#ifdef CONFIG_PM
+extern unsigned long freezer_state;
+
+#define test_freezer_state(bit) test_bit(bit, &freezer_state)
+#define set_freezer_state(bit) set_bit(bit, &freezer_state)
+#define clear_freezer_state(bit) clear_bit(bit, &freezer_state)
+
+#define freezer_is_on() (test_freezer_state(FREEZER_ON))
+
+extern void do_freeze_process(struct notifier_block *nl);
+
+#else
+
+#define test_freezer_state(bit) (0)
+#define set_freezer_state(bit) do { } while(0)
+#define clear_freezer_state(bit) do { } while(0)
+
+#define freezer_is_on() (0)
+
+#endif
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 267f152..b6d96ab 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1452,7 +1452,7 @@ static inline void frozen_process(struct
 
 extern void refrigerator(void);
 extern int freeze_processes(void);
-extern void thaw_processes(void);
+extern void thaw_processes(int which_threads);
 
 static inline int try_to_freeze(void)
 {
@@ -1471,7 +1471,7 @@ static inline void frozen_process(struct
 
 static inline void refrigerator(void) {}
 static inline int freeze_processes(void) { BUG(); return 0; }
-static inline void thaw_processes(void) {}
+static inline void thaw_processes(int which_threads) {}
 
 static inline int try_to_freeze(void) { return 0; }
 
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 96e31aa..b128fd2 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -8,6 +8,7 @@
 #include <linux/notifier.h>
 #include <linux/init.h>
 #include <linux/pm.h>
+#include <linux/suspend2.h>
 
 /* page backup entry */
 typedef struct pbe {
@@ -45,6 +46,8 @@ extern int software_suspend(void);
 #if defined(CONFIG_VT) && defined(CONFIG_VT_CONSOLE)
 extern int pm_prepare_console(void);
 extern void pm_restore_console(void);
+extern int freeze_processes(void);
+extern void thaw_processes(int which_threads);
 #else
 static inline int pm_prepare_console(void) { return 0; }
 static inline void pm_restore_console(void) {}
@@ -55,6 +58,8 @@ static inline int software_suspend(void)
 	printk("Warning: fake suspend called\n");
 	return -EPERM;
 }
+static inline int freeze_processes(void) { return 0; }
+static inline void thaw_processes(int which_threads) { }
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_SUSPEND_SMP
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 20a997c..b792b32 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -36,6 +36,7 @@
 #include <linux/mount.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/freezer.h>
 #include <asm/uaccess.h>
 
 extern int max_threads;
@@ -249,6 +250,9 @@ int call_usermodehelper_keys(char *path,
 	if (!khelper_wq)
 		return -EBUSY;
 
+	if (freezer_is_on())
+		return 0;
+
 	if (path[0] == '\0')
 		return 0;
 
diff --git a/kernel/power/disk.c b/kernel/power/disk.c
index 81d4d98..a2463e3 100644
--- a/kernel/power/disk.c
+++ b/kernel/power/disk.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/suspend.h>
+#include <linux/freezer.h>
 #include <linux/syscalls.h>
 #include <linux/reboot.h>
 #include <linux/string.h>
@@ -83,7 +84,7 @@ static int prepare_processes(void)
 	if (!(error = swsusp_shrink_memory()))
 		return 0;
 thaw:
-	thaw_processes();
+	thaw_processes(FREEZER_ALL_THREADS);
 	enable_nonboot_cpus();
 	pm_restore_console();
 	return error;
@@ -92,7 +93,7 @@ thaw:
 static void unprepare_processes(void)
 {
 	platform_finish();
-	thaw_processes();
+	thaw_processes(FREEZER_ALL_THREADS);
 	enable_nonboot_cpus();
 	pm_restore_console();
 }
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 0a907f0..8413db2 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/suspend.h>
+#include <linux/freezer.h>
 #include <linux/kobject.h>
 #include <linux/string.h>
 #include <linux/delay.h>
@@ -96,7 +97,7 @@ static int suspend_prepare(suspend_state
 	if (pm_ops->finish)
 		pm_ops->finish(state);
  Thaw:
-	thaw_processes();
+	thaw_processes(FREEZER_ALL_THREADS);
  Enable_cpu:
 	enable_nonboot_cpus();
 	pm_restore_console();
@@ -135,7 +136,7 @@ static void suspend_finish(suspend_state
 {
 	device_resume();
 	resume_console();
-	thaw_processes();
+	thaw_processes(FREEZER_ALL_THREADS);
 	enable_nonboot_cpus();
 	if (pm_ops && pm_ops->finish)
 		pm_ops->finish(state);
diff --git a/kernel/power/process.c b/kernel/power/process.c
index b2a5f67..020895d 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -5,7 +5,6 @@
  * Originally from swsusp.
  */
 
-
 #undef DEBUG
 
 #include <linux/smp_lock.h>
@@ -13,12 +12,72 @@
 #include <linux/suspend.h>
 #include <linux/module.h>
 #include <linux/syscalls.h>
+#include <linux/buffer_head.h>
+#include <linux/freezer.h>
+
+unsigned long freezer_state = 0;
+
+#ifdef CONFIG_PM_DEBUG
+#define freezer_message(msg, a...) do { printk(msg, ##a); } while(0)
+#else
+#define freezer_message(msg, a...) do { } while(0)
+#endif
 
 /* 
  * Timeout for stopping processes
  */
 #define TIMEOUT	(20 * HZ)
 
+struct frozen_fs
+{
+	struct list_head fsb_list;
+	struct super_block *sb;
+};
+
+LIST_HEAD(frozen_fs_list);
+
+void freezer_make_fses_rw(void)
+{
+	struct frozen_fs *fs, *next_fs;
+
+	list_for_each_entry_safe(fs, next_fs, &frozen_fs_list, fsb_list) {
+		thaw_bdev(fs->sb->s_bdev, fs->sb);
+
+		list_del(&fs->fsb_list);
+		kfree(fs);
+	}
+}
+
+/* 
+ * Done after userspace is frozen, so there should be no danger of
+ * fses being unmounted while we're in here.
+ */
+int freezer_make_fses_ro(void)
+{
+	struct frozen_fs *fs;
+	struct super_block *sb;
+
+	/* Generate the list */
+	list_for_each_entry(sb, &super_blocks, s_list) {
+		if (!sb->s_root || !sb->s_bdev ||
+		    (sb->s_frozen == SB_FREEZE_TRANS) ||
+		    (sb->s_flags & MS_RDONLY))
+			continue;
+
+		fs = kmalloc(sizeof(struct frozen_fs), GFP_ATOMIC);
+		fs->sb = sb;
+		list_add_tail(&fs->fsb_list, &frozen_fs_list);
+	};
+
+	/* Do the freezing in reverse order so filesystems dependant
+	 * upon others are frozen in the right order. (Eg loopback
+	 * on ext3). */
+	list_for_each_entry_reverse(fs, &frozen_fs_list, fsb_list)
+		freeze_bdev(fs->sb->s_bdev);
+
+	return 0;
+}
+
 
 static inline int freezeable(struct task_struct * p)
 {
@@ -39,7 +98,7 @@ void refrigerator(void)
 	long save;
 	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
-	printk("=");
+	freezer_message("=");
 
 	frozen_process(current);
 	spin_lock_irq(&current->sighand->siglock);
@@ -74,9 +133,13 @@ int freeze_processes(void)
 	struct task_struct *g, *p;
 	unsigned long flags;
 
-	printk( "Stopping tasks: " );
+	user_frozen = test_freezer_state(FREEZER_ON);
+	
+	if (!user_frozen)
+		set_freezer_state(FREEZER_ON);
+
+	freezer_message( "Stopping tasks: " );
 	start_time = jiffies;
-	user_frozen = 0;
 	do {
 		nr_user = todo = 0;
 		read_lock(&tasklist_lock);
@@ -103,8 +166,10 @@ int freeze_processes(void)
 		read_unlock(&tasklist_lock);
 		todo += nr_user;
 		if (!user_frozen && !nr_user) {
-			sys_sync();
+			freezer_message("Freezing bdevs... ");
+			freezer_make_fses_ro();
 			start_time = jiffies;
+			freezer_message("Freezing kernel threads... ");
 		}
 		user_frozen = !nr_user;
 		yield();			/* Yield is okay here */
@@ -118,14 +183,14 @@ int freeze_processes(void)
 	 * but it cleans up leftover PF_FREEZE requests.
 	 */
 	if (todo) {
-		printk( "\n" );
-		printk(KERN_ERR " stopping tasks timed out "
+		freezer_message( "\n" );
+		freezer_message(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) {
 			if (freezeable(p) && !frozen(p))
-				printk(KERN_ERR "  %s\n", p->comm);
+				freezer_message(KERN_ERR "  %s\n", p->comm);
 			if (freezing(p)) {
 				pr_debug("  clean up: %s\n", p->comm);
 				p->flags &= ~PF_FREEZE;
@@ -138,27 +203,53 @@ int freeze_processes(void)
 		return todo;
 	}
 
-	printk( "|\n" );
+	freezer_message( "|\n" );
 	BUG_ON(in_atomic());
+	
+	set_freezer_state(FREEZING_COMPLETE);
+
 	return 0;
 }
 
-void thaw_processes(void)
+void thaw_processes(int all)
 {
 	struct task_struct *g, *p;
+	int pass = 0; /* Start on kernel space */
+
+	if (!test_freezer_state(FREEZER_ON))
+		return;
+
+	if (!test_freezer_state(FREEZING_COMPLETE))
+		pass++;
+
+	clear_freezer_state(FREEZING_COMPLETE);
 
-	printk( "Restarting tasks..." );
+	freezer_message( "Restarting tasks..." );
 	read_lock(&tasklist_lock);
-	do_each_thread(g, p) {
-		if (!freezeable(p))
-			continue;
-		if (!thaw_process(p))
-			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
-	} while_each_thread(g, p);
+	do {
+		if (pass) {
+			read_unlock(&tasklist_lock);
+			freezer_make_fses_rw();
+			read_lock(&tasklist_lock);
+		}
+
+		do_each_thread(g, p) {
+			int is_user = !!(p->mm && !(p->flags & PF_BORROWED_MM));
+			if (!freezeable(p) || (is_user != pass))
+				continue;
+			if (!thaw_process(p))
+				freezer_message(KERN_INFO " Strange, %s not stopped\n", p->comm );
+		} while_each_thread(g, p);
+
+		pass++;
+	} while(pass < 2 && all);
 
 	read_unlock(&tasklist_lock);
 	schedule();
-	printk( " done\n" );
+	freezer_message( " done\n" );
+
+	if (all)
+		clear_freezer_state(FREEZER_ON);
 }
 
 EXPORT_SYMBOL(refrigerator);
diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
index c4016cb..64acda1 100644
--- a/kernel/power/swsusp.c
+++ b/kernel/power/swsusp.c
@@ -49,6 +49,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/highmem.h>
+#include <linux/freezer.h>
 
 #include "power.h"
 
@@ -184,6 +185,8 @@ int swsusp_shrink_memory(void)
 	unsigned int i = 0;
 	char *p = "-\\|/";
 
+	thaw_processes(FREEZER_KERNEL_THREADS);
+
 	printk("Shrinking memory...  ");
 	do {
 		size = 2 * count_highmem_pages();
@@ -207,6 +210,8 @@ int swsusp_shrink_memory(void)
 	} while (tmp > 0);
 	printk("\bdone (%lu pages freed)\n", pages);
 
+	freeze_processes();
+
 	return 0;
 }
 
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 3f1539f..10d5c9b 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -19,6 +19,7 @@
 #include <linux/swapops.h>
 #include <linux/pm.h>
 #include <linux/fs.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 
@@ -75,7 +76,7 @@ static int snapshot_release(struct inode
 	free_bitmap(data->bitmap);
 	if (data->frozen) {
 		down(&pm_sem);
-		thaw_processes();
+		thaw_processes(FREEZER_ALL_THREADS);
 		enable_nonboot_cpus();
 		up(&pm_sem);
 	}
@@ -141,7 +142,7 @@ static int snapshot_ioctl(struct inode *
 		down(&pm_sem);
 		disable_nonboot_cpus();
 		if (freeze_processes()) {
-			thaw_processes();
+			thaw_processes(FREEZER_ALL_THREADS);
 			enable_nonboot_cpus();
 			error = -EBUSY;
 		}
@@ -154,7 +155,7 @@ static int snapshot_ioctl(struct inode *
 		if (!data->frozen)
 			break;
 		down(&pm_sem);
-		thaw_processes();
+		thaw_processes(FREEZER_ALL_THREADS);
 		enable_nonboot_cpus();
 		up(&pm_sem);
 		data->frozen = 0;

--
Nigel Cunningham		nigel at suspend2 dot net

Re: [Suspend2][ 2/2] [Suspend2] Freezer upgrade.

[Rafael J. Wysocki]

Hi,

On Monday 26 June 2006 18:38, Nigel Cunningham wrote:
> This patch represents the Suspend2 upgrades to the freezer implementation.
> Due to recent changes in the vanilla version, I should be able to greatly
> reduce the size of this patch. TODO.

So I assume the patch will change in the future.

> Signed-off-by: Nigel Cunningham <nigel@suspend2.net>
> 
>  include/linux/freezer.h |   29 +++++++++++
>  include/linux/sched.h   |    4 +
>  include/linux/suspend.h |    5 ++
>  kernel/kmod.c           |    4 +
>  kernel/power/disk.c     |    5 +-
>  kernel/power/main.c     |    5 +-
>  kernel/power/process.c  |  127 ++++++++++++++++++++++++++++++++++++++++-------
>  kernel/power/swsusp.c   |    5 ++
>  kernel/power/user.c     |    7 +--
>  9 files changed, 164 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> new file mode 100644
> index 0000000..43ef3b2
> --- /dev/null
> +++ b/include/linux/freezer.h
> @@ -0,0 +1,29 @@
> +/* Freezer declarations */
> +
> +#define FREEZER_ON 0
> +#define ABORT_FREEZING 1
> +#define FREEZING_COMPLETE 2
> +
> +#define FREEZER_KERNEL_THREADS 0
> +#define FREEZER_ALL_THREADS 1

I think these need some comments.  It's not clear to me why you need them, actually.

> +
> +#ifdef CONFIG_PM
> +extern unsigned long freezer_state;
> +
> +#define test_freezer_state(bit) test_bit(bit, &freezer_state)
> +#define set_freezer_state(bit) set_bit(bit, &freezer_state)
> +#define clear_freezer_state(bit) clear_bit(bit, &freezer_state)
> +
> +#define freezer_is_on() (test_freezer_state(FREEZER_ON))
> +
> +extern void do_freeze_process(struct notifier_block *nl);

Ditto.

> +
> +#else
> +
> +#define test_freezer_state(bit) (0)
> +#define set_freezer_state(bit) do { } while(0)
> +#define clear_freezer_state(bit) do { } while(0)
> +
> +#define freezer_is_on() (0)
> +
> +#endif
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 267f152..b6d96ab 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1452,7 +1452,7 @@ static inline void frozen_process(struct
>  
>  extern void refrigerator(void);
>  extern int freeze_processes(void);
> -extern void thaw_processes(void);
> +extern void thaw_processes(int which_threads);
>  
>  static inline int try_to_freeze(void)
>  {
> @@ -1471,7 +1471,7 @@ static inline void frozen_process(struct
>  
>  static inline void refrigerator(void) {}
>  static inline int freeze_processes(void) { BUG(); return 0; }
> -static inline void thaw_processes(void) {}
> +static inline void thaw_processes(int which_threads) {}

I'd probably try to introduce two different functions like
thaw_user_processes() and thaw_kernel_threads() instead of this.  Even if they
called the same routine internally, it would be clear what they were for.

BTW, this also affects the suspend-to-RAM, at least on some architectures.

>  
>  static inline int try_to_freeze(void) { return 0; }
>  
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 96e31aa..b128fd2 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -8,6 +8,7 @@
>  #include <linux/notifier.h>
>  #include <linux/init.h>
>  #include <linux/pm.h>
> +#include <linux/suspend2.h>
>  
>  /* page backup entry */
>  typedef struct pbe {
> @@ -45,6 +46,8 @@ extern int software_suspend(void);
>  #if defined(CONFIG_VT) && defined(CONFIG_VT_CONSOLE)
>  extern int pm_prepare_console(void);
>  extern void pm_restore_console(void);
> +extern int freeze_processes(void);
> +extern void thaw_processes(int which_threads);
>  #else
>  static inline int pm_prepare_console(void) { return 0; }
>  static inline void pm_restore_console(void) {}
> @@ -55,6 +58,8 @@ static inline int software_suspend(void)
>  	printk("Warning: fake suspend called\n");
>  	return -EPERM;
>  }
> +static inline int freeze_processes(void) { return 0; }
> +static inline void thaw_processes(int which_threads) { }
>  #endif /* CONFIG_PM */
>  
>  #ifdef CONFIG_SUSPEND_SMP
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 20a997c..b792b32 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -36,6 +36,7 @@
>  #include <linux/mount.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/freezer.h>
>  #include <asm/uaccess.h>
>  
>  extern int max_threads;
> @@ -249,6 +250,9 @@ int call_usermodehelper_keys(char *path,
>  	if (!khelper_wq)
>  		return -EBUSY;
>  
> +	if (freezer_is_on())
> +		return 0;
> +
>  	if (path[0] == '\0')
>  		return 0;
>  

I'm not sure if I agree with this change.  AFAIR, this was discussed some time
ago with no specific conclusion, but at least some people argued it wouldn't
be right to do so.  Could you please provide some arguments?

> diff --git a/kernel/power/disk.c b/kernel/power/disk.c
> index 81d4d98..a2463e3 100644
> --- a/kernel/power/disk.c
> +++ b/kernel/power/disk.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/suspend.h>
> +#include <linux/freezer.h>
>  #include <linux/syscalls.h>
>  #include <linux/reboot.h>
>  #include <linux/string.h>
> @@ -83,7 +84,7 @@ static int prepare_processes(void)
>  	if (!(error = swsusp_shrink_memory()))
>  		return 0;
>  thaw:
> -	thaw_processes();
> +	thaw_processes(FREEZER_ALL_THREADS);
>  	enable_nonboot_cpus();
>  	pm_restore_console();
>  	return error;
> @@ -92,7 +93,7 @@ thaw:
>  static void unprepare_processes(void)
>  {
>  	platform_finish();
> -	thaw_processes();
> +	thaw_processes(FREEZER_ALL_THREADS);
>  	enable_nonboot_cpus();
>  	pm_restore_console();
>  }
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 0a907f0..8413db2 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/suspend.h>
> +#include <linux/freezer.h>
>  #include <linux/kobject.h>
>  #include <linux/string.h>
>  #include <linux/delay.h>
> @@ -96,7 +97,7 @@ static int suspend_prepare(suspend_state
>  	if (pm_ops->finish)
>  		pm_ops->finish(state);
>   Thaw:
> -	thaw_processes();
> +	thaw_processes(FREEZER_ALL_THREADS);
>   Enable_cpu:
>  	enable_nonboot_cpus();
>  	pm_restore_console();
> @@ -135,7 +136,7 @@ static void suspend_finish(suspend_state
>  {
>  	device_resume();
>  	resume_console();
> -	thaw_processes();
> +	thaw_processes(FREEZER_ALL_THREADS);
>  	enable_nonboot_cpus();
>  	if (pm_ops && pm_ops->finish)
>  		pm_ops->finish(state);
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index b2a5f67..020895d 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -5,7 +5,6 @@
>   * Originally from swsusp.
>   */
>  
> -
>  #undef DEBUG
>  
>  #include <linux/smp_lock.h>
> @@ -13,12 +12,72 @@
>  #include <linux/suspend.h>
>  #include <linux/module.h>
>  #include <linux/syscalls.h>
> +#include <linux/buffer_head.h>
> +#include <linux/freezer.h>
> +
> +unsigned long freezer_state = 0;
> +
> +#ifdef CONFIG_PM_DEBUG
> +#define freezer_message(msg, a...) do { printk(msg, ##a); } while(0)
> +#else
> +#define freezer_message(msg, a...) do { } while(0)
> +#endif
>  
>  /* 
>   * Timeout for stopping processes
>   */
>  #define TIMEOUT	(20 * HZ)
>  
> +struct frozen_fs
> +{
> +	struct list_head fsb_list;
> +	struct super_block *sb;
> +};
> +
> +LIST_HEAD(frozen_fs_list);
> +
> +void freezer_make_fses_rw(void)
> +{
> +	struct frozen_fs *fs, *next_fs;
> +
> +	list_for_each_entry_safe(fs, next_fs, &frozen_fs_list, fsb_list) {
> +		thaw_bdev(fs->sb->s_bdev, fs->sb);
> +
> +		list_del(&fs->fsb_list);
> +		kfree(fs);
> +	}
> +}
> +
> +/* 
> + * Done after userspace is frozen, so there should be no danger of
> + * fses being unmounted while we're in here.
> + */
> +int freezer_make_fses_ro(void)
> +{
> +	struct frozen_fs *fs;
> +	struct super_block *sb;
> +
> +	/* Generate the list */
> +	list_for_each_entry(sb, &super_blocks, s_list) {
> +		if (!sb->s_root || !sb->s_bdev ||
> +		    (sb->s_frozen == SB_FREEZE_TRANS) ||
> +		    (sb->s_flags & MS_RDONLY))
> +			continue;
> +
> +		fs = kmalloc(sizeof(struct frozen_fs), GFP_ATOMIC);

Are you _sure_ fs will be !=0 here?

> +		fs->sb = sb;
> +		list_add_tail(&fs->fsb_list, &frozen_fs_list);
> +	};
> +
> +	/* Do the freezing in reverse order so filesystems dependant
> +	 * upon others are frozen in the right order. (Eg loopback
> +	 * on ext3). */
> +	list_for_each_entry_reverse(fs, &frozen_fs_list, fsb_list)
> +		freeze_bdev(fs->sb->s_bdev);
> +
> +	return 0;
> +}
> +
>  
>  static inline int freezeable(struct task_struct * p)
>  {
> @@ -39,7 +98,7 @@ void refrigerator(void)
>  	long save;
>  	save = current->state;
>  	pr_debug("%s entered refrigerator\n", current->comm);
> -	printk("=");
> +	freezer_message("=");

Where is it defined?

>  
>  	frozen_process(current);
>  	spin_lock_irq(&current->sighand->siglock);
> @@ -74,9 +133,13 @@ int freeze_processes(void)
>  	struct task_struct *g, *p;
>  	unsigned long flags;
>  
> -	printk( "Stopping tasks: " );
> +	user_frozen = test_freezer_state(FREEZER_ON);
> +	
> +	if (!user_frozen)
> +		set_freezer_state(FREEZER_ON);

I'm not quite sure it needs to be done this way.

> +
> +	freezer_message( "Stopping tasks: " );
>  	start_time = jiffies;
> -	user_frozen = 0;
>  	do {
>  		nr_user = todo = 0;
>  		read_lock(&tasklist_lock);
> @@ -103,8 +166,10 @@ int freeze_processes(void)
>  		read_unlock(&tasklist_lock);
>  		todo += nr_user;
>  		if (!user_frozen && !nr_user) {
> -			sys_sync();
> +			freezer_message("Freezing bdevs... ");
> +			freezer_make_fses_ro();
>  			start_time = jiffies;
> +			freezer_message("Freezing kernel threads... ");
>  		}
>  		user_frozen = !nr_user;
>  		yield();			/* Yield is okay here */
> @@ -118,14 +183,14 @@ int freeze_processes(void)
>  	 * but it cleans up leftover PF_FREEZE requests.
>  	 */
>  	if (todo) {
> -		printk( "\n" );
> -		printk(KERN_ERR " stopping tasks timed out "
> +		freezer_message( "\n" );
> +		freezer_message(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) {
>  			if (freezeable(p) && !frozen(p))
> -				printk(KERN_ERR "  %s\n", p->comm);
> +				freezer_message(KERN_ERR "  %s\n", p->comm);
>  			if (freezing(p)) {
>  				pr_debug("  clean up: %s\n", p->comm);
>  				p->flags &= ~PF_FREEZE;
> @@ -138,27 +203,53 @@ int freeze_processes(void)
>  		return todo;
>  	}
>  
> -	printk( "|\n" );
> +	freezer_message( "|\n" );
>  	BUG_ON(in_atomic());
> +	
> +	set_freezer_state(FREEZING_COMPLETE);
> +
>  	return 0;
>  }
>  
> -void thaw_processes(void)
> +void thaw_processes(int all)
>  {
>  	struct task_struct *g, *p;
> +	int pass = 0; /* Start on kernel space */
> +
> +	if (!test_freezer_state(FREEZER_ON))
> +		return;
> +
> +	if (!test_freezer_state(FREEZING_COMPLETE))
> +		pass++;
> +
> +	clear_freezer_state(FREEZING_COMPLETE);
>  
> -	printk( "Restarting tasks..." );
> +	freezer_message( "Restarting tasks..." );
>  	read_lock(&tasklist_lock);
> -	do_each_thread(g, p) {
> -		if (!freezeable(p))
> -			continue;
> -		if (!thaw_process(p))
> -			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
> -	} while_each_thread(g, p);
> +	do {
> +		if (pass) {
> +			read_unlock(&tasklist_lock);
> +			freezer_make_fses_rw();
> +			read_lock(&tasklist_lock);
> +		}
> +
> +		do_each_thread(g, p) {
> +			int is_user = !!(p->mm && !(p->flags & PF_BORROWED_MM));
> +			if (!freezeable(p) || (is_user != pass))

Well, this test looks a bit too convoluted, so to speak. ;-)

> +				continue;
> +			if (!thaw_process(p))
> +				freezer_message(KERN_INFO " Strange, %s not stopped\n", p->comm );
> +		} while_each_thread(g, p);
> +
> +		pass++;
> +	} while(pass < 2 && all);
>  
>  	read_unlock(&tasklist_lock);
>  	schedule();
> -	printk( " done\n" );
> +	freezer_message( " done\n" );
> +
> +	if (all)
> +		clear_freezer_state(FREEZER_ON);
>  }
>  
>  EXPORT_SYMBOL(refrigerator);
> diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
> index c4016cb..64acda1 100644
> --- a/kernel/power/swsusp.c
> +++ b/kernel/power/swsusp.c
> @@ -49,6 +49,7 @@
>  #include <linux/bootmem.h>
>  #include <linux/syscalls.h>
>  #include <linux/highmem.h>
> +#include <linux/freezer.h>
>  
>  #include "power.h"
>  
> @@ -184,6 +185,8 @@ int swsusp_shrink_memory(void)
>  	unsigned int i = 0;
>  	char *p = "-\\|/";
>  
> +	thaw_processes(FREEZER_KERNEL_THREADS);
> +

Could you please explain why you think that we should thaw the kernel
threads here?

>  	printk("Shrinking memory...  ");
>  	do {
>  		size = 2 * count_highmem_pages();
> @@ -207,6 +210,8 @@ int swsusp_shrink_memory(void)
>  	} while (tmp > 0);
>  	printk("\bdone (%lu pages freed)\n", pages);
>  
> +	freeze_processes();
> +
>  	return 0;
>  }
>  
> diff --git a/kernel/power/user.c b/kernel/power/user.c
> index 3f1539f..10d5c9b 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -19,6 +19,7 @@
>  #include <linux/swapops.h>
>  #include <linux/pm.h>
>  #include <linux/fs.h>
> +#include <linux/freezer.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -75,7 +76,7 @@ static int snapshot_release(struct inode
>  	free_bitmap(data->bitmap);
>  	if (data->frozen) {
>  		down(&pm_sem);
> -		thaw_processes();
> +		thaw_processes(FREEZER_ALL_THREADS);

I'd prefer if you defined:

 thaw_processes() {
	thaw_kernel_threads();
	thaw_user_processes();
}

so that this change (and the next two) were not necessary.

Also, I think we could try to merge the freezing of bdevs independently if
you can provide a test case in which it is really necessary.

>  		enable_nonboot_cpus();
>  		up(&pm_sem);
>  	}
> @@ -141,7 +142,7 @@ static int snapshot_ioctl(struct inode *
>  		down(&pm_sem);
>  		disable_nonboot_cpus();
>  		if (freeze_processes()) {
> -			thaw_processes();
> +			thaw_processes(FREEZER_ALL_THREADS);
>  			enable_nonboot_cpus();
>  			error = -EBUSY;
>  		}
> @@ -154,7 +155,7 @@ static int snapshot_ioctl(struct inode *
>  		if (!data->frozen)
>  			break;
>  		down(&pm_sem);
> -		thaw_processes();
> +		thaw_processes(FREEZER_ALL_THREADS);
>  		enable_nonboot_cpus();
>  		up(&pm_sem);
>  		data->frozen = 0;
> 
> --

Re: [Suspend2][ 2/2] [Suspend2] Freezer upgrade.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 06:01, Rafael J. Wysocki wrote:
> Hi,
>
> On Monday 26 June 2006 18:38, Nigel Cunningham wrote:
> > This patch represents the Suspend2 upgrades to the freezer
> > implementation. Due to recent changes in the vanilla version, I should be
> > able to greatly reduce the size of this patch. TODO.
>
> So I assume the patch will change in the future.

This is after the changes. Sorry - forgot to update the comment.

> > Signed-off-by: Nigel Cunningham <nigel@suspend2.net>
> >
> >  include/linux/freezer.h |   29 +++++++++++
> >  include/linux/sched.h   |    4 +
> >  include/linux/suspend.h |    5 ++
> >  kernel/kmod.c           |    4 +
> >  kernel/power/disk.c     |    5 +-
> >  kernel/power/main.c     |    5 +-
> >  kernel/power/process.c  |  127
> > ++++++++++++++++++++++++++++++++++++++++------- kernel/power/swsusp.c   |
> >    5 ++
> >  kernel/power/user.c     |    7 +--
> >  9 files changed, 164 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> > new file mode 100644
> > index 0000000..43ef3b2
> > --- /dev/null
> > +++ b/include/linux/freezer.h
> > @@ -0,0 +1,29 @@
> > +/* Freezer declarations */
> > +
> > +#define FREEZER_ON 0
> > +#define ABORT_FREEZING 1
> > +#define FREEZING_COMPLETE 2
> > +
> > +#define FREEZER_KERNEL_THREADS 0
> > +#define FREEZER_ALL_THREADS 1
>
> I think these need some comments.  It's not clear to me why you need them,
> actually.

Ok.

> > +
> > +#ifdef CONFIG_PM
> > +extern unsigned long freezer_state;
> > +
> > +#define test_freezer_state(bit) test_bit(bit, &freezer_state)
> > +#define set_freezer_state(bit) set_bit(bit, &freezer_state)
> > +#define clear_freezer_state(bit) clear_bit(bit, &freezer_state)
> > +
> > +#define freezer_is_on() (test_freezer_state(FREEZER_ON))
> > +
> > +extern void do_freeze_process(struct notifier_block *nl);
>
> Ditto.

do_freeze_process should go. It's a cleanup I missed when I stopped using 
Christoph's todo list code.

> > +
> > +#else
> > +
> > +#define test_freezer_state(bit) (0)
> > +#define set_freezer_state(bit) do { } while(0)
> > +#define clear_freezer_state(bit) do { } while(0)
> > +
> > +#define freezer_is_on() (0)
> > +
> > +#endif
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 267f152..b6d96ab 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1452,7 +1452,7 @@ static inline void frozen_process(struct
> >
> >  extern void refrigerator(void);
> >  extern int freeze_processes(void);
> > -extern void thaw_processes(void);
> > +extern void thaw_processes(int which_threads);
> >
> >  static inline int try_to_freeze(void)
> >  {
> > @@ -1471,7 +1471,7 @@ static inline void frozen_process(struct
> >
> >  static inline void refrigerator(void) {}
> >  static inline int freeze_processes(void) { BUG(); return 0; }
> > -static inline void thaw_processes(void) {}
> > +static inline void thaw_processes(int which_threads) {}
>
> I'd probably try to introduce two different functions like
> thaw_user_processes() and thaw_kernel_threads() instead of this.  Even if
> they called the same routine internally, it would be clear what they were
> for.

Ok. I was just trying to minimise the delta, so I don't mind this idea at all.
thaw_user_processes() would imply thawing kernel threads in the logic I have 
at the moment. Would calling it thaw_processes() instead sound ok?

> BTW, this also affects the suspend-to-RAM, at least on some architectures.

I'll double check for other refrigerator calls then.

> >  static inline int try_to_freeze(void) { return 0; }
> >
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 96e31aa..b128fd2 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -8,6 +8,7 @@
> >  #include <linux/notifier.h>
> >  #include <linux/init.h>
> >  #include <linux/pm.h>
> > +#include <linux/suspend2.h>
> >
> >  /* page backup entry */
> >  typedef struct pbe {
> > @@ -45,6 +46,8 @@ extern int software_suspend(void);
> >  #if defined(CONFIG_VT) && defined(CONFIG_VT_CONSOLE)
> >  extern int pm_prepare_console(void);
> >  extern void pm_restore_console(void);
> > +extern int freeze_processes(void);
> > +extern void thaw_processes(int which_threads);
> >  #else
> >  static inline int pm_prepare_console(void) { return 0; }
> >  static inline void pm_restore_console(void) {}
> > @@ -55,6 +58,8 @@ static inline int software_suspend(void)
> >  	printk("Warning: fake suspend called\n");
> >  	return -EPERM;
> >  }
> > +static inline int freeze_processes(void) { return 0; }
> > +static inline void thaw_processes(int which_threads) { }
> >  #endif /* CONFIG_PM */
> >
> >  #ifdef CONFIG_SUSPEND_SMP
> > diff --git a/kernel/kmod.c b/kernel/kmod.c
> > index 20a997c..b792b32 100644
> > --- a/kernel/kmod.c
> > +++ b/kernel/kmod.c
> > @@ -36,6 +36,7 @@
> >  #include <linux/mount.h>
> >  #include <linux/kernel.h>
> >  #include <linux/init.h>
> > +#include <linux/freezer.h>
> >  #include <asm/uaccess.h>
> >
> >  extern int max_threads;
> > @@ -249,6 +250,9 @@ int call_usermodehelper_keys(char *path,
> >  	if (!khelper_wq)
> >  		return -EBUSY;
> >
> > +	if (freezer_is_on())
> > +		return 0;
> > +
> >  	if (path[0] == '\0')
> >  		return 0;
>
> I'm not sure if I agree with this change.  AFAIR, this was discussed some
> time ago with no specific conclusion, but at least some people argued it
> wouldn't be right to do so.  Could you please provide some arguments?

Yes, I don't remember a specific conclusion either. I'm more than happy to see 
something else happen. I can just report that this has been used for quite a 
while with no negative reports. It would be good to use this as provocation 
to come up with a clear agreement on the right way.

> > diff --git a/kernel/power/disk.c b/kernel/power/disk.c
> > index 81d4d98..a2463e3 100644
> > --- a/kernel/power/disk.c
> > +++ b/kernel/power/disk.c
> > @@ -10,6 +10,7 @@
> >   */
> >
> >  #include <linux/suspend.h>
> > +#include <linux/freezer.h>
> >  #include <linux/syscalls.h>
> >  #include <linux/reboot.h>
> >  #include <linux/string.h>
> > @@ -83,7 +84,7 @@ static int prepare_processes(void)
> >  	if (!(error = swsusp_shrink_memory()))
> >  		return 0;
> >  thaw:
> > -	thaw_processes();
> > +	thaw_processes(FREEZER_ALL_THREADS);
> >  	enable_nonboot_cpus();
> >  	pm_restore_console();
> >  	return error;
> > @@ -92,7 +93,7 @@ thaw:
> >  static void unprepare_processes(void)
> >  {
> >  	platform_finish();
> > -	thaw_processes();
> > +	thaw_processes(FREEZER_ALL_THREADS);
> >  	enable_nonboot_cpus();
> >  	pm_restore_console();
> >  }
> > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > index 0a907f0..8413db2 100644
> > --- a/kernel/power/main.c
> > +++ b/kernel/power/main.c
> > @@ -9,6 +9,7 @@
> >   */
> >
> >  #include <linux/suspend.h>
> > +#include <linux/freezer.h>
> >  #include <linux/kobject.h>
> >  #include <linux/string.h>
> >  #include <linux/delay.h>
> > @@ -96,7 +97,7 @@ static int suspend_prepare(suspend_state
> >  	if (pm_ops->finish)
> >  		pm_ops->finish(state);
> >   Thaw:
> > -	thaw_processes();
> > +	thaw_processes(FREEZER_ALL_THREADS);
> >   Enable_cpu:
> >  	enable_nonboot_cpus();
> >  	pm_restore_console();
> > @@ -135,7 +136,7 @@ static void suspend_finish(suspend_state
> >  {
> >  	device_resume();
> >  	resume_console();
> > -	thaw_processes();
> > +	thaw_processes(FREEZER_ALL_THREADS);
> >  	enable_nonboot_cpus();
> >  	if (pm_ops && pm_ops->finish)
> >  		pm_ops->finish(state);
> > diff --git a/kernel/power/process.c b/kernel/power/process.c
> > index b2a5f67..020895d 100644
> > --- a/kernel/power/process.c
> > +++ b/kernel/power/process.c
> > @@ -5,7 +5,6 @@
> >   * Originally from swsusp.
> >   */
> >
> > -
> >  #undef DEBUG
> >
> >  #include <linux/smp_lock.h>
> > @@ -13,12 +12,72 @@
> >  #include <linux/suspend.h>
> >  #include <linux/module.h>
> >  #include <linux/syscalls.h>
> > +#include <linux/buffer_head.h>
> > +#include <linux/freezer.h>
> > +
> > +unsigned long freezer_state = 0;
> > +
> > +#ifdef CONFIG_PM_DEBUG
> > +#define freezer_message(msg, a...) do { printk(msg, ##a); } while(0)
> > +#else
> > +#define freezer_message(msg, a...) do { } while(0)
> > +#endif
> >
> >  /*
> >   * Timeout for stopping processes
> >   */
> >  #define TIMEOUT	(20 * HZ)
> >
> > +struct frozen_fs
> > +{
> > +	struct list_head fsb_list;
> > +	struct super_block *sb;
> > +};
> > +
> > +LIST_HEAD(frozen_fs_list);
> > +
> > +void freezer_make_fses_rw(void)
> > +{
> > +	struct frozen_fs *fs, *next_fs;
> > +
> > +	list_for_each_entry_safe(fs, next_fs, &frozen_fs_list, fsb_list) {
> > +		thaw_bdev(fs->sb->s_bdev, fs->sb);
> > +
> > +		list_del(&fs->fsb_list);
> > +		kfree(fs);
> > +	}
> > +}
> > +
> > +/*
> > + * Done after userspace is frozen, so there should be no danger of
> > + * fses being unmounted while we're in here.
> > + */
> > +int freezer_make_fses_ro(void)
> > +{
> > +	struct frozen_fs *fs;
> > +	struct super_block *sb;
> > +
> > +	/* Generate the list */
> > +	list_for_each_entry(sb, &super_blocks, s_list) {
> > +		if (!sb->s_root || !sb->s_bdev ||
> > +		    (sb->s_frozen == SB_FREEZE_TRANS) ||
> > +		    (sb->s_flags & MS_RDONLY))
> > +			continue;
> > +
> > +		fs = kmalloc(sizeof(struct frozen_fs), GFP_ATOMIC);
>
> Are you _sure_ fs will be !=0 here?

Thanks. I'll add a check. I know never having seen it fail doesn't mean it 
can't :)

> > +		fs->sb = sb;
> > +		list_add_tail(&fs->fsb_list, &frozen_fs_list);
> > +	};
> > +
> > +	/* Do the freezing in reverse order so filesystems dependant
> > +	 * upon others are frozen in the right order. (Eg loopback
> > +	 * on ext3). */
> > +	list_for_each_entry_reverse(fs, &frozen_fs_list, fsb_list)
> > +		freeze_bdev(fs->sb->s_bdev);
> > +
> > +	return 0;
> > +}
> > +
> >
> >  static inline int freezeable(struct task_struct * p)
> >  {
> > @@ -39,7 +98,7 @@ void refrigerator(void)
> >  	long save;
> >  	save = current->state;
> >  	pr_debug("%s entered refrigerator\n", current->comm);
> > -	printk("=");
> > +	freezer_message("=");
>
> Where is it defined?

At the top of the file.

> >  	frozen_process(current);
> >  	spin_lock_irq(&current->sighand->siglock);
> > @@ -74,9 +133,13 @@ int freeze_processes(void)
> >  	struct task_struct *g, *p;
> >  	unsigned long flags;
> >
> > -	printk( "Stopping tasks: " );
> > +	user_frozen = test_freezer_state(FREEZER_ON);
> > +
> > +	if (!user_frozen)
> > +		set_freezer_state(FREEZER_ON);
>
> I'm not quite sure it needs to be done this way.

We want this path to handle two situations. First, where nothing is frozen and 
second, where userspace is already frozen. In the second case, we don't want 
to try to freeze userspace all over again (we'll deadlock for a start), and 
we also don't want to try to freeze bdevs again (another cause for 
deadlocking).

> > +
> > +	freezer_message( "Stopping tasks: " );
> >  	start_time = jiffies;
> > -	user_frozen = 0;
> >  	do {
> >  		nr_user = todo = 0;
> >  		read_lock(&tasklist_lock);
> > @@ -103,8 +166,10 @@ int freeze_processes(void)
> >  		read_unlock(&tasklist_lock);
> >  		todo += nr_user;
> >  		if (!user_frozen && !nr_user) {
> > -			sys_sync();
> > +			freezer_message("Freezing bdevs... ");
> > +			freezer_make_fses_ro();
> >  			start_time = jiffies;
> > +			freezer_message("Freezing kernel threads... ");
> >  		}
> >  		user_frozen = !nr_user;
> >  		yield();			/* Yield is okay here */
> > @@ -118,14 +183,14 @@ int freeze_processes(void)
> >  	 * but it cleans up leftover PF_FREEZE requests.
> >  	 */
> >  	if (todo) {
> > -		printk( "\n" );
> > -		printk(KERN_ERR " stopping tasks timed out "
> > +		freezer_message( "\n" );
> > +		freezer_message(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) {
> >  			if (freezeable(p) && !frozen(p))
> > -				printk(KERN_ERR "  %s\n", p->comm);
> > +				freezer_message(KERN_ERR "  %s\n", p->comm);
> >  			if (freezing(p)) {
> >  				pr_debug("  clean up: %s\n", p->comm);
> >  				p->flags &= ~PF_FREEZE;
> > @@ -138,27 +203,53 @@ int freeze_processes(void)
> >  		return todo;
> >  	}
> >
> > -	printk( "|\n" );
> > +	freezer_message( "|\n" );
> >  	BUG_ON(in_atomic());
> > +
> > +	set_freezer_state(FREEZING_COMPLETE);
> > +
> >  	return 0;
> >  }
> >
> > -void thaw_processes(void)
> > +void thaw_processes(int all)
> >  {
> >  	struct task_struct *g, *p;
> > +	int pass = 0; /* Start on kernel space */
> > +
> > +	if (!test_freezer_state(FREEZER_ON))
> > +		return;
> > +
> > +	if (!test_freezer_state(FREEZING_COMPLETE))
> > +		pass++;
> > +
> > +	clear_freezer_state(FREEZING_COMPLETE);
> >
> > -	printk( "Restarting tasks..." );
> > +	freezer_message( "Restarting tasks..." );
> >  	read_lock(&tasklist_lock);
> > -	do_each_thread(g, p) {
> > -		if (!freezeable(p))
> > -			continue;
> > -		if (!thaw_process(p))
> > -			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
> > -	} while_each_thread(g, p);
> > +	do {
> > +		if (pass) {
> > +			read_unlock(&tasklist_lock);
> > +			freezer_make_fses_rw();
> > +			read_lock(&tasklist_lock);
> > +		}
> > +
> > +		do_each_thread(g, p) {
> > +			int is_user = !!(p->mm && !(p->flags & PF_BORROWED_MM));
> > +			if (!freezeable(p) || (is_user != pass))
>
> Well, this test looks a bit too convoluted, so to speak. ;-)

It needs to be readable too?! I'd better submit patches for some other bits of 
the kernel too then! :). Seriously, though, pass == 0 if we're thawing kernel 
threads or 1 if userspace. Would adding a comment to this effect make you 
happy?

> > +				continue;
> > +			if (!thaw_process(p))
> > +				freezer_message(KERN_INFO " Strange, %s not stopped\n", p->comm );
> > +		} while_each_thread(g, p);
> > +
> > +		pass++;
> > +	} while(pass < 2 && all);
> >
> >  	read_unlock(&tasklist_lock);
> >  	schedule();
> > -	printk( " done\n" );
> > +	freezer_message( " done\n" );
> > +
> > +	if (all)
> > +		clear_freezer_state(FREEZER_ON);
> >  }
> >
> >  EXPORT_SYMBOL(refrigerator);
> > diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
> > index c4016cb..64acda1 100644
> > --- a/kernel/power/swsusp.c
> > +++ b/kernel/power/swsusp.c
> > @@ -49,6 +49,7 @@
> >  #include <linux/bootmem.h>
> >  #include <linux/syscalls.h>
> >  #include <linux/highmem.h>
> > +#include <linux/freezer.h>
> >
> >  #include "power.h"
> >
> > @@ -184,6 +185,8 @@ int swsusp_shrink_memory(void)
> >  	unsigned int i = 0;
> >  	char *p = "-\\|/";
> >
> > +	thaw_processes(FREEZER_KERNEL_THREADS);
> > +
>
> Could you please explain why you think that we should thaw the kernel
> threads here?

IIRC, there are deadlocking issues if swap is on a journalled filesystem and 
the kjournald et al are frozen.

> >  	printk("Shrinking memory...  ");
> >  	do {
> >  		size = 2 * count_highmem_pages();
> > @@ -207,6 +210,8 @@ int swsusp_shrink_memory(void)
> >  	} while (tmp > 0);
> >  	printk("\bdone (%lu pages freed)\n", pages);
> >
> > +	freeze_processes();
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/kernel/power/user.c b/kernel/power/user.c
> > index 3f1539f..10d5c9b 100644
> > --- a/kernel/power/user.c
> > +++ b/kernel/power/user.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/swapops.h>
> >  #include <linux/pm.h>
> >  #include <linux/fs.h>
> > +#include <linux/freezer.h>
> >
> >  #include <asm/uaccess.h>
> >
> > @@ -75,7 +76,7 @@ static int snapshot_release(struct inode
> >  	free_bitmap(data->bitmap);
> >  	if (data->frozen) {
> >  		down(&pm_sem);
> > -		thaw_processes();
> > +		thaw_processes(FREEZER_ALL_THREADS);
>
> I'd prefer if you defined:
>
>  thaw_processes() {
> 	thaw_kernel_threads();
> 	thaw_user_processes();
> }
>
> so that this change (and the next two) were not necessary.

Ok. I'll wait to see if you like the suggestion above before I start to do 
that.

> Also, I think we could try to merge the freezing of bdevs independently if
> you can provide a test case in which it is really necessary.

XFS. Did you see Nathan's reply not long ago, confirming that it doesn't stop 
all activity if you don't freeze bdevs? That isn't critical for swsusp 
(although I guess you could end up with some filesystem inconsistency if XFS 
writes something after the atomic copy), but keeping the LRU static is 
important for suspend2.

> >  		enable_nonboot_cpus();
> >  		up(&pm_sem);
> >  	}
> > @@ -141,7 +142,7 @@ static int snapshot_ioctl(struct inode *
> >  		down(&pm_sem);
> >  		disable_nonboot_cpus();
> >  		if (freeze_processes()) {
> > -			thaw_processes();
> > +			thaw_processes(FREEZER_ALL_THREADS);
> >  			enable_nonboot_cpus();
> >  			error = -EBUSY;
> >  		}
> > @@ -154,7 +155,7 @@ static int snapshot_ioctl(struct inode *
> >  		if (!data->frozen)
> >  			break;
> >  		down(&pm_sem);
> > -		thaw_processes();
> > +		thaw_processes(FREEZER_ALL_THREADS);
> >  		enable_nonboot_cpus();
> >  		up(&pm_sem);
> >  		data->frozen = 0;
> >
> > --

Thanks very much for the feedback!

Nigel

-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

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

Re: [Suspend2][ 2/2] [Suspend2] Freezer upgrade.

[Pavel Machek]

Hi!

> > > This patch represents the Suspend2 upgrades to the freezer
> > > implementation. Due to recent changes in the vanilla version, I should be
> > > able to greatly reduce the size of this patch. TODO.
> >
> > So I assume the patch will change in the future.
> 
> This is after the changes. Sorry - forgot to update the comment.

Also please explain why we want those patches. "upgrades the freezer"
is not good enough reason to apply a patch.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Suspend2][ 1/2] [Suspend2] Disable load updating during suspending.

[Pavel Machek]

Hi!

> Suspend2 uses the cpu very intensively, with the result that the load
> average can be quite high when a cycle has just completed. This in turn can
> cause problems with mail delivery and other activities that suspend
> activities when the load average gets too high. To avoid this, we suspend
> updates of the load average while the freezer is on.

If we want to do this at all... why not simply set load average to
zero when resume is done?

After all, system probably was completely idle for quite a while :-).
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Suspend2][ 2/2] [Suspend2] Freezer upgrade.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 21:09, Pavel Machek wrote:
> Hi!
>
> > > > This patch represents the Suspend2 upgrades to the freezer
> > > > implementation. Due to recent changes in the vanilla version, I
> > > > should be able to greatly reduce the size of this patch. TODO.
> > >
> > > So I assume the patch will change in the future.
> >
> > This is after the changes. Sorry - forgot to update the comment.
>
> Also please explain why we want those patches. "upgrades the freezer"
> is not good enough reason to apply a patch.

I guess you missed the reply to Rafael. In it, I wrote:

"XFS. Did you see Nathan's reply not long ago, confirming that it doesn't stop 
all activity if you don't freeze bdevs? That isn't critical for swsusp 
(although I guess you could end up with some filesystem inconsistency if XFS 
writes something after the atomic copy), but keeping the LRU static is 
important for suspend2."

In another email, I mentioned that trying to free memory with swap on a 
journalled filesystem is a potential deadlock situation without the 
capability of thawing kernel threads alone. You could potentially thaw all 
threads while eating memory, but then there's a greater chance of racing 
against another program that's trying to allocate memory (if you're in this 
situation, you're low on memory to start with).

Regards,

Nigel

-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

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

Re: [Suspend2][ 1/2] [Suspend2] Disable load updating during suspending.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 22:07, Pavel Machek wrote:
> Hi!
>
> > Suspend2 uses the cpu very intensively, with the result that the load
> > average can be quite high when a cycle has just completed. This in turn
> > can cause problems with mail delivery and other activities that suspend
> > activities when the load average gets too high. To avoid this, we suspend
> > updates of the load average while the freezer is on.
>
> If we want to do this at all... why not simply set load average to
> zero when resume is done?
>
> After all, system probably was completely idle for quite a while :-).

Yeah, that's a possibility. Neither seems inherently better to me. Maybe 
others will come up with an argument for one or the other?

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

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

Re: [Suspend2][ 1/2] [Suspend2] Disable load updating during suspending.

[Andreas Mohr]

Hi,

On Tue, Jun 27, 2006 at 10:16:03PM +1000, Nigel Cunningham wrote:
> Hi.
> 
> On Tuesday 27 June 2006 22:07, Pavel Machek wrote:
> > Hi!
> >
> > > Suspend2 uses the cpu very intensively, with the result that the load
> > > average can be quite high when a cycle has just completed. This in turn
> > > can cause problems with mail delivery and other activities that suspend
> > > activities when the load average gets too high. To avoid this, we suspend
> > > updates of the load average while the freezer is on.
> >
> > If we want to do this at all... why not simply set load average to
> > zero when resume is done?
> >
> > After all, system probably was completely idle for quite a while :-).
> 
> Yeah, that's a possibility. Neither seems inherently better to me. Maybe 
> others will come up with an argument for one or the other?

I don't know, why would *any* of the processes running on this system
be concerned with how much time this system has been suspended, thrown in
the fridge, shipped overseas and back and then finally resumed?
The current load average, OTOH, seems to be much more important than the time
spent sitting idle gathering dust, since those processes are still running
(after resume, that is) and caring about what other processes do and
what they thus should or shouldn't do based on the activity of others.

OK, some processes might do a time-based and not load-average-based
evaluation (thus there would be a change in time behaviour after resume),
but OTOH many processes *continue* their running state so the load average
should reflect a *continued* state, not a very drastic change to zero
(and then sharply climbing back up from zeroed state).

BTW, thanks a lot for this incredible patch activity now, Nigel!
(and thanks to all for such a nice functionality!)

Andreas Mohr