[PATCH 0/4] swsusp cleanups

[PATCH 0/4] swsusp cleanups

[Rafael J. Wysocki]

Hi,

The following series of patches cleans up some suspend-related code.

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

[PATCH 1/4] swsusp: Untangle thaw_processes

[Rafael J. Wysocki]

Move the loop from thaw_processes() to a separate function and call it
independently for kernel threads and user space processes so that the order
of thawing tasks is clearly visible.

Drop thaw_kernel_threads() which is never used.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |    6 -----
 kernel/power/process.c  |   55 +++++++++++++++++++++++++++---------------------
 2 files changed, 33 insertions(+), 28 deletions(-)

Index: linux-2.6.19-rc5-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.19-rc5-mm2.orig/include/linux/freezer.h
+++ linux-2.6.19-rc5-mm2/include/linux/freezer.h
@@ -1,8 +1,5 @@
 /* Freezer declarations */
 
-#define FREEZER_KERNEL_THREADS 0
-#define FREEZER_ALL_THREADS 1
-
 #ifdef CONFIG_PM
 /*
  * Check if a process has been frozen
@@ -60,8 +57,7 @@ static inline void frozen_process(struct
 
 extern void refrigerator(void);
 extern int freeze_processes(void);
-#define thaw_processes() do { thaw_some_processes(FREEZER_ALL_THREADS); } while(0)
-#define thaw_kernel_threads() do { thaw_some_processes(FREEZER_KERNEL_THREADS); } while(0)
+extern void thaw_processes(void);
 
 static inline int try_to_freeze(void)
 {
Index: linux-2.6.19-rc5-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/process.c
+++ linux-2.6.19-rc5-mm2/kernel/power/process.c
@@ -20,6 +20,8 @@
  */
 #define TIMEOUT	(20 * HZ)
 
+#define FREEZER_KERNEL_THREADS 0
+#define FREEZER_USER_SPACE 1
 
 static inline int freezeable(struct task_struct * p)
 {
@@ -79,6 +81,11 @@ static void cancel_freezing(struct task_
 	}
 }
 
+static inline int is_user_space(struct task_struct *p)
+{
+	return p->mm && !(p->flags & PF_BORROWED_MM);
+}
+
 /* 0 = success, else # of processes that we failed to stop */
 int freeze_processes(void)
 {
@@ -103,10 +110,9 @@ int freeze_processes(void)
 				cancel_freezing(p);
 				continue;
 			}
-			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 (is_user_space(p)) {
+				/* Freeze the task unless there is a vfork
+				 * completion pending
 				 */
 				if (!p->vfork_done)
 					freeze_process(p);
@@ -155,31 +161,34 @@ int freeze_processes(void)
 	return 0;
 }
 
-void thaw_some_processes(int all)
+static void thaw_tasks(int thaw_user_space)
 {
 	struct task_struct *g, *p;
-	int pass = 0; /* Pass 0 = Kernel space, 1 = Userspace */
 
-	printk("Restarting tasks... ");
 	read_lock(&tasklist_lock);
-	do {
-		do_each_thread(g, p) {
-			/*
-			 * is_user = 0 if kernel thread or borrowed mm,
-			 * 1 otherwise.
-			 */
-			int is_user = !!(p->mm && !(p->flags & PF_BORROWED_MM));
-			if (!freezeable(p) || (is_user != pass))
-				continue;
-			if (!thaw_process(p))
-				printk(KERN_INFO
-					"Strange, %s not stopped\n", p->comm);
-		} while_each_thread(g, p);
-
-		pass++;
-	} while (pass < 2 && all);
+	do_each_thread(g, p) {
+		if (!freezeable(p))
+			continue;
 
+		if (is_user_space(p)) {
+			if (!thaw_user_space)
+				continue;
+		} else {
+			if (thaw_user_space)
+				continue;
+		}
+		if (!thaw_process(p))
+			printk(KERN_WARNING " Strange, %s not stopped\n",
+				p->comm );
+	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
+}
+
+void thaw_processes(void)
+{
+	printk("Restarting tasks ... ");
+	thaw_tasks(FREEZER_KERNEL_THREADS);
+	thaw_tasks(FREEZER_USER_SPACE);
 	schedule();
 	printk("done.\n");
 }

[PATCH 2/4] swsusp: Untangle freeze_processes

[Rafael J. Wysocki]

Move the loop from freeze_processes() to a separate function and call it
independently for user space processes and kernel threads so that the order of
freezing tasks is clearly visible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/process.c |   88 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 30 deletions(-)

Index: linux-2.6.19-rc5-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/process.c
+++ linux-2.6.19-rc5-mm2/kernel/power/process.c
@@ -86,24 +86,23 @@ static inline int is_user_space(struct t
 	return p->mm && !(p->flags & PF_BORROWED_MM);
 }
 
-/* 0 = success, else # of processes that we failed to stop */
-int freeze_processes(void)
+static unsigned int try_to_freeze_tasks(int freeze_user_space)
 {
-	int todo, nr_user, user_frozen;
-	unsigned long start_time;
 	struct task_struct *g, *p;
+	unsigned long end_time;
+	unsigned int todo;
 
-	printk("Stopping tasks... ");
-	start_time = jiffies;
-	user_frozen = 0;
+	end_time = jiffies + TIMEOUT;
 	do {
-		nr_user = todo = 0;
+		todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			if (!freezeable(p))
 				continue;
+
 			if (frozen(p))
 				continue;
+
 			if (p->state == TASK_TRACED &&
 			    (frozen(p->parent) ||
 			     p->parent->state == TASK_STOPPED)) {
@@ -111,51 +110,80 @@ int freeze_processes(void)
 				continue;
 			}
 			if (is_user_space(p)) {
+				if (!freeze_user_space)
+					continue;
+
 				/* Freeze the task unless there is 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++;
+				if (freeze_user_space)
+					continue;
+
+				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))
+		if (todo && time_after(jiffies, end_time))
 			break;
-	} while(todo);
+	} while (todo);
 
-	/* 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) {
+		/* 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.
+		 */
 		printk("\n");
-		printk(KERN_ERR "Stopping tasks timed out "
-			"after %d seconds (%d tasks remaining):\n",
-			TIMEOUT / HZ, todo);
+		printk(KERN_ERR "Stopping %s timed out after %d seconds "
+				"(%d tasks refusing to freeze):\n",
+				freeze_user_space ? "user space processes" :
+					"kernel threads",
+				TIMEOUT / HZ, todo);
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
+			if (is_user_space(p)) {
+				if(!freeze_user_space)
+					continue;
+			} else {
+				if(freeze_user_space)
+					continue;
+			}
 			if (freezeable(p) && !frozen(p))
 				printk(KERN_ERR " %s\n", p->comm);
+
 			cancel_freezing(p);
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
-		return todo;
 	}
 
+	return todo;
+}
+
+/**
+ *	freeze_processes - tell processes to enter the refrigerator
+ *
+ *	Returns 0 on success, or the number of processes that didn't freeze,
+ *	although they were told to.
+ */
+int freeze_processes(void)
+{
+	unsigned int nr_unfrozen;
+
+	printk("Stopping tasks ... ");
+	nr_unfrozen = try_to_freeze_tasks(FREEZER_USER_SPACE);
+	if (nr_unfrozen)
+		return nr_unfrozen;
+
+	sys_sync();
+	nr_unfrozen = try_to_freeze_tasks(FREEZER_KERNEL_THREADS);
+	if (nr_unfrozen)
+		return nr_unfrozen;
+
 	printk("done.\n");
 	BUG_ON(in_atomic());
 	return 0;

[PATCH 3/4] swsusp: Fix coding style in suspend.c

[Rafael J. Wysocki]

Fix coding style in suspend.c.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/snapshot.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.19-rc5-mm2/kernel/power/snapshot.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/snapshot.c
+++ linux-2.6.19-rc5-mm2/kernel/power/snapshot.c
@@ -85,7 +85,8 @@ unsigned long get_safe_page(gfp_t gfp_ma
 	return (unsigned long)get_image_page(gfp_mask, PG_SAFE);
 }
 
-static struct page *alloc_image_page(gfp_t gfp_mask) {
+static struct page *alloc_image_page(gfp_t gfp_mask)
+{
 	struct page *page;
 
 	page = alloc_page(gfp_mask);

[PATCH 4/4] swsusp: Fix labels

[Rafael J. Wysocki]

Move all labels in the swsusp code to the second column, so that they won't
fool diff -p.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/disk.c     |    6 +++---
 kernel/power/snapshot.c |    8 ++++----
 kernel/power/swap.c     |    4 ++--
 kernel/power/swsusp.c   |    2 +-
 kernel/power/user.c     |    2 +-
 5 files changed, 11 insertions(+), 11 deletions(-)

Index: linux-2.6.19-rc5-mm2/kernel/power/disk.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/disk.c
+++ linux-2.6.19-rc5-mm2/kernel/power/disk.c
@@ -119,9 +119,9 @@ static int prepare_processes(void)
 		return 0;
 
 	platform_finish();
-thaw:
+ thaw:
 	thaw_processes();
-enable_cpus:
+ enable_cpus:
 	enable_nonboot_cpus();
 	pm_restore_console();
 	return error;
@@ -394,7 +394,7 @@ static ssize_t resume_store(struct subsy
 	noresume = 0;
 	software_resume();
 	ret = n;
-out:
+ out:
 	return ret;
 }
 
Index: linux-2.6.19-rc5-mm2/kernel/power/snapshot.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/snapshot.c
+++ linux-2.6.19-rc5-mm2/kernel/power/snapshot.c
@@ -411,7 +411,7 @@ memory_bm_create(struct memory_bitmap *b
 	memory_bm_position_reset(bm);
 	return 0;
 
-Free:
+ Free:
 	bm->p_list = ca.chain;
 	memory_bm_free(bm, PG_UNSAFE_CLEAR);
 	return -ENOMEM;
@@ -557,7 +557,7 @@ static unsigned long memory_bm_next_pfn(
 	memory_bm_position_reset(bm);
 	return BM_END_OF_MAP;
 
-Return_pfn:
+ Return_pfn:
 	bm->cur.chunk = chunk;
 	bm->cur.bit = bit;
 	return bb->start_pfn + chunk * BM_BITS_PER_CHUNK + bit;
@@ -964,7 +964,7 @@ swsusp_alloc(struct memory_bitmap *orig_
 	}
 	return 0;
 
-Free:
+ Free:
 	swsusp_free();
 	return -ENOMEM;
 }
@@ -1540,7 +1540,7 @@ prepare_image(struct memory_bitmap *new_
 	}
 	return 0;
 
-Free:
+ Free:
 	swsusp_free();
 	return error;
 }
Index: linux-2.6.19-rc5-mm2/kernel/power/swsusp.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/swsusp.c
+++ linux-2.6.19-rc5-mm2/kernel/power/swsusp.c
@@ -288,7 +288,7 @@ int swsusp_suspend(void)
 	 * that suspended with irqs off ... no overall powerup.
 	 */
 	device_power_up();
-Enable_irqs:
+ Enable_irqs:
 	local_irq_enable();
 	return error;
 }
Index: linux-2.6.19-rc5-mm2/kernel/power/user.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/user.c
+++ linux-2.6.19-rc5-mm2/kernel/power/user.c
@@ -313,7 +313,7 @@ static int snapshot_ioctl(struct inode *
 		if (pm_ops->finish)
 			pm_ops->finish(PM_SUSPEND_MEM);
 
-OutS3:
+ OutS3:
 		up(&pm_sem);
 		break;
 
Index: linux-2.6.19-rc5-mm2/kernel/power/swap.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/swap.c
+++ linux-2.6.19-rc5-mm2/kernel/power/swap.c
@@ -301,7 +301,7 @@ static int swap_write_page(struct swap_m
 		handle->cur_swap = offset;
 		handle->k = 0;
 	}
-out:
+ out:
 	return error;
 }
 
@@ -429,7 +429,7 @@ int swsusp_write(void)
 	if (error)
 		free_all_swap_pages(root_swap, handle.bitmap);
 	release_swap_writer(&handle);
-out:
+ out:
 	swsusp_close();
 	return error;
 }

Re: [PATCH 1/4] swsusp: Untangle thaw_processes

[Nigel Cunningham]

Hi.

On Sat, 2006-11-18 at 23:47 +0100, Rafael J. Wysocki wrote:
> Move the loop from thaw_processes() to a separate function and call it
> independently for kernel threads and user space processes so that the order
> of thawing tasks is clearly visible.
> 
> Drop thaw_kernel_threads() which is never used.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Ack.

Nigel

Re: [PATCH 4/4] swsusp: Fix labels

[Nigel Cunningham]

Hi.

On Sat, 2006-11-18 at 23:51 +0100, Rafael J. Wysocki wrote:
> Move all labels in the swsusp code to the second column, so that they won't
> fool diff -p.

This sounds like working around brokenness in diff -p. Should/could a
patch be submitted to the diff maintainer instead?

Regards,

Nigel

Re: [PATCH 4/4] swsusp: Fix labels

[Rafael J. Wysocki]

On Sunday, 19 November 2006 00:06, Nigel Cunningham wrote:
> Hi.
> 
> On Sat, 2006-11-18 at 23:51 +0100, Rafael J. Wysocki wrote:
> > Move all labels in the swsusp code to the second column, so that they won't
> > fool diff -p.
> 
> This sounds like working around brokenness in diff -p. Should/could a
> patch be submitted to the diff maintainer instead?

No.  This feature of diff is actually documented.

There was a discussion on LKML about it some time ago and the patch follows
the conclusion.

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

Re: [PATCH 4/4] swsusp: Fix labels

[Pavel Machek]

On Sat 2006-11-18 23:51:01, Rafael J. Wysocki wrote:
> Move all labels in the swsusp code to the second column, so that they won't
> fool diff -p.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/4] swsusp: Untangle thaw_processes

[Pavel Machek]

Hi!

> Move the loop from thaw_processes() to a separate function and call it
> independently for kernel threads and user space processes so that the order
> of thawing tasks is clearly visible.
> 
> Drop thaw_kernel_threads() which is never used.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/freezer.h |    6 -----
>  kernel/power/process.c  |   55 +++++++++++++++++++++++++++---------------------
>  2 files changed, 33 insertions(+), 28 deletions(-)
> 
> Index: linux-2.6.19-rc5-mm2/include/linux/freezer.h
> ===================================================================
> --- linux-2.6.19-rc5-mm2.orig/include/linux/freezer.h
> +++ linux-2.6.19-rc5-mm2/include/linux/freezer.h
> @@ -1,8 +1,5 @@
>  /* Freezer declarations */
>  
> -#define FREEZER_KERNEL_THREADS 0
> -#define FREEZER_ALL_THREADS 1
> -
>  #ifdef CONFIG_PM
>  /*
>   * Check if a process has been frozen
> @@ -60,8 +57,7 @@ static inline void frozen_process(struct
>  
>  extern void refrigerator(void);
>  extern int freeze_processes(void);
> -#define thaw_processes() do { thaw_some_processes(FREEZER_ALL_THREADS); } while(0)
> -#define thaw_kernel_threads() do { thaw_some_processes(FREEZER_KERNEL_THREADS); } while(0)
> +extern void thaw_processes(void);
>  
>  static inline int try_to_freeze(void)
>  {
> Index: linux-2.6.19-rc5-mm2/kernel/power/process.c
> ===================================================================
> --- linux-2.6.19-rc5-mm2.orig/kernel/power/process.c
> +++ linux-2.6.19-rc5-mm2/kernel/power/process.c
> @@ -20,6 +20,8 @@
>   */
>  #define TIMEOUT	(20 * HZ)
>  
> +#define FREEZER_KERNEL_THREADS 0
> +#define FREEZER_USER_SPACE 1

The variable is named "is_user_space"... so maybe the defines are not
strictly needed?

> +	do_each_thread(g, p) {
> +		if (!freezeable(p))
> +			continue;
>  
> +		if (is_user_space(p)) {
> +			if (!thaw_user_space)
> +				continue;
> +		} else {
> +			if (thaw_user_space)
> +				continue;
> +		}

if (is_user_space(p) != thaw_user_space)
	continue;

?

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/4] swsusp: Fix coding style in suspend.c

[Pavel Machek]

On Sat 2006-11-18 23:48:35, Rafael J. Wysocki wrote:
> Fix coding style in suspend.c.

ACK.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/4] swsusp: Untangle freeze_processes

[Pavel Machek]

Hi!

> Move the loop from freeze_processes() to a separate function and call it
> independently for user space processes and kernel threads so that the order of
> freezing tasks is clearly visible.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/power/process.c |   88 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 58 insertions(+), 30 deletions(-)
>

>  		do_each_thread(g, p) {
> +			if (is_user_space(p)) {
> +				if(!freeze_user_space)
> +					continue;
> +			} else {
> +				if(freeze_user_space)
> +					continue;
> +			}

Needs space between if and (, but lets use if (is_user_space() !=
freeze_user_space) trick here, too. 

> +/**
> + *	freeze_processes - tell processes to enter the refrigerator
> + *
> + *	Returns 0 on success, or the number of processes that didn't freeze,
> + *	although they were told to.
> + */

Above you point out to the broken calling convention. Perhaps this is
great time to fix it, too? Hmm, or maybe not. Patch looks good,
otherwise.

							Pavel
> +int freeze_processes(void)
> +{
> +	unsigned int nr_unfrozen;
> +
> +	printk("Stopping tasks ... ");
> +	nr_unfrozen = try_to_freeze_tasks(FREEZER_USER_SPACE);
> +	if (nr_unfrozen)
> +		return nr_unfrozen;
> +
> +	sys_sync();
> +	nr_unfrozen = try_to_freeze_tasks(FREEZER_KERNEL_THREADS);
> +	if (nr_unfrozen)
> +		return nr_unfrozen;
> +
>  	printk("done.\n");
>  	BUG_ON(in_atomic());
>  	return 0;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/4] swsusp: Untangle thaw_processes

[Rafael J. Wysocki]

Hi,

On Sunday, 19 November 2006 03:04, Pavel Machek wrote:
> Hi!
> 
> > Move the loop from thaw_processes() to a separate function and call it
> > independently for kernel threads and user space processes so that the order
> > of thawing tasks is clearly visible.
> > 
> > Drop thaw_kernel_threads() which is never used.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  include/linux/freezer.h |    6 -----
> >  kernel/power/process.c  |   55 +++++++++++++++++++++++++++---------------------
> >  2 files changed, 33 insertions(+), 28 deletions(-)
> > 
> > Index: linux-2.6.19-rc5-mm2/include/linux/freezer.h
> > ===================================================================
> > --- linux-2.6.19-rc5-mm2.orig/include/linux/freezer.h
> > +++ linux-2.6.19-rc5-mm2/include/linux/freezer.h
> > @@ -1,8 +1,5 @@
> >  /* Freezer declarations */
> >  
> > -#define FREEZER_KERNEL_THREADS 0
> > -#define FREEZER_ALL_THREADS 1
> > -
> >  #ifdef CONFIG_PM
> >  /*
> >   * Check if a process has been frozen
> > @@ -60,8 +57,7 @@ static inline void frozen_process(struct
> >  
> >  extern void refrigerator(void);
> >  extern int freeze_processes(void);
> > -#define thaw_processes() do { thaw_some_processes(FREEZER_ALL_THREADS); } while(0)
> > -#define thaw_kernel_threads() do { thaw_some_processes(FREEZER_KERNEL_THREADS); } while(0)
> > +extern void thaw_processes(void);
> >  
> >  static inline int try_to_freeze(void)
> >  {
> > Index: linux-2.6.19-rc5-mm2/kernel/power/process.c
> > ===================================================================
> > --- linux-2.6.19-rc5-mm2.orig/kernel/power/process.c
> > +++ linux-2.6.19-rc5-mm2/kernel/power/process.c
> > @@ -20,6 +20,8 @@
> >   */
> >  #define TIMEOUT	(20 * HZ)
> >  
> > +#define FREEZER_KERNEL_THREADS 0
> > +#define FREEZER_USER_SPACE 1
> 
> The variable is named "is_user_space"... so maybe the defines are not
> strictly needed?

Not strlctly, but if I see

	thaw_tasks(FREEZER_KERNEL_THREADS);
	thaw_tasks(FREEZER_USER_SPACE);

I can figure out what's going on even without looking at thaw_tasks().

> > +	do_each_thread(g, p) {
> > +		if (!freezeable(p))
> > +			continue;
> >  
> > +		if (is_user_space(p)) {
> > +			if (!thaw_user_space)
> > +				continue;
> > +		} else {
> > +			if (thaw_user_space)
> > +				continue;
> > +		}
> 
> if (is_user_space(p) != thaw_user_space)
> 	continue;

Ah, good idea.  In fact I prefer if (is_user_space(p) == !thaw_user_space),
because it will work even if thaw_user_space is different to 1 and 0.

Revised patch follows.

---
Move the loop from thaw_processes() to a separate function and call it
independently for kernel threads and user space processes so that the order
of thawing tasks is clearly visible.

Drop thaw_kernel_threads() which is never used.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |    6 -----
 kernel/power/process.c  |   53 ++++++++++++++++++++++++++----------------------
 2 files changed, 30 insertions(+), 29 deletions(-)

Index: linux-2.6.19-rc5-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.19-rc5-mm2.orig/include/linux/freezer.h
+++ linux-2.6.19-rc5-mm2/include/linux/freezer.h
@@ -1,8 +1,5 @@
 /* Freezer declarations */
 
-#define FREEZER_KERNEL_THREADS 0
-#define FREEZER_ALL_THREADS 1
-
 #ifdef CONFIG_PM
 /*
  * Check if a process has been frozen
@@ -60,8 +57,7 @@ static inline void frozen_process(struct
 
 extern void refrigerator(void);
 extern int freeze_processes(void);
-#define thaw_processes() do { thaw_some_processes(FREEZER_ALL_THREADS); } while(0)
-#define thaw_kernel_threads() do { thaw_some_processes(FREEZER_KERNEL_THREADS); } while(0)
+extern void thaw_processes(void);
 
 static inline int try_to_freeze(void)
 {
Index: linux-2.6.19-rc5-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/process.c
+++ linux-2.6.19-rc5-mm2/kernel/power/process.c
@@ -20,6 +20,8 @@
  */
 #define TIMEOUT	(20 * HZ)
 
+#define FREEZER_KERNEL_THREADS 0
+#define FREEZER_USER_SPACE 1
 
 static inline int freezeable(struct task_struct * p)
 {
@@ -79,6 +81,11 @@ static void cancel_freezing(struct task_
 	}
 }
 
+static inline int is_user_space(struct task_struct *p)
+{
+	return p->mm && !(p->flags & PF_BORROWED_MM);
+}
+
 /* 0 = success, else # of processes that we failed to stop */
 int freeze_processes(void)
 {
@@ -103,10 +110,9 @@ int freeze_processes(void)
 				cancel_freezing(p);
 				continue;
 			}
-			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 (is_user_space(p)) {
+				/* Freeze the task unless there is a vfork
+				 * completion pending
 				 */
 				if (!p->vfork_done)
 					freeze_process(p);
@@ -155,31 +161,30 @@ int freeze_processes(void)
 	return 0;
 }
 
-void thaw_some_processes(int all)
+static void thaw_tasks(int thaw_user_space)
 {
 	struct task_struct *g, *p;
-	int pass = 0; /* Pass 0 = Kernel space, 1 = Userspace */
 
-	printk("Restarting tasks... ");
 	read_lock(&tasklist_lock);
-	do {
-		do_each_thread(g, p) {
-			/*
-			 * is_user = 0 if kernel thread or borrowed mm,
-			 * 1 otherwise.
-			 */
-			int is_user = !!(p->mm && !(p->flags & PF_BORROWED_MM));
-			if (!freezeable(p) || (is_user != pass))
-				continue;
-			if (!thaw_process(p))
-				printk(KERN_INFO
-					"Strange, %s not stopped\n", p->comm);
-		} while_each_thread(g, p);
-
-		pass++;
-	} while (pass < 2 && all);
-
+	do_each_thread(g, p) {
+		if (!freezeable(p))
+			continue;
+
+		if (is_user_space(p) == !thaw_user_space)
+			continue;
+
+		if (!thaw_process(p))
+			printk(KERN_WARNING " Strange, %s not stopped\n",
+				p->comm );
+	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
+}
+
+void thaw_processes(void)
+{
+	printk("Restarting tasks ... ");
+	thaw_tasks(FREEZER_KERNEL_THREADS);
+	thaw_tasks(FREEZER_USER_SPACE);
 	schedule();
 	printk("done.\n");
 }

Re: [PATCH 2/4] swsusp: Untangle freeze_processes

[Rafael J. Wysocki]

Hi,

On Sunday, 19 November 2006 03:09, Pavel Machek wrote:
> Hi!
> 
> > Move the loop from freeze_processes() to a separate function and call it
> > independently for user space processes and kernel threads so that the order of
> > freezing tasks is clearly visible.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/power/process.c |   88 ++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 58 insertions(+), 30 deletions(-)
> >
> 
> >  		do_each_thread(g, p) {
> > +			if (is_user_space(p)) {
> > +				if(!freeze_user_space)
> > +					continue;
> > +			} else {
> > +				if(freeze_user_space)
> > +					continue;
> > +			}
> 
> Needs space between if and (, but lets use if (is_user_space() !=
> freeze_user_space) trick here, too.

OK

New version of the patch follows.

---
Move the loop from freeze_processes() to a separate function and call it
independently for user space processes and kernel threads so that the order of
freezing tasks is clearly visible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/process.c |   84 +++++++++++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 30 deletions(-)

Index: linux-2.6.19-rc5-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc5-mm2.orig/kernel/power/process.c
+++ linux-2.6.19-rc5-mm2/kernel/power/process.c
@@ -86,24 +86,23 @@ static inline int is_user_space(struct t
 	return p->mm && !(p->flags & PF_BORROWED_MM);
 }
 
-/* 0 = success, else # of processes that we failed to stop */
-int freeze_processes(void)
+static unsigned int try_to_freeze_tasks(int freeze_user_space)
 {
-	int todo, nr_user, user_frozen;
-	unsigned long start_time;
 	struct task_struct *g, *p;
+	unsigned long end_time;
+	unsigned int todo;
 
-	printk("Stopping tasks... ");
-	start_time = jiffies;
-	user_frozen = 0;
+	end_time = jiffies + TIMEOUT;
 	do {
-		nr_user = todo = 0;
+		todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			if (!freezeable(p))
 				continue;
+
 			if (frozen(p))
 				continue;
+
 			if (p->state == TASK_TRACED &&
 			    (frozen(p->parent) ||
 			     p->parent->state == TASK_STOPPED)) {
@@ -111,51 +110,76 @@ int freeze_processes(void)
 				continue;
 			}
 			if (is_user_space(p)) {
+				if (!freeze_user_space)
+					continue;
+
 				/* Freeze the task unless there is 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++;
+				if (freeze_user_space)
+					continue;
+
+				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))
+		if (todo && time_after(jiffies, end_time))
 			break;
-	} while(todo);
+	} while (todo);
 
-	/* 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) {
+		/* 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.
+		 */
 		printk("\n");
-		printk(KERN_ERR "Stopping tasks timed out "
-			"after %d seconds (%d tasks remaining):\n",
-			TIMEOUT / HZ, todo);
+		printk(KERN_ERR "Stopping %s timed out after %d seconds "
+				"(%d tasks refusing to freeze):\n",
+				freeze_user_space ? "user space processes" :
+					"kernel threads",
+				TIMEOUT / HZ, todo);
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
+			if (is_user_space(p) == !freeze_user_space)
+				continue;
+
 			if (freezeable(p) && !frozen(p))
 				printk(KERN_ERR " %s\n", p->comm);
+
 			cancel_freezing(p);
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
-		return todo;
 	}
 
+	return todo;
+}
+
+/**
+ *	freeze_processes - tell processes to enter the refrigerator
+ *
+ *	Returns 0 on success, or the number of processes that didn't freeze,
+ *	although they were told to.
+ */
+int freeze_processes(void)
+{
+	unsigned int nr_unfrozen;
+
+	printk("Stopping tasks ... ");
+	nr_unfrozen = try_to_freeze_tasks(FREEZER_USER_SPACE);
+	if (nr_unfrozen)
+		return nr_unfrozen;
+
+	sys_sync();
+	nr_unfrozen = try_to_freeze_tasks(FREEZER_KERNEL_THREADS);
+	if (nr_unfrozen)
+		return nr_unfrozen;
+
 	printk("done.\n");
 	BUG_ON(in_atomic());
 	return 0;

Re: [PATCH 1/4] swsusp: Untangle thaw_processes

[Pavel Machek]

Hi!

> Ah, good idea.  In fact I prefer if (is_user_space(p) == !thaw_user_space),
> because it will work even if thaw_user_space is different to 1 and 0.
> 
> Revised patch follows.
> 
> ---
> Move the loop from thaw_processes() to a separate function and call it
> independently for kernel threads and user space processes so that the order
> of thawing tasks is clearly visible.
> 
> Drop thaw_kernel_threads() which is never used.

ACK.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/4] swsusp: Untangle freeze_processes

[Pavel Machek]

Hi!

> > Needs space between if and (, but lets use if (is_user_space() !=
> > freeze_user_space) trick here, too.
> 
> OK
> 
> New version of the patch follows.
> 
> ---
> Move the loop from freeze_processes() to a separate function and call it
> independently for user space processes and kernel threads so that the order of
> freezing tasks is clearly visible.

ACK.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 4/4] swsusp: Fix labels

[Randy Dunlap]

On Sun, 19 Nov 2006 00:25:06 +0100 Rafael J. Wysocki wrote:

> On Sunday, 19 November 2006 00:06, Nigel Cunningham wrote:
> > Hi.
> > 
> > On Sat, 2006-11-18 at 23:51 +0100, Rafael J. Wysocki wrote:
> > > Move all labels in the swsusp code to the second column, so that they won't
> > > fool diff -p.
> > 
> > This sounds like working around brokenness in diff -p. Should/could a
> > patch be submitted to the diff maintainer instead?
> 
> No.  This feature of diff is actually documented.
> 
> There was a discussion on LKML about it some time ago and the patch follows
> the conclusion.

There was discussion, and then both Jesper and I tried to
reproduce the problem with diff and could not do so.
Maybe it has already been fixed. (?)

---
~Randy

Re: [PATCH 4/4] swsusp: Fix labels

[Rafael J. Wysocki]

On Wednesday, 22 November 2006 23:32, Randy Dunlap wrote:
> On Sun, 19 Nov 2006 00:25:06 +0100 Rafael J. Wysocki wrote:
> 
> > On Sunday, 19 November 2006 00:06, Nigel Cunningham wrote:
> > > Hi.
> > > 
> > > On Sat, 2006-11-18 at 23:51 +0100, Rafael J. Wysocki wrote:
> > > > Move all labels in the swsusp code to the second column, so that they won't
> > > > fool diff -p.
> > > 
> > > This sounds like working around brokenness in diff -p. Should/could a
> > > patch be submitted to the diff maintainer instead?
> > 
> > No.  This feature of diff is actually documented.
> > 
> > There was a discussion on LKML about it some time ago and the patch follows
> > the conclusion.
> 
> There was discussion, and then both Jesper and I tried to
> reproduce the problem with diff and could not do so.
> Maybe it has already been fixed. (?)

Well, I'm not sure.

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller