[RFC 0/2] Fix Freezer related races.

[RFC 0/2] Fix Freezer related races.

[Gautham R Shenoy]

Hello Everyone, 
Oleg had pointed out some subtle races which could lead to the failure
of the process freezer in the patchset which I had posted earlier.

A couple of these problems seem to be present in the latest -mm(2.6.21-rc6-mm1).

This is an attempt to fix them.

Awaiting you feedback, 
Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

[RFC PATCH 1/2] Fix PF_NOFREEZE and freezeable race

[Gautham R Shenoy]

This patch fixes the race pointed out by Oleg Nesterov.

* Freezer marks a thread as freezeable. 
* The thread now marks itself PF_NOFREEZE causing it to
  freeze on calling try_to_freeze(). Thus the task is frozen, even though
  it doesn't want to.
* Subsequent thaw_processes() will also fail to thaw the task since it is 
  marked PF_NOFREEZE.

Avoid this problem by checking the current task's PF_NOFREEZE status in the 
refrigerator before marking current as frozen.

Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
 kernel/power/process.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-2.6.21-rc6/kernel/power/process.c
===================================================================
--- linux-2.6.21-rc6.orig/kernel/power/process.c
+++ linux-2.6.21-rc6/kernel/power/process.c
@@ -41,6 +41,15 @@ void refrigerator(void)
 
 	task_lock(current);
 	if (freezing(current)) {
+		/* check if we had marked ourself PF_NOFREEZE
+		 * *after* the freezer did the freezeable() check
+		 * on us.
+		 */
+		if (current->flags & PF_NOFREEZE) {
+			clear_tsk_thread_flag(current, TIF_FREEZE);
+			task_unlock(current);
+			return;
+		}
 		frozen_process(current);
 		task_unlock(current);
 	} else {
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

[RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Gautham R Shenoy]

Threads which wait for completion on a frozen thread might result in 
causing the freezer to fail, if the waiting thread is freezeable.

There are some well known cases where it's preferable to temporarily thaw
the frozen process, finish the wait for completion and allow both the 
processes to call try_to_freeze. 

kthread_stop is one such case. flush_workqueue might be another. 

This patch attempts to address such a situation with a fix for kthread_stop.

Strictly experimental. Compile tested on i386.

Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>

---
 include/asm-arm/thread_info.h      |    2 
 include/asm-blackfin/thread_info.h |    4 +
 include/asm-frv/thread_info.h      |    4 +
 include/asm-i386/thread_info.h     |    4 +
 include/asm-ia64/thread_info.h     |    4 +
 include/asm-mips/thread_info.h     |    2 
 include/asm-powerpc/thread_info.h  |    4 +
 include/asm-sh/thread_info.h       |    2 
 include/asm-x86_64/thread_info.h   |    4 +
 include/linux/freezer.h            |    4 +
 kernel/kthread.c                   |    4 +
 kernel/power/process.c             |   81 ++++++++++++++++++++++++++++++++++++-
 12 files changed, 118 insertions(+), 1 deletion(-)

Index: linux-2.6.21-rc6/kernel/power/process.c
===================================================================
--- linux-2.6.21-rc6.orig/kernel/power/process.c
+++ linux-2.6.21-rc6/kernel/power/process.c
@@ -23,6 +23,16 @@
 #define FREEZER_KERNEL_THREADS 0
 #define FREEZER_USER_SPACE 1
 
+struct freezer_status_struct {
+	spinlock_t lock;
+	int count;
+};
+
+static struct freezer_status_struct freezer_status = {
+				.lock = SPIN_LOCK_UNLOCKED,
+				.count = 0,
+			};
+
 static inline int freezeable(struct task_struct * p)
 {
 	if ((p == current) ||
@@ -45,7 +55,8 @@ void refrigerator(void)
 		 * *after* the freezer did the freezeable() check
 		 * on us.
 		 */
-		if (current->flags & PF_NOFREEZE) {
+		if ((current->flags & PF_NOFREEZE) ||
+		    test_tsk_thread_flag(current, TIF_FREEZER_HELD)) {
 			clear_tsk_thread_flag(current, TIF_FREEZE);
 			task_unlock(current);
 			return;
@@ -63,12 +74,16 @@ void refrigerator(void)
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(&current->sighand->siglock);
 
+	task_lock(current);
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (!frozen(current))
 			break;
+		task_unlock(current);
 		schedule();
+		task_lock(current);
 	}
+	task_unlock(current);
 	pr_debug("%s left refrigerator\n", current->comm);
 	current->state = save;
 }
@@ -114,6 +129,47 @@ static inline int is_user_space(struct t
 	return ret;
 }
 
+/*
+ * Delay the freezer from declaring the system as frozen,
+ * if it is not frozen already.
+ *
+ * Usage:
+ * int result = hold_freezer_for_task(p);
+ * wait_for_completion(something_which_p_completes);
+ * release_freezer(p, result);
+ */
+
+int hold_freezer_for_task(struct task_struct *p)
+{
+	int ret = 0;
+	spin_lock(&freezer_status.lock);
+	if (freezer_status.count >= 0)
+	{
+		set_tsk_thread_flag(p, TIF_FREEZER_HELD);
+		thaw_process(p);
+		freezer_status.count++;
+		ret = 1;
+	}
+	spin_unlock(&freezer_status.lock);
+
+	return ret;
+}
+
+/*
+ * Allow freezer to function normally as before.
+ * Usage: See the comment above definition of hold_freezer_for_task()
+ */
+void release_freezer(struct task_struct *p, int result)
+{
+	if (result) {
+		spin_lock(&freezer_status.lock);
+		BUG_ON(freezer_status.count <= 0);
+		clear_tsk_thread_flag(p, TIF_FREEZER_HELD);
+		freezer_status.count--;
+		spin_unlock(&freezer_status.lock);
+	}
+
+}
 static unsigned int try_to_freeze_tasks(int freeze_user_space)
 {
 	struct task_struct *g, *p;
@@ -146,6 +202,22 @@ static unsigned int try_to_freeze_tasks(
 		yield();			/* Yield is okay here */
 		if (todo && time_after(jiffies, end_time))
 			break;
+
+		if (!freeze_user_space && !todo) {
+			spin_lock(&freezer_status.lock);
+			if (freezer_status.count == 0)
+				freezer_status.count--;
+			else {
+				spin_unlock(&freezer_status.lock);
+				/* check once more for any unfrozen
+				 * tasks. someone might have thawed
+				 * a task temporarily.
+				 */
+				continue;
+			}
+			spin_unlock(&freezer_status.lock);
+		}
+
 	} while (todo);
 
 	if (todo) {
@@ -219,6 +291,13 @@ static void thaw_tasks(int thaw_user_spa
 		thaw_process(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
+
+	if (thaw_user_space) {
+		spin_lock(&freezer_status.lock);
+			if (freezer_status.count < 0)
+				freezer_status.count++;
+		spin_unlock(&freezer_status.lock);
+	}
 }
 
 void thaw_processes(void)
Index: linux-2.6.21-rc6/include/linux/freezer.h
===================================================================
--- linux-2.6.21-rc6.orig/include/linux/freezer.h
+++ linux-2.6.21-rc6/include/linux/freezer.h
@@ -65,6 +65,8 @@ static inline void frozen_process(struct
 extern void refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
+extern int hold_freezer_for_task(struct task_struct *p);
+extern void release_freezer(struct task_struct *p, int result);
 
 static inline int try_to_freeze(void)
 {
@@ -125,6 +127,8 @@ static inline int freezing(struct task_s
 static inline void freeze(struct task_struct *p) { BUG(); }
 static inline int thaw_process(struct task_struct *p) { return 1; }
 static inline void frozen_process(struct task_struct *p) { BUG(); }
+static inline int hold_freezer_for_task(struct task_struct *p) { return 0;}
+static inline void release_freezer(struct task_struct *p, int result) {}
 
 static inline void refrigerator(void) {}
 static inline int freeze_processes(void) { BUG(); return 0; }
Index: linux-2.6.21-rc6/kernel/kthread.c
===================================================================
--- linux-2.6.21-rc6.orig/kernel/kthread.c
+++ linux-2.6.21-rc6/kernel/kthread.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <asm/semaphore.h>
+#include <linux/freezer.h>
 
 /*
  * We dont want to execute off keventd since it might
@@ -220,6 +221,7 @@ EXPORT_SYMBOL(kthread_bind);
 int kthread_stop(struct task_struct *k)
 {
 	int ret;
+	int freezer_is_held;
 
 	mutex_lock(&kthread_stop_lock);
 
@@ -236,7 +238,9 @@ int kthread_stop(struct task_struct *k)
 	put_task_struct(k);
 
 	/* Once it dies, reset stop ptr, gather result and we're done. */
+	freezer_is_held = hold_freezer_for_task(k);
 	wait_for_completion(&kthread_stop_info.done);
+	release_freezer(k, freezer_is_held);
 	kthread_stop_info.k = NULL;
 	ret = kthread_stop_info.err;
 	mutex_unlock(&kthread_stop_lock);
Index: linux-2.6.21-rc6/include/asm-arm/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-arm/thread_info.h
+++ linux-2.6.21-rc6/include/asm-arm/thread_info.h
@@ -148,6 +148,7 @@ extern void iwmmxt_task_switch(struct th
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
+#define TIF_FREEZER_HELD	20
 
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -156,6 +157,7 @@ extern void iwmmxt_task_switch(struct th
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1 << TIF_FREEZER_HELD)
 
 /*
  * Change these and you break ASM code in entry-common.S
Index: linux-2.6.21-rc6/include/asm-blackfin/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-blackfin/thread_info.h
+++ linux-2.6.21-rc6/include/asm-blackfin/thread_info.h
@@ -127,6 +127,9 @@ static inline struct thread_info *curren
 #define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
 #define TIF_FREEZE              7       /* is freezing for suspend */
 #define TIF_SINGLESTEP		8       /* restore singlestep on return to user mode */
+#define TIF_FREEZER_HELD	9	/* Thread is temporarily holding up
+					 * the process freezer
+					 */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -137,6 +140,7 @@ static inline struct thread_info *curren
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_FREEZE             (1<<TIF_FREEZE)
 #define _TIF_SINGLESTEP		(1<<TIF_SINGLESTEP)
+#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)
 
 #define _TIF_WORK_MASK		0x0000FFFE	/* work to do on interrupt/exception return */
 
Index: linux-2.6.21-rc6/include/asm-frv/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-frv/thread_info.h
+++ linux-2.6.21-rc6/include/asm-frv/thread_info.h
@@ -117,6 +117,9 @@ register struct thread_info *__current_t
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		17	/* OOM killer killed process */
 #define TIF_FREEZE		18	/* freezing for suspend */
+#define TIF_FREEZER_HELD	19	/* Thread is holding up freezer
+					 * temporarily.
+					 */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
@@ -127,6 +130,7 @@ register struct thread_info *__current_t
 #define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1 << TIF_FREEZER_HELD)
 
 #define _TIF_WORK_MASK		0x0000FFFE	/* work to do on interrupt/exception return */
 #define _TIF_ALLWORK_MASK	0x0000FFFF	/* work to do on any return to u-space */
Index: linux-2.6.21-rc6/include/asm-i386/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-i386/thread_info.h
+++ linux-2.6.21-rc6/include/asm-i386/thread_info.h
@@ -137,6 +137,9 @@ static inline struct thread_info *curren
 #define TIF_IO_BITMAP		18	/* uses I/O bitmap */
 #define TIF_FREEZE		19	/* is freezing for suspend */
 #define TIF_FORCED_TF		20	/* true if TF in eflags artificially */
+#define TIF_FREEZER_HELD	21	/* is temporarily holding up process
+					 * freezer
+					 */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -151,6 +154,7 @@ static inline struct thread_info *curren
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
 #define _TIF_FORCED_TF		(1<<TIF_FORCED_TF)
+#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
Index: linux-2.6.21-rc6/include/asm-ia64/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-ia64/thread_info.h
+++ linux-2.6.21-rc6/include/asm-ia64/thread_info.h
@@ -90,6 +90,9 @@ struct thread_info {
 #define TIF_MCA_INIT		18	/* this task is processing MCA or INIT */
 #define TIF_DB_DISABLED		19	/* debug trap disabled for fsyscall */
 #define TIF_FREEZE		20	/* is freezing for suspend */
+#define TIF_FREEZER_HELD	21	/* is temporarily holding up the
+					 * process freezer
+					 */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
@@ -102,6 +105,7 @@ struct thread_info {
 #define _TIF_MCA_INIT		(1 << TIF_MCA_INIT)
 #define _TIF_DB_DISABLED	(1 << TIF_DB_DISABLED)
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1 << TIF_FREEZER_HELD)
 
 /* "work to do on user-return" bits */
 #define TIF_ALLWORK_MASK	(_TIF_NOTIFY_RESUME|_TIF_SIGPENDING|_TIF_NEED_RESCHED|_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)
Index: linux-2.6.21-rc6/include/asm-mips/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-mips/thread_info.h
+++ linux-2.6.21-rc6/include/asm-mips/thread_info.h
@@ -120,6 +120,7 @@ register struct thread_info *__current_t
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
 #define TIF_ALLOW_FP_IN_KERNEL	20
+#define TIF_FREEZER_HELD	21
 #define TIF_SYSCALL_TRACE	31	/* syscall trace active */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -132,6 +133,7 @@ register struct thread_info *__current_t
 #define _TIF_USEDFPU		(1<<TIF_USEDFPU)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK		(0x0000ffef & ~_TIF_SECCOMP)
Index: linux-2.6.21-rc6/include/asm-powerpc/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-powerpc/thread_info.h
+++ linux-2.6.21-rc6/include/asm-powerpc/thread_info.h
@@ -123,6 +123,9 @@ static inline struct thread_info *curren
 #define TIF_NOERROR		14	/* Force successful syscall return */
 #define TIF_RESTORE_SIGMASK	15	/* Restore signal mask in do_signal */
 #define TIF_FREEZE		16	/* Freezing for suspend */
+#define TIF_FREEZER_HELD	17	/* Is temporarily holding up the
+					 * process freezer.
+					 */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -140,6 +143,7 @@ static inline struct thread_info *curren
 #define _TIF_NOERROR		(1<<TIF_NOERROR)
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SECCOMP)
 
 #define _TIF_USER_WORK_MASK	(_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | \
Index: linux-2.6.21-rc6/include/asm-sh/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-sh/thread_info.h
+++ linux-2.6.21-rc6/include/asm-sh/thread_info.h
@@ -116,6 +116,7 @@ static inline struct thread_info *curren
 #define TIF_POLLING_NRFLAG	17	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
+#define TIF_FREEZER_HELD	20
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -126,6 +127,7 @@ static inline struct thread_info *curren
 #define _TIF_USEDFPU		(1<<TIF_USEDFPU)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)
 
 #define _TIF_WORK_MASK		0x000000FE	/* work to do on interrupt/exception return */
 #define _TIF_ALLWORK_MASK	0x000000FF	/* work to do on any return to u-space */
Index: linux-2.6.21-rc6/include/asm-x86_64/thread_info.h
===================================================================
--- linux-2.6.21-rc6.orig/include/asm-x86_64/thread_info.h
+++ linux-2.6.21-rc6/include/asm-x86_64/thread_info.h
@@ -123,6 +123,9 @@ static inline struct thread_info *stack_
 #define TIF_DEBUG		21	/* uses debug registers */
 #define TIF_IO_BITMAP		22	/* uses I/O bitmap */
 #define TIF_FREEZE		23	/* is freezing for suspend */
+#define TIF_FREEZER_HELD	24	/* is temporarily holding up the
+					 * process freezer
+					 */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -140,6 +143,7 @@ static inline struct thread_info *stack_
 #define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Andrew Morton]

On Thu, 19 Apr 2007 17:34:19 +0530
Gautham R Shenoy <ego@in.ibm.com> wrote:

> Threads which wait for completion on a frozen thread might result in 
> causing the freezer to fail, if the waiting thread is freezeable.
> 
> There are some well known cases where it's preferable to temporarily thaw
> the frozen process, finish the wait for completion and allow both the 
> processes to call try_to_freeze. 
> 
> kthread_stop is one such case.

hm.

> flush_workqueue might be another. 

flush_workqueue() just needs to die.  I think there are (almost) no
legitimate users of it once cancel_work_sync() is merged.

> This patch attempts to address such a situation with a fix for kthread_stop.

Via wholly undescribed means :(

> Strictly experimental. Compile tested on i386.

Rather than doing <whatever you did>, perhaps we could make the freezing
process a dual-pass thing.  On pass 1, mark all the threads as "we'll be
freezing you soon" and on the second pass, do the actual freezing.  Then,
in problematic places such as kthread_stop() we can look to see if we'll
soon be asked to freeze and if so, run try_to_freeze().

Of course, running try_to_freeze() in kthread_stop() would be very wrong,
so we'd actually need to do it in callers, preferably via a new
kthread_stop_freezeable() wrapper.

And the two-pass-freeze thing is of course racy.  It's also unnecessary:
setting a flag on every task in the machine is equivalent to setting a
global variable.  So perhaps just use a global variable?

int kthread_stop_freezeable(struct task_struct *k)
{
	if (freeze_state == ABOUT_TO_START) {
		wait_for(freeze_state == STARTED);
		try_to_freeze();
	}
	kthread_stop(k);
}

which is theoretically racy if another freeze_processes() starts
immediately.  Anyway - please have a think about it ;)

> +static struct freezer_status_struct freezer_status = {
> +				.lock = SPIN_LOCK_UNLOCKED,
> +				.count = 0,
> +			};

SPIN_LOCK_UNLOCKED is deprecated (it subverts lockdep)

>  static inline int freezeable(struct task_struct * p)
>  {
>  	if ((p == current) ||
> @@ -45,7 +55,8 @@ void refrigerator(void)
>  		 * *after* the freezer did the freezeable() check
>  		 * on us.
>  		 */
> -		if (current->flags & PF_NOFREEZE) {
> +		if ((current->flags & PF_NOFREEZE) ||
> +		    test_tsk_thread_flag(current, TIF_FREEZER_HELD)) {
>  			clear_tsk_thread_flag(current, TIF_FREEZE);
>  			task_unlock(current);
>  			return;
> @@ -63,12 +74,16 @@ void refrigerator(void)
>  	recalc_sigpending(); /* We sent fake signal, clean it up */
>  	spin_unlock_irq(&current->sighand->siglock);
>  
> +	task_lock(current);
>  	for (;;) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
>  		if (!frozen(current))
>  			break;
> +		task_unlock(current);
>  		schedule();
> +		task_lock(current);
>  	}
> +	task_unlock(current);
>  	pr_debug("%s left refrigerator\n", current->comm);
>  	current->state = save;

I guess we should use set_current_state() here.

> +
> +	if (thaw_user_space) {
> +		spin_lock(&freezer_status.lock);
> +			if (freezer_status.count < 0)
> +				freezer_status.count++;
> +		spin_unlock(&freezer_status.lock);
> +	}
>  }

whitespace went wrong

> +#define TIF_FREEZER_HELD	21	/* is temporarily holding up the
> +					 * process freezer
> +					 */
>  
>  #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
>  #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
> @@ -102,6 +105,7 @@ struct thread_info {
>  #define _TIF_MCA_INIT		(1 << TIF_MCA_INIT)
>  #define _TIF_DB_DISABLED	(1 << TIF_DB_DISABLED)
>  #define _TIF_FREEZE		(1 << TIF_FREEZE)
> +#define _TIF_FREEZER_HELD	(1 << TIF_FREEZER_HELD)
>  
>  /* "work to do on user-return" bits */
>  #define TIF_ALLWORK_MASK	(_TIF_NOTIFY_RESUME|_TIF_SIGPENDING|_TIF_NEED_RESCHED|_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)
> Index: linux-2.6.21-rc6/include/asm-mips/thread_info.h
> ===================================================================
> --- linux-2.6.21-rc6.orig/include/asm-mips/thread_info.h
> +++ linux-2.6.21-rc6/include/asm-mips/thread_info.h
> @@ -120,6 +120,7 @@ register struct thread_info *__current_t
>  #define TIF_MEMDIE		18
>  #define TIF_FREEZE		19
>  #define TIF_ALLOW_FP_IN_KERNEL	20
> +#define TIF_FREEZER_HELD	21
>  #define TIF_SYSCALL_TRACE	31	/* syscall trace active */
>  
>  #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
> @@ -132,6 +133,7 @@ register struct thread_info *__current_t
>  #define _TIF_USEDFPU		(1<<TIF_USEDFPU)
>  #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
>  #define _TIF_FREEZE		(1<<TIF_FREEZE)
> +#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)

hm, all this duplication is unpleasing.  We could do something similar to
include/linux/buffer_head.h:BH_PrivateStart here: get all architectures to
define a TIF_COMMON_STUFF_STARTS_HERE then include asm-generic/whatever.h
which defines all the flags which every architecture must define, as
TIF_COMMON_STUFF_STARTS_HERE+0, TIF_COMMON_STUFF_STARTS_HERE+1, etc.

Re: [RFC PATCH 1/2] Fix PF_NOFREEZE and freezeable race

[Rafael J. Wysocki]

Hi,

On Thursday, 19 April 2007 14:02, Gautham R Shenoy wrote:
> This patch fixes the race pointed out by Oleg Nesterov.
> 
> * Freezer marks a thread as freezeable. 
> * The thread now marks itself PF_NOFREEZE causing it to
>   freeze on calling try_to_freeze(). Thus the task is frozen, even though
>   it doesn't want to.
> * Subsequent thaw_processes() will also fail to thaw the task since it is 
>   marked PF_NOFREEZE.
> 
> Avoid this problem by checking the current task's PF_NOFREEZE status in the 
> refrigerator before marking current as frozen.
> 
> Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>

Looks good, although I'm not sure if we don't need to call recalc_sigpending()
for tasks that turn out to be PF_NOFREEZE.

Greetings,
Rafael

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Rafael J. Wysocki]

On Thursday, 19 April 2007 23:31, Andrew Morton wrote:
> On Thu, 19 Apr 2007 17:34:19 +0530
> Gautham R Shenoy <ego@in.ibm.com> wrote:
> 
> > Threads which wait for completion on a frozen thread might result in 
> > causing the freezer to fail, if the waiting thread is freezeable.
> > 
> > There are some well known cases where it's preferable to temporarily thaw
> > the frozen process, finish the wait for completion and allow both the 
> > processes to call try_to_freeze. 
> > 
> > kthread_stop is one such case.
> 
> hm.
> 
> > flush_workqueue might be another. 
> 
> flush_workqueue() just needs to die.  I think there are (almost) no
> legitimate users of it once cancel_work_sync() is merged.
> 
> > This patch attempts to address such a situation with a fix for kthread_stop.
> 
> Via wholly undescribed means :(

Yeah, I have the same problem with it. :-)

> > Strictly experimental. Compile tested on i386.
> 
> Rather than doing <whatever you did>, perhaps we could make the freezing
> process a dual-pass thing.  On pass 1, mark all the threads as "we'll be
> freezing you soon" and on the second pass, do the actual freezing.  Then,
> in problematic places such as kthread_stop() we can look to see if we'll
> soon be asked to freeze and if so, run try_to_freeze().
> 
> Of course, running try_to_freeze() in kthread_stop() would be very wrong,
> so we'd actually need to do it in callers, preferably via a new
> kthread_stop_freezeable() wrapper.
> 
> And the two-pass-freeze thing is of course racy.  It's also unnecessary:
> setting a flag on every task in the machine is equivalent to setting a
> global variable.  So perhaps just use a global variable?
> 
> int kthread_stop_freezeable(struct task_struct *k)
> {
> 	if (freeze_state == ABOUT_TO_START) {
> 		wait_for(freeze_state == STARTED);
> 		try_to_freeze();
> 	}
> 	kthread_stop(k);
> }
> 
> which is theoretically racy if another freeze_processes() starts
> immediately.  Anyway - please have a think about it ;)

Hmm, can't we do something like this instead:

---
 kernel/kthread.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-2.6.21-rc7/kernel/kthread.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/kthread.c
+++ linux-2.6.21-rc7/kernel/kthread.c
@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 #include <asm/semaphore.h>
 
 /*
@@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!(current->flags & PF_NOFREEZE)) {
+		/* If we are freezable, the freezer will wait for us */
+		task_lock(k);
+		k->flags |= PF_NOFREEZE;
+		if (frozen(k))
+			k->flags &= ~PF_FROZEN;
+
+		task_unlock(k);
+	}
 	wake_up_process(k);
 	put_task_struct(k);
 

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Gautham R Shenoy]

On Thu, Apr 19, 2007 at 02:31:33PM -0700, Andrew Morton wrote:
> On Thu, 19 Apr 2007 17:34:19 +0530
> Gautham R Shenoy <ego@in.ibm.com> wrote:
> 
> 
> flush_workqueue() just needs to die.  I think there are (almost) no
> legitimate users of it once cancel_work_sync() is merged.
> 
> > This patch attempts to address such a situation with a fix for kthread_stop.
> 
> Via wholly undescribed means :(

Sorry. I will document stuff better next time.
> 
> > Strictly experimental. Compile tested on i386.
> 
> Rather than doing <whatever you did>, perhaps we could make the freezing
> process a dual-pass thing.  On pass 1, mark all the threads as "we'll be
> freezing you soon" and on the second pass, do the actual freezing.  Then,
> in problematic places such as kthread_stop() we can look to see if we'll
> soon be asked to freeze and if so, run try_to_freeze().

We can do that. Just that the freezer will now have to wait for 2 sets
of ack's instead of 1 set before declaring the system as frozen.

But the whole point of the patch was so that a thread A can tell
a thread B that it's dependent on the latter, and hence would like
to postpone B's freezing for some time. So I am thinking if we can
achieve this through the scheme you described.

> 
> Of course, running try_to_freeze() in kthread_stop() would be very wrong,
> so we'd actually need to do it in callers, preferably via a new
> kthread_stop_freezeable() wrapper.
> 

Well, even then we'll need to ensure that a thread would not call
kthread_stop_freezeable() with any locks held. That would mean more
audits :)

> And the two-pass-freeze thing is of course racy.  It's also unnecessary:
> setting a flag on every task in the machine is equivalent to setting a
> global variable.  So perhaps just use a global variable?
> 
> int kthread_stop_freezeable(struct task_struct *k)
> {
> 	if (freeze_state == ABOUT_TO_START) {
> 		wait_for(freeze_state == STARTED);
> 		try_to_freeze();
> 	}
> 	kthread_stop(k);
> }
> 
> which is theoretically racy if another freeze_processes() starts
> immediately.  Anyway - please have a think about it ;)
> 

Sure, am already thinking about it :)

> > +static struct freezer_status_struct freezer_status = {
> > +				.lock = SPIN_LOCK_UNLOCKED,
> > +				.count = 0,
> > +			};
> 
> SPIN_LOCK_UNLOCKED is deprecated (it subverts lockdep)
> 

Ok, will change it to __SPIN_LOCK_UNLOCKED(freezer_status.lock)

> >  static inline int freezeable(struct task_struct * p)
> >  {
> >  	if ((p == current) ||
> > @@ -45,7 +55,8 @@ void refrigerator(void)
> >  		 * *after* the freezer did the freezeable() check
> >  		 * on us.
> >  		 */
> > -		if (current->flags & PF_NOFREEZE) {
> > +		if ((current->flags & PF_NOFREEZE) ||
> > +		    test_tsk_thread_flag(current, TIF_FREEZER_HELD)) {
> >  			clear_tsk_thread_flag(current, TIF_FREEZE);
> >  			task_unlock(current);
> >  			return;
> > @@ -63,12 +74,16 @@ void refrigerator(void)
> >  	recalc_sigpending(); /* We sent fake signal, clean it up */
> >  	spin_unlock_irq(&current->sighand->siglock);
> >  
> > +	task_lock(current);
> >  	for (;;) {
> >  		set_current_state(TASK_UNINTERRUPTIBLE);
> >  		if (!frozen(current))
> >  			break;
> > +		task_unlock(current);
> >  		schedule();
> > +		task_lock(current);
> >  	}
> > +	task_unlock(current);
> >  	pr_debug("%s left refrigerator\n", current->comm);
> >  	current->state = save;
> 
> I guess we should use set_current_state() here.
> 
> > +
> > +	if (thaw_user_space) {
> > +		spin_lock(&freezer_status.lock);
> > +			if (freezer_status.count < 0)
> > +				freezer_status.count++;
> > +		spin_unlock(&freezer_status.lock);
> > +	}
> >  }
> 
> whitespace went wrong
>

Huh! yeah, dunno how though.

> > +#define TIF_FREEZER_HELD	21	/* is temporarily holding up the
> > +					 * process freezer
> > +					 */
> >  
> >  #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
> >  #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
> > @@ -102,6 +105,7 @@ struct thread_info {
> >  #define _TIF_MCA_INIT		(1 << TIF_MCA_INIT)
> >  #define _TIF_DB_DISABLED	(1 << TIF_DB_DISABLED)
> >  #define _TIF_FREEZE		(1 << TIF_FREEZE)
> > +#define _TIF_FREEZER_HELD	(1 << TIF_FREEZER_HELD)
> >  
> >  /* "work to do on user-return" bits */
> >  #define TIF_ALLWORK_MASK	(_TIF_NOTIFY_RESUME|_TIF_SIGPENDING|_TIF_NEED_RESCHED|_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)
> > Index: linux-2.6.21-rc6/include/asm-mips/thread_info.h
> > ===================================================================
> > --- linux-2.6.21-rc6.orig/include/asm-mips/thread_info.h
> > +++ linux-2.6.21-rc6/include/asm-mips/thread_info.h
> > @@ -120,6 +120,7 @@ register struct thread_info *__current_t
> >  #define TIF_MEMDIE		18
> >  #define TIF_FREEZE		19
> >  #define TIF_ALLOW_FP_IN_KERNEL	20
> > +#define TIF_FREEZER_HELD	21
> >  #define TIF_SYSCALL_TRACE	31	/* syscall trace active */
> >  
> >  #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
> > @@ -132,6 +133,7 @@ register struct thread_info *__current_t
> >  #define _TIF_USEDFPU		(1<<TIF_USEDFPU)
> >  #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
> >  #define _TIF_FREEZE		(1<<TIF_FREEZE)
> > +#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)
> 
> hm, all this duplication is unpleasing.  We could do something similar to
> include/linux/buffer_head.h:BH_PrivateStart here: get all architectures to
> define a TIF_COMMON_STUFF_STARTS_HERE then include asm-generic/whatever.h
> which defines all the flags which every architecture must define, as
> TIF_COMMON_STUFF_STARTS_HERE+0, TIF_COMMON_STUFF_STARTS_HERE+1, etc.
> 

Ok.

Thanks and Regards
gautham.

-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Gautham R Shenoy]

On Fri, Apr 20, 2007 at 10:54:36AM +0200, Rafael J. Wysocki wrote:
> 
> Hmm, can't we do something like this instead:
> 
> ---
>  kernel/kthread.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> Index: linux-2.6.21-rc7/kernel/kthread.c
> ===================================================================
> --- linux-2.6.21-rc7.orig/kernel/kthread.c
> +++ linux-2.6.21-rc7/kernel/kthread.c
> @@ -13,6 +13,7 @@
>  #include <linux/file.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/freezer.h>
>  #include <asm/semaphore.h>
> 
>  /*
> @@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
> 
>  	/* Now set kthread_should_stop() to true, and wake it up. */
>  	kthread_stop_info.k = k;
> +	if (!(current->flags & PF_NOFREEZE)) {
> +		/* If we are freezable, the freezer will wait for us */
> +		task_lock(k);
> +		k->flags |= PF_NOFREEZE;
> +		if (frozen(k))
> +			k->flags &= ~PF_FROZEN;
> +
> +		task_unlock(k);
> +	}

Yes, we can do this for now since the tasks have only two freeze states,
namely Freezeable and Non Freezeable. 
But with more events like cpu-hotplug and kprobes using the process
freezer, the simple check

	if(!(current->flags & PF_NOFREEZE))

may not suffice and something like
	if(!(current->flags & PFE_ALL & global_freeze_mask))
 	/* global_freeze_mask is the mask of the events for which the system
	  is freezing. */

appears to be racy. Not that we cannot work it out ;-)

However, I was attempting to solve the generic problem where
A waits on B and B is frozen. If A is freezeable (under one of the
events) then the freezer will fail. So a solution would be for A to
somehow inform B of the dependency and postpone it's freezing.

Since akpm mentioned that flush_workqueue() needs to go, I guess, I am
ok with fixing only kthread_stop/kthread_should_stop for the moment.
Unless I can spot any other valid case :)


>  	wake_up_process(k);
>  	put_task_struct(k);
> 

Thanks and Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Rafael J. Wysocki]

On Friday, 20 April 2007 13:05, Gautham R Shenoy wrote:
> On Fri, Apr 20, 2007 at 10:54:36AM +0200, Rafael J. Wysocki wrote:
> > 
> > Hmm, can't we do something like this instead:
> > 
> > ---
> >  kernel/kthread.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > Index: linux-2.6.21-rc7/kernel/kthread.c
> > ===================================================================
> > --- linux-2.6.21-rc7.orig/kernel/kthread.c
> > +++ linux-2.6.21-rc7/kernel/kthread.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/file.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > +#include <linux/freezer.h>
> >  #include <asm/semaphore.h>
> > 
> >  /*
> > @@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
> > 
> >  	/* Now set kthread_should_stop() to true, and wake it up. */
> >  	kthread_stop_info.k = k;
> > +	if (!(current->flags & PF_NOFREEZE)) {
> > +		/* If we are freezable, the freezer will wait for us */
> > +		task_lock(k);
> > +		k->flags |= PF_NOFREEZE;
> > +		if (frozen(k))
> > +			k->flags &= ~PF_FROZEN;
> > +
> > +		task_unlock(k);
> > +	}
> 
> Yes, we can do this for now since the tasks have only two freeze states,
> namely Freezeable and Non Freezeable. 
> But with more events like cpu-hotplug and kprobes using the process
> freezer, the simple check
> 
> 	if(!(current->flags & PF_NOFREEZE))
> 
> may not suffice and something like
> 	if(!(current->flags & PFE_ALL & global_freeze_mask))
>  	/* global_freeze_mask is the mask of the events for which the system
> 	  is freezing. */
> 
> appears to be racy. Not that we cannot work it out ;-)

Actually, I thought about it for a while.  The thread that is going to stop
another one may temporarily mark itself as freezable in all cases, which
will have no effect on it, since it's not going to cally try_to_freeze(), but
will make the freezer wait for it.  Next, after returning from
wait_for_completion(), it should restore its old freezability status and that
should make the freezer finish.

> However, I was attempting to solve the generic problem where
> A waits on B and B is frozen. If A is freezeable (under one of the
> events) then the freezer will fail. So a solution would be for A to
> somehow inform B of the dependency and postpone it's freezing.

Well, I think it might be simpler to consider each case separately.  This way
we may be able to avoid introducing the additional TIF_ flag.

> Since akpm mentioned that flush_workqueue() needs to go, I guess, I am
> ok with fixing only kthread_stop/kthread_should_stop for the moment.
> Unless I can spot any other valid case :)

Sure. :-)

BTW, if it turns out that we need to introduce yet another freezer-related
TIF_ flag, it may be acceptable (?) to move all of the freezer-related flags
into a separate member of task_struct (eg. freezer_flags) that can only be
manipulated under task_lock().

I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN,
PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for
the freezer-based CPU hotplug, so if yet another one is needed, that will make
up almost a separate u8 field ...

Greetings,
Rafael

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Gautham R Shenoy]

On Fri, Apr 20, 2007 at 01:59:29PM +0200, Rafael J. Wysocki wrote:
> 
> Actually, I thought about it for a while.  The thread that is going to stop
> another one may temporarily mark itself as freezable in all cases, which
> will have no effect on it, since it's not going to cally try_to_freeze(), but
> will make the freezer wait for it.  Next, after returning from
> wait_for_completion(), it should restore its old freezability status and that
> should make the freezer finish.

But that will have no affect if the thread to be stopped is already
frozen. The only affect might be that now, freezer will fail whether
the thread that is going to stop another one was freezeable or not.

Concern is whether we can somehow complete these wait_for_completion()'s in 
the freezing context and reduce the probabilty of freezer failing.

> 
> > However, I was attempting to solve the generic problem where
> > A waits on B and B is frozen. If A is freezeable (under one of the
> > events) then the freezer will fail. So a solution would be for A to
> > somehow inform B of the dependency and postpone it's freezing.
> 
> Well, I think it might be simpler to consider each case separately.  This way
> we may be able to avoid introducing the additional TIF_ flag.
> 

Makes sense.

> > Since akpm mentioned that flush_workqueue() needs to go, I guess, I am
> > ok with fixing only kthread_stop/kthread_should_stop for the moment.
> > Unless I can spot any other valid case :)
> 
> Sure. :-)
> 
> BTW, if it turns out that we need to introduce yet another freezer-related
> TIF_ flag, it may be acceptable (?) to move all of the freezer-related flags
> into a separate member of task_struct (eg. freezer_flags) that can only be
> manipulated under task_lock().
> 
> I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN,
> PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for
> the freezer-based CPU hotplug, so if yet another one is needed, that will make
> up almost a separate u8 field ...

I am perfectly ok with it. But I am not sure if everybody would agree to have
another field in the task struct, though in this case it does make sense :-)

> 
> Greetings,
> Rafael
> -

Thanks and Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Rafael J. Wysocki]

On Friday, 20 April 2007 14:26, Gautham R Shenoy wrote:
> On Fri, Apr 20, 2007 at 01:59:29PM +0200, Rafael J. Wysocki wrote:
> > 
> > Actually, I thought about it for a while.  The thread that is going to stop
> > another one may temporarily mark itself as freezable in all cases, which
> > will have no effect on it, since it's not going to cally try_to_freeze(), but
> > will make the freezer wait for it.  Next, after returning from
> > wait_for_completion(), it should restore its old freezability status and that
> > should make the freezer finish.
> 
> But that will have no affect if the thread to be stopped is already
> frozen. The only affect might be that now, freezer will fail whether
> the thread that is going to stop another one was freezeable or not.
> 
> Concern is whether we can somehow complete these wait_for_completion()'s in 
> the freezing context and reduce the probabilty of freezer failing.

To be precise, I was thinking about something like this (in pseudo-code):

	kthread_stop_info.k = k;
	save_freezable_status(current);
	set_always_freezable(current);
	/* Now, we know that the freezer will wait for us, although we're not
	 * really going to freeze
	 */
	task_lock(k);
	k->flags |= PF_NOFREEZE;
	if (frozen(k))
		k->flags &= ~PF_FROZEN;

	task_unlock(k);
	wake_up_process(k);
	put_task_struct(k);

	/* Once it dies, reset stop ptr, gather result and we're done. */
	wait_for_completion(&kthread_stop_info.done);
	restore_freezable_status(current);

> > > However, I was attempting to solve the generic problem where
> > > A waits on B and B is frozen. If A is freezeable (under one of the
> > > events) then the freezer will fail. So a solution would be for A to
> > > somehow inform B of the dependency and postpone it's freezing.
> > 
> > Well, I think it might be simpler to consider each case separately.  This way
> > we may be able to avoid introducing the additional TIF_ flag.
> > 
> 
> Makes sense.
> 
> > > Since akpm mentioned that flush_workqueue() needs to go, I guess, I am
> > > ok with fixing only kthread_stop/kthread_should_stop for the moment.
> > > Unless I can spot any other valid case :)
> > 
> > Sure. :-)
> > 
> > BTW, if it turns out that we need to introduce yet another freezer-related
> > TIF_ flag, it may be acceptable (?) to move all of the freezer-related flags
> > into a separate member of task_struct (eg. freezer_flags) that can only be
> > manipulated under task_lock().
> > 
> > I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN,
> > PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for
> > the freezer-based CPU hotplug, so if yet another one is needed, that will make
> > up almost a separate u8 field ...
> 
> I am perfectly ok with it. But I am not sure if everybody would agree to have
> another field in the task struct, though in this case it does make sense :-)

Well, I'm not sure either, but I guess the most practical way to learn it is to
send a patch. ;-)

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Andrew Morton]

On Fri, 20 Apr 2007 17:56:09 +0530
Gautham R Shenoy <ego@in.ibm.com> wrote:

> > I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN,
> > PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for
> > the freezer-based CPU hotplug, so if yet another one is needed, that will make
> > up almost a separate u8 field ...
> 
> I am perfectly ok with it. But I am not sure if everybody would agree to have
> another field in the task struct, though in this case it does make sense :-)

OK by me.  You might want to consider making that fields's locking protocol
be set_bit(), clear_bit(), etc rather than task_lock().

Re: [RFC PATCH 1/2] Fix PF_NOFREEZE and freezeable race

[Oleg Nesterov]

On 04/19, Rafael J. Wysocki wrote:
> 
> On Thursday, 19 April 2007 14:02, Gautham R Shenoy wrote:
> > This patch fixes the race pointed out by Oleg Nesterov.
> > 
> > * Freezer marks a thread as freezeable. 
> > * The thread now marks itself PF_NOFREEZE causing it to
> >   freeze on calling try_to_freeze(). Thus the task is frozen, even though
> >   it doesn't want to.
> > * Subsequent thaw_processes() will also fail to thaw the task since it is 
> >   marked PF_NOFREEZE.
> > 
> > Avoid this problem by checking the current task's PF_NOFREEZE status in the 
> > refrigerator before marking current as frozen.
> > 
> > Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
> 
> Looks good, although I'm not sure if we don't need to call recalc_sigpending()
> for tasks that turn out to be PF_NOFREEZE.

I agree, we should clear TIF_SIGPENDING. It is not so critical for user-space
tasks, but for the kernel thread it may remain pending forever, causing subtle
failures.

Gautham, isn't it possible to make a more simpler patch ? Just add PF_NOFREEZE
check to frozen_process,

	static inline void frozen_process(struct task_struct *p)
	{
		if (!unlikely(current->flags & PF_NOFREEZE)) {
			p->flags |= PF_FROZEN;
			wmb();
		}
		clear_tsk_thread_flag(p, TIF_FREEZE);
	}

No?

Oleg.

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > > I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN, 
> > > PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two 
> > > more for the freezer-based CPU hotplug, so if yet another one is 
> > > needed, that will make up almost a separate u8 field ...
> > 
> > I am perfectly ok with it. But I am not sure if everybody would 
> > agree to have another field in the task struct, though in this case 
> > it does make sense :-)
> 
> OK by me.  You might want to consider making that fields's locking 
> protocol be set_bit(), clear_bit(), etc rather than task_lock().

is OK to me too, the extra field isnt a problem.

	Ingo

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Oleg Nesterov]

On 04/19, Gautham R Shenoy wrote:
>
> @@ -63,12 +74,16 @@ void refrigerator(void)
>  	recalc_sigpending(); /* We sent fake signal, clean it up */
>  	spin_unlock_irq(&current->sighand->siglock);
>  
> +	task_lock(current);
>  	for (;;) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
>  		if (!frozen(current))
>  			break;
> +		task_unlock(current);
>  		schedule();
> +		task_lock(current);
>  	}
> +	task_unlock(current);
>  	pr_debug("%s left refrigerator\n", current->comm);
>  	current->state = save;

Just curious, why this change?

> +int hold_freezer_for_task(struct task_struct *p)
> +{
> +	int ret = 0;
> +	spin_lock(&freezer_status.lock);
> +	if (freezer_status.count >= 0)
> +	{
> +		set_tsk_thread_flag(p, TIF_FREEZER_HELD);
> +		thaw_process(p);
> +		freezer_status.count++;
> +		ret = 1;
> +	}
> +	spin_unlock(&freezer_status.lock);
> +
> +	return ret;
> +}

I think this can work if it is used only in kthread_stop(). But what if
another task wants to do hold_freezer_for_task(p) ? freezer_status.count
is recursive, but TIF_FREEZER_HELD is not. IOW, I believe this is not
generic enough.

Also, you are planning to add different freezing states (FE_HOTPLUG_CPU,
FE_SUSPEND, etc). In that case each of them needs a separate .count, because
it should be negative when try_to_freeze_tasks() returns. Now consider
the case when we are doing freeze_processes(FE_A | FE_B) ...

Oleg.

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Rafael J. Wysocki]

On Friday, 20 April 2007 20:31, Ingo Molnar wrote:
> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > > > I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN, 
> > > > PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two 
> > > > more for the freezer-based CPU hotplug, so if yet another one is 
> > > > needed, that will make up almost a separate u8 field ...
> > > 
> > > I am perfectly ok with it. But I am not sure if everybody would 
> > > agree to have another field in the task struct, though in this case 
> > > it does make sense :-)
> > 
> > OK by me.  You might want to consider making that fields's locking 
> > protocol be set_bit(), clear_bit(), etc rather than task_lock().
> 
> is OK to me too, the extra field isnt a problem.

OK, so I'll try to prepare a patch introducing it over the weekend. :-)

Greetings,
Rafael

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Oleg Nesterov]

On 04/20, Gautham R Shenoy wrote:
>
> On Fri, Apr 20, 2007 at 10:54:36AM +0200, Rafael J. Wysocki wrote:
> > 
> > Hmm, can't we do something like this instead:
> > 
> > ---
> >  kernel/kthread.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > Index: linux-2.6.21-rc7/kernel/kthread.c
> > ===================================================================
> > --- linux-2.6.21-rc7.orig/kernel/kthread.c
> > +++ linux-2.6.21-rc7/kernel/kthread.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/file.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > +#include <linux/freezer.h>
> >  #include <asm/semaphore.h>
> > 
> >  /*
> > @@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
> > 
> >  	/* Now set kthread_should_stop() to true, and wake it up. */
> >  	kthread_stop_info.k = k;
> > +	if (!(current->flags & PF_NOFREEZE)) {
> > +		/* If we are freezable, the freezer will wait for us */
> > +		task_lock(k);
> > +		k->flags |= PF_NOFREEZE;
> > +		if (frozen(k))
> > +			k->flags &= ~PF_FROZEN;
> > +
> > +		task_unlock(k);
> > +	}
> 
> Yes, we can do this for now since the tasks have only two freeze states,
> namely Freezeable and Non Freezeable. 

No, we can't change k->flags, k owns its ->flags, and it is not atomic.

Rafael, may I suggest you to document task_lock() in thaw_process() ? This
looks really confusing, as if task_lock() protects "p->flags &= ~PF_FROZEN".

Actually, task_lock() is needed to prevent the race with refrigerator()
when the freezing fails, but this is not obvious.

Oleg.

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Rafael J. Wysocki]

On Friday, 20 April 2007 23:20, Oleg Nesterov wrote:
> On 04/20, Gautham R Shenoy wrote:
> >
> > On Fri, Apr 20, 2007 at 10:54:36AM +0200, Rafael J. Wysocki wrote:
> > > 
> > > Hmm, can't we do something like this instead:
> > > 
> > > ---
> > >  kernel/kthread.c |   10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > Index: linux-2.6.21-rc7/kernel/kthread.c
> > > ===================================================================
> > > --- linux-2.6.21-rc7.orig/kernel/kthread.c
> > > +++ linux-2.6.21-rc7/kernel/kthread.c
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/file.h>
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>
> > > +#include <linux/freezer.h>
> > >  #include <asm/semaphore.h>
> > > 
> > >  /*
> > > @@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
> > > 
> > >  	/* Now set kthread_should_stop() to true, and wake it up. */
> > >  	kthread_stop_info.k = k;
> > > +	if (!(current->flags & PF_NOFREEZE)) {
> > > +		/* If we are freezable, the freezer will wait for us */
> > > +		task_lock(k);
> > > +		k->flags |= PF_NOFREEZE;
> > > +		if (frozen(k))
> > > +			k->flags &= ~PF_FROZEN;
> > > +
> > > +		task_unlock(k);
> > > +	}
> > 
> > Yes, we can do this for now since the tasks have only two freeze states,
> > namely Freezeable and Non Freezeable. 
> 
> No, we can't change k->flags, k owns its ->flags, and it is not atomic.

Yes, but if we move PF_FROZEN to a separate field in task_struct with
appropriate locking, then it won't be a problem any more IMO.
 
> Rafael, may I suggest you to document task_lock() in thaw_process() ? This
> looks really confusing, as if task_lock() protects "p->flags &= ~PF_FROZEN".
> 
> Actually, task_lock() is needed to prevent the race with refrigerator()
> when the freezing fails, but this is not obvious.

Sure, I will.

Greetings,
Rafael

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Pavel Machek]

Hi!

> > Of course, running try_to_freeze() in kthread_stop() would be very wrong,
> > so we'd actually need to do it in callers, preferably via a new
> > kthread_stop_freezeable() wrapper.
> > 
> Well, even then we'll need to ensure that a thread would not call
> kthread_stop_freezeable() with any locks held. That would mean more
> audits :)

I'll take audits over 200 new lines of code ;-). (And it should be
reasonably simple to audit; person doing kthread_stop() ->
ktrhread_stop_freezeable() should look after locks).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH -mm 0/3] Separate freezer flags

[Rafael J. Wysocki]

Hi,

The following three patches are related to the separation of the freezer flags
from process/threadinfo flags.

The first patch separates the freezer from the PM code, because it's no longer
a PM-specific piece of code.  This also makes the second patch look better.

The second patch introduces the freezer_flags field of task_struct, present
only if the freezer is compiled in.  All of the freezer-related flags per-task
flags are moved to this field and auxiliary functions for operating them are
defined in freezer.h .  This overlaps with the Gautham's work to some extent,
but I think it's better to introduce these changes independently of the CPU
hotplug code.

The third patch is a bonus. ;-)

Greetings,
Rafael

[RFC][PATCH -mm 1/3] Separate freezer from PM code

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Now that the freezer is used by kprobes, it is no longer a PM-specific piece of
code.  Move the freezer code out of kernel/power and introduce the
CONFIG_FREEZER option that will be chosen automatically if PM or KPROBES is set.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/arm/Kconfig         |    5 +
 arch/avr32/Kconfig.debug |    5 +
 arch/blackfin/Kconfig    |    5 +
 arch/frv/Kconfig         |    5 +
 arch/i386/Kconfig        |    5 +
 arch/ia64/Kconfig        |    5 +
 arch/mips/Kconfig        |    5 +
 arch/powerpc/Kconfig     |    5 +
 arch/ppc/Kconfig         |    5 +
 arch/s390/Kconfig        |    5 +
 arch/sh/Kconfig          |    5 +
 arch/sparc64/Kconfig     |    5 +
 arch/x86_64/Kconfig      |    8 +
 include/linux/freezer.h  |    2 
 kernel/Makefile          |    1 
 kernel/freezer.c         |  224 +++++++++++++++++++++++++++++++++++++++++++++++
 kernel/kprobes.c         |    2 
 kernel/power/Makefile    |    2 
 kernel/power/process.c   |  224 -----------------------------------------------
 19 files changed, 296 insertions(+), 227 deletions(-)

Index: linux-2.6.21-rc6-mm1/arch/x86_64/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/x86_64/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/x86_64/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -695,6 +695,14 @@ config GENERIC_PENDING_IRQ
 	depends on GENERIC_HARDIRQS && SMP
 	default y
 
+#
+# Use the tasks freezer
+#
+config FREEZER
+	bool
+	default y
+	depends on PM || KPROBES
+
 menu "Power management options"
 
 source kernel/power/Kconfig
Index: linux-2.6.21-rc6-mm1/arch/avr32/Kconfig.debug
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/avr32/Kconfig.debug	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/avr32/Kconfig.debug	2007-04-22 14:16:03.000000000 +0200
@@ -17,3 +17,8 @@ config KPROBES
           If in doubt, say "N".
 
 endmenu
+
+config FREEZER
+	bool
+	default y
+	depends on KPROBES
Index: linux-2.6.21-rc6-mm1/arch/frv/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/frv/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/frv/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -364,6 +364,11 @@ source "drivers/pcmcia/Kconfig"
 #	  sleep-deprived psychotic hacker types can say Y now, everyone else
 #	  should probably wait a while.
 
+config FREEZER
+	bool
+	default y
+	depends on PM
+
 menu "Power management options"
 source kernel/power/Kconfig
 endmenu
Index: linux-2.6.21-rc6-mm1/arch/i386/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/i386/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/i386/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -912,6 +912,11 @@ config ARCH_ENABLE_MEMORY_HOTPLUG
 	def_bool y
 	depends on HIGHMEM
 
+config FREEZER
+	bool
+	default y
+	depends on PM || KPROBES
+
 menu "Power management options (ACPI, APM)"
 	depends on !X86_VOYAGER
 
Index: linux-2.6.21-rc6-mm1/arch/ia64/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/ia64/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/ia64/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -490,6 +490,11 @@ source "fs/Kconfig.binfmt"
 
 endmenu
 
+config FREEZER
+	bool
+	default y
+	depends on PM || KPROBES
+
 menu "Power management and ACPI"
 
 source "kernel/power/Kconfig"
Index: linux-2.6.21-rc6-mm1/arch/powerpc/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/powerpc/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/powerpc/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -569,6 +569,11 @@ config CMDLINE
 	  some command-line options at build time by entering them here.  In
 	  most cases you will need to specify the root device here.
 
+config FREEZER
+	bool
+	default y
+	depends on PM || KPROBES
+
 if !44x || BROKEN
 source kernel/power/Kconfig
 endif
Index: linux-2.6.21-rc6-mm1/arch/ppc/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/ppc/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/ppc/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -1154,6 +1154,11 @@ config PROC_HARDWARE
 source "drivers/zorro/Kconfig"
 
 if !44x || BROKEN
+config FREEZER
+	bool
+	default y
+	depends on PM
+
 source kernel/power/Kconfig
 endif
 
Index: linux-2.6.21-rc6-mm1/arch/s390/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/s390/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/s390/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -564,6 +564,11 @@ config KPROBES
 	  for kernel debugging, non-intrusive instrumentation and testing.
 	  If in doubt, say "N".
 
+config FREEZER
+	bool
+	default y
+	depends on KPROBES
+
 source "kernel/Kconfig.marker"
 
 source "lib/Kconfig.statistic"
Index: linux-2.6.21-rc6-mm1/arch/sh/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/sh/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/sh/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -688,6 +688,11 @@ source "fs/Kconfig.binfmt"
 
 endmenu
 
+config FREEZER
+	bool
+	default y
+	depends on PM
+
 menu "Power management options (EXPERIMENTAL)"
 depends on EXPERIMENTAL
 
Index: linux-2.6.21-rc6-mm1/arch/sparc64/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/sparc64/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/sparc64/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -430,6 +430,11 @@ config KPROBES
 	  for kernel debugging, non-intrusive instrumentation and testing.
 	  If in doubt, say "N".
 
+config FREEZER
+	bool
+	default y
+	depends on KPROBES
+
 source "kernel/Kconfig.marker"
 
 endmenu
Index: linux-2.6.21-rc6-mm1/kernel/freezer.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.21-rc6-mm1/kernel/freezer.c	2007-04-22 18:03:27.000000000 +0200
@@ -0,0 +1,224 @@
+/*
+ * linux/kernel/freezer.c
+ *
+ * Generic mechanism for freezing and thawing tasks, originally from swsusp.
+ *
+ * Distributed under the GPLv2
+ */
+
+
+#undef DEBUG
+
+#include <linux/sched.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/syscalls.h>
+#include <linux/freezer.h>
+
+/*
+ * Timeout for stopping processes
+ */
+#define TIMEOUT	(20 * HZ)
+
+#define FREEZER_KERNEL_THREADS 0
+#define FREEZER_USER_SPACE 1
+
+static inline int freezeable(struct task_struct * p)
+{
+	if ((p == current) ||
+	    (p->flags & PF_NOFREEZE) ||
+	    (p->exit_state != 0))
+		return 0;
+	return 1;
+}
+
+/* Refrigerator is place where frozen processes are stored :-). */
+void refrigerator(void)
+{
+	/* Hmm, should we be allowed to suspend when there are realtime
+	   processes around? */
+	long save;
+
+	task_lock(current);
+	if (freezing(current)) {
+		frozen_process(current);
+		task_unlock(current);
+	} else {
+		task_unlock(current);
+		return;
+	}
+	save = current->state;
+	pr_debug("%s entered refrigerator\n", current->comm);
+
+	spin_lock_irq(&current->sighand->siglock);
+	recalc_sigpending(); /* We sent fake signal, clean it up */
+	spin_unlock_irq(&current->sighand->siglock);
+
+	for (;;) {
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		if (!frozen(current))
+			break;
+		schedule();
+	}
+	pr_debug("%s left refrigerator\n", current->comm);
+	current->state = save;
+}
+
+static inline void freeze_process(struct task_struct *p)
+{
+	unsigned long flags;
+
+	if (!freezing(p)) {
+		rmb();
+		if (!frozen(p)) {
+			if (p->state == TASK_STOPPED)
+				force_sig_specific(SIGSTOP, p);
+
+			freeze(p);
+			spin_lock_irqsave(&p->sighand->siglock, flags);
+			signal_wake_up(p, p->state == TASK_STOPPED);
+			spin_unlock_irqrestore(&p->sighand->siglock, flags);
+		}
+	}
+}
+
+static void cancel_freezing(struct task_struct *p)
+{
+	unsigned long flags;
+
+	if (freezing(p)) {
+		pr_debug("  clean up: %s\n", p->comm);
+		do_not_freeze(p);
+		spin_lock_irqsave(&p->sighand->siglock, flags);
+		recalc_sigpending_tsk(p);
+		spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	}
+}
+
+static inline int is_user_space(struct task_struct *p)
+{
+	int ret;
+
+	task_lock(p);
+	ret = p->mm && !(p->flags & PF_BORROWED_MM);
+	task_unlock(p);
+	return ret;
+}
+
+static unsigned int try_to_freeze_tasks(int freeze_user_space)
+{
+	struct task_struct *g, *p;
+	unsigned long end_time;
+	unsigned int todo;
+
+	end_time = jiffies + TIMEOUT;
+	do {
+		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)) {
+				cancel_freezing(p);
+				continue;
+			}
+			if (freeze_user_space && !is_user_space(p))
+				continue;
+
+			freeze_process(p);
+			if (!freezer_should_skip(p))
+				todo++;
+		} while_each_thread(g, p);
+		read_unlock(&tasklist_lock);
+		yield();			/* Yield is okay here */
+		if (todo && time_after(jiffies, end_time))
+			break;
+	} while (todo);
+
+	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 %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 (freeze_user_space && !is_user_space(p))
+				continue;
+
+			task_lock(p);
+			if (freezeable(p) && !frozen(p) &&
+			    !freezer_should_skip(p))
+				printk(KERN_ERR " %s\n", p->comm);
+
+			cancel_freezing(p);
+			task_unlock(p);
+		} while_each_thread(g, p);
+		read_unlock(&tasklist_lock);
+	}
+
+	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;
+}
+
+static void thaw_tasks(int thaw_user_space)
+{
+	struct task_struct *g, *p;
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+		if (!freezeable(p))
+			continue;
+
+		if (is_user_space(p) == !thaw_user_space)
+			continue;
+
+		thaw_process(p);
+	} 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");
+}
+
+EXPORT_SYMBOL(refrigerator);
Index: linux-2.6.21-rc6-mm1/kernel/power/process.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/power/process.c	2007-04-22 14:14:44.000000000 +0200
+++ /dev/null	1970-01-01 00:00:00.000000000 +0000
@@ -1,224 +0,0 @@
-/*
- * drivers/power/process.c - Functions for starting/stopping processes on 
- *                           suspend transitions.
- *
- * Originally from swsusp.
- */
-
-
-#undef DEBUG
-
-#include <linux/sched.h>
-#include <linux/interrupt.h>
-#include <linux/suspend.h>
-#include <linux/module.h>
-#include <linux/syscalls.h>
-#include <linux/freezer.h>
-
-/* 
- * Timeout for stopping processes
- */
-#define TIMEOUT	(20 * HZ)
-
-#define FREEZER_KERNEL_THREADS 0
-#define FREEZER_USER_SPACE 1
-
-static inline int freezeable(struct task_struct * p)
-{
-	if ((p == current) ||
-	    (p->flags & PF_NOFREEZE) ||
-	    (p->exit_state != 0))
-		return 0;
-	return 1;
-}
-
-/* Refrigerator is place where frozen processes are stored :-). */
-void refrigerator(void)
-{
-	/* Hmm, should we be allowed to suspend when there are realtime
-	   processes around? */
-	long save;
-
-	task_lock(current);
-	if (freezing(current)) {
-		frozen_process(current);
-		task_unlock(current);
-	} else {
-		task_unlock(current);
-		return;
-	}
-	save = current->state;
-	pr_debug("%s entered refrigerator\n", current->comm);
-
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
-	for (;;) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (!frozen(current))
-			break;
-		schedule();
-	}
-	pr_debug("%s left refrigerator\n", current->comm);
-	current->state = save;
-}
-
-static inline void freeze_process(struct task_struct *p)
-{
-	unsigned long flags;
-
-	if (!freezing(p)) {
-		rmb();
-		if (!frozen(p)) {
-			if (p->state == TASK_STOPPED)
-				force_sig_specific(SIGSTOP, p);
-
-			freeze(p);
-			spin_lock_irqsave(&p->sighand->siglock, flags);
-			signal_wake_up(p, p->state == TASK_STOPPED);
-			spin_unlock_irqrestore(&p->sighand->siglock, flags);
-		}
-	}
-}
-
-static void cancel_freezing(struct task_struct *p)
-{
-	unsigned long flags;
-
-	if (freezing(p)) {
-		pr_debug("  clean up: %s\n", p->comm);
-		do_not_freeze(p);
-		spin_lock_irqsave(&p->sighand->siglock, flags);
-		recalc_sigpending_tsk(p);
-		spin_unlock_irqrestore(&p->sighand->siglock, flags);
-	}
-}
-
-static inline int is_user_space(struct task_struct *p)
-{
-	int ret;
-
-	task_lock(p);
-	ret = p->mm && !(p->flags & PF_BORROWED_MM);
-	task_unlock(p);
-	return ret;
-}
-
-static unsigned int try_to_freeze_tasks(int freeze_user_space)
-{
-	struct task_struct *g, *p;
-	unsigned long end_time;
-	unsigned int todo;
-
-	end_time = jiffies + TIMEOUT;
-	do {
-		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)) {
-				cancel_freezing(p);
-				continue;
-			}
-			if (freeze_user_space && !is_user_space(p))
-				continue;
-
-			freeze_process(p);
-			if (!freezer_should_skip(p))
-				todo++;
-		} while_each_thread(g, p);
-		read_unlock(&tasklist_lock);
-		yield();			/* Yield is okay here */
-		if (todo && time_after(jiffies, end_time))
-			break;
-	} while (todo);
-
-	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 %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 (freeze_user_space && !is_user_space(p))
-				continue;
-
-			task_lock(p);
-			if (freezeable(p) && !frozen(p) &&
-			    !freezer_should_skip(p))
-				printk(KERN_ERR " %s\n", p->comm);
-
-			cancel_freezing(p);
-			task_unlock(p);
-		} while_each_thread(g, p);
-		read_unlock(&tasklist_lock);
-	}
-
-	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;
-}
-
-static void thaw_tasks(int thaw_user_space)
-{
-	struct task_struct *g, *p;
-
-	read_lock(&tasklist_lock);
-	do_each_thread(g, p) {
-		if (!freezeable(p))
-			continue;
-
-		if (is_user_space(p) == !thaw_user_space)
-			continue;
-
-		thaw_process(p);
-	} 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");
-}
-
-EXPORT_SYMBOL(refrigerator);
Index: linux-2.6.21-rc6-mm1/kernel/Makefile
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/Makefile	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/Makefile	2007-04-22 14:16:03.000000000 +0200
@@ -33,6 +33,7 @@ obj-$(CONFIG_MODULES) += module.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_STACK_UNWIND) += unwind.o
 obj-$(CONFIG_PM) += power/
+obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
 obj-$(CONFIG_KEXEC) += kexec.o
 obj-$(CONFIG_COMPAT) += compat.o
Index: linux-2.6.21-rc6-mm1/kernel/power/Makefile
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/power/Makefile	2007-04-22 14:14:47.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/power/Makefile	2007-04-22 14:16:03.000000000 +0200
@@ -3,7 +3,7 @@ ifeq ($(CONFIG_PM_DEBUG),y)
 EXTRA_CFLAGS	+=	-DDEBUG
 endif
 
-obj-y				:= main.o process.o console.o
+obj-y				:= main.o console.o
 obj-$(CONFIG_PM_LEGACY)		+= pm.o
 obj-$(CONFIG_SOFTWARE_SUSPEND)	+= swsusp.o disk.o snapshot.o swap.o user.o
 
Index: linux-2.6.21-rc6-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/kprobes.c	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/kprobes.c	2007-04-22 14:16:03.000000000 +0200
@@ -107,7 +107,7 @@ static int collect_garbage_slots(void);
 static int __kprobes check_safety(void)
 {
 	int ret = 0;
-#if defined(CONFIG_PREEMPT) && defined(CONFIG_PM)
+#ifdef CONFIG_PREEMPT
 	ret = freeze_processes();
 	if (ret == 0) {
 		struct task_struct *p, *q;
Index: linux-2.6.21-rc6-mm1/include/linux/freezer.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/linux/freezer.h	2007-04-22 14:15:20.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/linux/freezer.h	2007-04-22 18:03:02.000000000 +0200
@@ -2,7 +2,7 @@
 
 #include <linux/sched.h>
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_FREEZER
 /*
  * Check if a process has been frozen
  */
Index: linux-2.6.21-rc6-mm1/arch/arm/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/arm/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/arm/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -916,6 +916,11 @@ menu "Power management options"
 
 source "kernel/power/Kconfig"
 
+config FREEZER
+	bool
+	default y
+	depends on PM
+
 endmenu
 
 source "net/Kconfig"
Index: linux-2.6.21-rc6-mm1/arch/blackfin/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/blackfin/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/blackfin/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -818,6 +818,11 @@ endmenu
 menu "Power management options"
 source "kernel/power/Kconfig"
 
+config FREEZER
+	bool
+	default y
+	depends on PM
+
 choice
 	prompt "Select PM Wakeup Event Source"
 	default PM_WAKEUP_GPIO_BY_SIC_IWR
Index: linux-2.6.21-rc6-mm1/arch/mips/Kconfig
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/mips/Kconfig	2007-04-22 14:14:44.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/mips/Kconfig	2007-04-22 14:16:03.000000000 +0200
@@ -2138,6 +2138,11 @@ config BINFMT_ELF32
 
 source "kernel/power/Kconfig"
 
+config FREEZER
+	bool
+	default y
+	depends on PM
+
 endmenu
 
 source "net/Kconfig"

[RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Move all of the freezer-related flags to a separate field in task_struct and
introduce functions to operate them using set_bit() etc.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/power/kernel_threads.txt |    2 -
 Documentation/power/swsusp.txt         |    6 +--
 arch/i386/kernel/apm.c                 |    2 -
 drivers/block/loop.c                   |    2 -
 drivers/char/apm-emulation.c           |    6 +--
 drivers/ieee1394/ieee1394_core.c       |    2 -
 drivers/md/md.c                        |    2 -
 drivers/mmc/card/queue.c               |    3 +
 drivers/mtd/mtd_blkdevs.c              |    3 +
 drivers/scsi/libsas/sas_scsi_host.c    |    2 -
 drivers/scsi/scsi_error.c              |    2 -
 drivers/usb/storage/usb.c              |    2 -
 include/asm-arm/thread_info.h          |    2 -
 include/asm-blackfin/thread_info.h     |    2 -
 include/asm-frv/thread_info.h          |    2 -
 include/asm-i386/thread_info.h         |    2 -
 include/asm-ia64/thread_info.h         |    2 -
 include/asm-mips/thread_info.h         |    2 -
 include/asm-powerpc/thread_info.h      |    2 -
 include/asm-sh/thread_info.h           |    2 -
 include/asm-x86_64/thread_info.h       |    2 -
 include/linux/freezer.h                |   65 ++++++++++++++++++++++++++-------
 include/linux/sched.h                  |    8 ++--
 kernel/fork.c                          |    2 -
 kernel/freezer.c                       |    2 -
 kernel/rcutorture.c                    |    4 +-
 kernel/sched.c                         |    2 -
 kernel/softirq.c                       |    2 -
 kernel/softlockup.c                    |    2 -
 kernel/workqueue.c                     |    2 -
 30 files changed, 83 insertions(+), 58 deletions(-)

Index: linux-2.6.21-rc6-mm1/include/linux/sched.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/linux/sched.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/linux/sched.h	2007-04-22 20:55:01.000000000 +0200
@@ -1002,7 +1002,10 @@ struct task_struct {
 	/* Deadlock detection and priority inheritance handling */
 	struct rt_mutex_waiter *pi_blocked_on;
 #endif
-
+#ifdef CONFIG_FREEZER
+	/* Used by the process freezer, defined in freezer.h */
+	unsigned int freezer_flags;
+#endif
 #ifdef CONFIG_DEBUG_MUTEXES
 	/* mutex deadlock detection */
 	struct mutex_waiter *blocked_on;
@@ -1187,8 +1190,6 @@ static inline void put_task_struct(struc
 #define PF_MEMALLOC	0x00000800	/* Allocating memory */
 #define PF_FLUSHER	0x00001000	/* responsible for disk writeback */
 #define PF_USED_MATH	0x00002000	/* if unset the fpu must be initialized before use */
-#define PF_NOFREEZE	0x00008000	/* this thread should not be frozen */
-#define PF_FROZEN	0x00010000	/* frozen for system suspend */
 #define PF_FSTRANS	0x00020000	/* inside a filesystem transaction */
 #define PF_KSWAPD	0x00040000	/* I am kswapd */
 #define PF_SWAPOFF	0x00080000	/* I am in swapoff */
@@ -1200,7 +1201,6 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB	0x02000000	/* Spread some slab caches over cpuset */
 #define PF_MEMPOLICY	0x10000000	/* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER	0x20000000	/* Thread belongs to the rt mutex tester */
-#define PF_FREEZER_SKIP	0x40000000	/* Freezer should not count it as freezeable */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
Index: linux-2.6.21-rc6-mm1/include/asm-arm/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-arm/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-arm/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -147,7 +147,6 @@ extern void iwmmxt_task_switch(struct th
 #define TIF_POLLING_NRFLAG	16
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18
-#define TIF_FREEZE		19
 
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -155,7 +154,6 @@ extern void iwmmxt_task_switch(struct th
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
-#define _TIF_FREEZE		(1 << TIF_FREEZE)
 
 /*
  * Change these and you break ASM code in entry-common.S
Index: linux-2.6.21-rc6-mm1/include/asm-blackfin/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-blackfin/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-blackfin/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -125,7 +125,6 @@ static inline struct thread_info *curren
 					   TIF_NEED_RESCHED */
 #define TIF_MEMDIE              5
 #define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
-#define TIF_FREEZE              7       /* is freezing for suspend */
 #define TIF_SINGLESTEP		8       /* restore singlestep on return to user mode */
 
 /* as above, but as bit values */
@@ -135,7 +134,6 @@ static inline struct thread_info *curren
 #define _TIF_NEED_RESCHED	(1<<TIF_NEED_RESCHED)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
-#define _TIF_FREEZE             (1<<TIF_FREEZE)
 #define _TIF_SINGLESTEP		(1<<TIF_SINGLESTEP)
 
 #define _TIF_WORK_MASK		0x0000FFFE	/* work to do on interrupt/exception return */
Index: linux-2.6.21-rc6-mm1/include/asm-frv/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-frv/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-frv/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -116,7 +116,6 @@ register struct thread_info *__current_t
 #define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		17	/* OOM killer killed process */
-#define TIF_FREEZE		18	/* freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
@@ -126,7 +125,6 @@ register struct thread_info *__current_t
 #define _TIF_IRET		(1 << TIF_IRET)
 #define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
-#define _TIF_FREEZE		(1 << TIF_FREEZE)
 
 #define _TIF_WORK_MASK		0x0000FFFE	/* work to do on interrupt/exception return */
 #define _TIF_ALLWORK_MASK	0x0000FFFF	/* work to do on any return to u-space */
Index: linux-2.6.21-rc6-mm1/include/asm-i386/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-i386/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-i386/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -135,7 +135,6 @@ static inline struct thread_info *curren
 #define TIF_MEMDIE		16
 #define TIF_DEBUG		17	/* uses debug registers */
 #define TIF_IO_BITMAP		18	/* uses I/O bitmap */
-#define TIF_FREEZE		19	/* is freezing for suspend */
 #define TIF_FORCED_TF		20	/* true if TF in eflags artificially */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -149,7 +148,6 @@ static inline struct thread_info *curren
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
-#define _TIF_FREEZE		(1<<TIF_FREEZE)
 #define _TIF_FORCED_TF		(1<<TIF_FORCED_TF)
 
 /* work to do on interrupt/exception return */
Index: linux-2.6.21-rc6-mm1/include/asm-ia64/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-ia64/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-ia64/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -89,7 +89,6 @@ struct thread_info {
 #define TIF_MEMDIE		17
 #define TIF_MCA_INIT		18	/* this task is processing MCA or INIT */
 #define TIF_DB_DISABLED		19	/* debug trap disabled for fsyscall */
-#define TIF_FREEZE		20	/* is freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
@@ -101,7 +100,6 @@ struct thread_info {
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_MCA_INIT		(1 << TIF_MCA_INIT)
 #define _TIF_DB_DISABLED	(1 << TIF_DB_DISABLED)
-#define _TIF_FREEZE		(1 << TIF_FREEZE)
 
 /* "work to do on user-return" bits */
 #define TIF_ALLWORK_MASK	(_TIF_NOTIFY_RESUME|_TIF_SIGPENDING|_TIF_NEED_RESCHED|_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)
Index: linux-2.6.21-rc6-mm1/include/asm-mips/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-mips/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-mips/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -118,7 +118,6 @@ register struct thread_info *__current_t
 #define TIF_USEDFPU		16	/* FPU was used by this task this quantum (SMP) */
 #define TIF_POLLING_NRFLAG	17	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		18
-#define TIF_FREEZE		19
 #define TIF_ALLOW_FP_IN_KERNEL	20
 #define TIF_SYSCALL_TRACE	31	/* syscall trace active */
 
@@ -131,7 +130,6 @@ register struct thread_info *__current_t
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_USEDFPU		(1<<TIF_USEDFPU)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
-#define _TIF_FREEZE		(1<<TIF_FREEZE)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK		(0x0000ffef & ~_TIF_SECCOMP)
Index: linux-2.6.21-rc6-mm1/include/asm-powerpc/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-powerpc/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-powerpc/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -122,7 +122,6 @@ static inline struct thread_info *curren
 #define TIF_RESTOREALL		12	/* Restore all regs (implies NOERROR) */
 #define TIF_NOERROR		14	/* Force successful syscall return */
 #define TIF_RESTORE_SIGMASK	15	/* Restore signal mask in do_signal */
-#define TIF_FREEZE		16	/* Freezing for suspend */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -139,7 +138,6 @@ static inline struct thread_info *curren
 #define _TIF_RESTOREALL		(1<<TIF_RESTOREALL)
 #define _TIF_NOERROR		(1<<TIF_NOERROR)
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
-#define _TIF_FREEZE		(1<<TIF_FREEZE)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SECCOMP)
 
 #define _TIF_USER_WORK_MASK	(_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | \
Index: linux-2.6.21-rc6-mm1/include/asm-sh/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-sh/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-sh/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -115,7 +115,6 @@ static inline struct thread_info *curren
 #define TIF_USEDFPU		16	/* FPU was used by this task this quantum (SMP) */
 #define TIF_POLLING_NRFLAG	17	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		18
-#define TIF_FREEZE		19
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -125,7 +124,6 @@ static inline struct thread_info *curren
 #define _TIF_SINGLESTEP		(1<<TIF_SINGLESTEP)
 #define _TIF_USEDFPU		(1<<TIF_USEDFPU)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
-#define _TIF_FREEZE		(1<<TIF_FREEZE)
 
 #define _TIF_WORK_MASK		0x000000FE	/* work to do on interrupt/exception return */
 #define _TIF_ALLWORK_MASK	0x000000FF	/* work to do on any return to u-space */
Index: linux-2.6.21-rc6-mm1/include/asm-x86_64/thread_info.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/asm-x86_64/thread_info.h	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/asm-x86_64/thread_info.h	2007-04-22 20:55:01.000000000 +0200
@@ -122,7 +122,6 @@ static inline struct thread_info *stack_
 #define TIF_MEMDIE		20
 #define TIF_DEBUG		21	/* uses debug registers */
 #define TIF_IO_BITMAP		22	/* uses I/O bitmap */
-#define TIF_FREEZE		23	/* is freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -139,7 +138,6 @@ static inline struct thread_info *stack_
 #define _TIF_ABI_PENDING	(1<<TIF_ABI_PENDING)
 #define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
-#define _TIF_FREEZE		(1<<TIF_FREEZE)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
Index: linux-2.6.21-rc6-mm1/include/linux/freezer.h
===================================================================
--- linux-2.6.21-rc6-mm1.orig/include/linux/freezer.h	2007-04-22 20:54:59.000000000 +0200
+++ linux-2.6.21-rc6-mm1/include/linux/freezer.h	2007-04-22 20:55:01.000000000 +0200
@@ -3,12 +3,33 @@
 #include <linux/sched.h>
 
 #ifdef CONFIG_FREEZER
+
+/*
+ *	Per task flags used by the freezer
+ *
+ *	They should not be referred to directly outside of this file.
+ */
+#define TFF_NOFREEZE	0	/* task should not be frozen */
+#define TFF_FREEZE	8	/* task should go to the refrigerator ASAP */
+#define TFF_SKIP	9	/* do not count this task as freezable */
+#define TFF_FROZEN	10	/* task is frozen */
+
 /*
  * Check if a process has been frozen
  */
 static inline int frozen(struct task_struct *p)
 {
-	return p->flags & PF_FROZEN;
+	return test_bit(TFF_FROZEN, &p->freezer_flags);
+}
+
+static inline void set_frozen_flag(struct task_struct *p)
+{
+	set_bit(TFF_FROZEN, &p->freezer_flags);
+}
+
+static inline void clear_frozen_flag(struct task_struct *p)
+{
+	clear_bit(TFF_FROZEN, &p->freezer_flags);
 }
 
 /*
@@ -16,7 +37,7 @@ static inline int frozen(struct task_str
  */
 static inline int freezing(struct task_struct *p)
 {
-	return test_tsk_thread_flag(p, TIF_FREEZE);
+	return test_bit(TFF_FREEZE, &p->freezer_flags);
 }
 
 /*
@@ -24,15 +45,31 @@ static inline int freezing(struct task_s
  */
 static inline void freeze(struct task_struct *p)
 {
-	set_tsk_thread_flag(p, TIF_FREEZE);
+	set_bit(TFF_FREEZE, &p->freezer_flags);
 }
 
 /*
- * Sometimes we may need to cancel the previous 'freeze' request
+ * Cancel the previous 'freeze' request
  */
 static inline void do_not_freeze(struct task_struct *p)
 {
-	clear_tsk_thread_flag(p, TIF_FREEZE);
+	clear_bit(TFF_FREEZE, &p->freezer_flags);
+}
+
+/*
+ * Check if the task wants to be exempted from freezing
+ */
+static inline int freezer_should_exempt(struct task_struct *p)
+{
+	return test_bit(TFF_NOFREEZE, &p->freezer_flags);
+}
+
+/*
+ * Tell the freezer to exempt this task from freezing
+ */
+static inline void freezer_exempt(struct task_struct *p)
+{
+	set_bit(TFF_NOFREEZE, &p->freezer_flags);
 }
 
 /*
@@ -48,12 +85,12 @@ static inline int thaw_process(struct ta
 {
 	task_lock(p);
 	if (frozen(p)) {
-		p->flags &= ~PF_FROZEN;
+		clear_frozen_flag(p);
 		task_unlock(p);
 		wake_up_process(p);
 		return 1;
 	}
-	clear_tsk_thread_flag(p, TIF_FREEZE);
+	do_not_freeze(p);
 	task_unlock(p);
 	return 0;
 }
@@ -63,9 +100,9 @@ static inline int thaw_process(struct ta
  */
 static inline void frozen_process(struct task_struct *p)
 {
-	p->flags |= PF_FROZEN;
+	set_frozen_flag(p);
 	wmb();
-	clear_tsk_thread_flag(p, TIF_FREEZE);
+	do_not_freeze(p);
 }
 
 extern void refrigerator(void);
@@ -102,7 +139,7 @@ static inline int try_to_freeze(void)
 static inline void freezer_do_not_count(void)
 {
 	if (current->mm)
-		current->flags |= PF_FREEZER_SKIP;
+		set_bit(TFF_SKIP, &current->freezer_flags);
 }
 
 /*
@@ -112,7 +149,7 @@ static inline void freezer_do_not_count(
 static inline void freezer_count(void)
 {
 	if (current->mm) {
-		current->flags &= ~PF_FREEZER_SKIP;
+		clear_bit(TFF_SKIP, &current->freezer_flags);
 		try_to_freeze();
 	}
 }
@@ -122,13 +159,17 @@ static inline void freezer_count(void)
  */
 static inline int freezer_should_skip(struct task_struct *p)
 {
-	return !!(p->flags & PF_FREEZER_SKIP);
+	return test_bit(TFF_SKIP, &current->freezer_flags);
 }
 
 #else
 static inline int frozen(struct task_struct *p) { return 0; }
+static inline void set_frozen_flag(struct task_struct *p) {}
+static inline void clear_frozen_flag(struct task_struct *p) {}
 static inline int freezing(struct task_struct *p) { return 0; }
 static inline void freeze(struct task_struct *p) { BUG(); }
+static inline int freezer_should_exempt(struct task_struct *p) { return 0; }
+static inline void freezer_exempt(struct task_struct *p) {}
 static inline int thaw_process(struct task_struct *p) { return 1; }
 static inline void frozen_process(struct task_struct *p) { BUG(); }
 
Index: linux-2.6.21-rc6-mm1/kernel/sched.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/sched.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/sched.c	2007-04-22 20:55:01.000000000 +0200
@@ -5431,7 +5431,7 @@ migration_call(struct notifier_block *nf
 		p = kthread_create(migration_thread, hcpu, "migration/%d",cpu);
 		if (IS_ERR(p))
 			return NOTIFY_BAD;
-		p->flags |= PF_NOFREEZE;
+		freezer_exempt(p);
 		kthread_bind(p, cpu);
 		/* Must be high prio: stop_machine expects to yield to it. */
 		rq = task_rq_lock(p, &flags);
Index: linux-2.6.21-rc6-mm1/arch/i386/kernel/apm.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/arch/i386/kernel/apm.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/arch/i386/kernel/apm.c	2007-04-22 20:55:01.000000000 +0200
@@ -2313,7 +2313,7 @@ static int __init apm_init(void)
 		remove_proc_entry("apm", NULL);
 		return err;
 	}
-	kapmd_task->flags |= PF_NOFREEZE;
+	freezer_exempt(kapmd_task);
 	wake_up_process(kapmd_task);
 
 	if (num_online_cpus() > 1 && !smp ) {
Index: linux-2.6.21-rc6-mm1/drivers/block/loop.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/block/loop.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/block/loop.c	2007-04-22 20:55:01.000000000 +0200
@@ -586,7 +586,7 @@ static int loop_thread(void *data)
 	 * hence, it mustn't be stopped at all
 	 * because it could be indirectly used during suspension
 	 */
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	set_user_nice(current, -20);
 
Index: linux-2.6.21-rc6-mm1/drivers/char/apm-emulation.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/char/apm-emulation.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/char/apm-emulation.c	2007-04-22 20:55:01.000000000 +0200
@@ -336,7 +336,7 @@ apm_ioctl(struct inode * inode, struct f
 			 * threads.
 			 */
 			flags = current->flags;
-			current->flags |= PF_NOFREEZE;
+			freezer_exempt(current);
 
 			wait_event(apm_suspend_waitqueue,
 				   as->suspend_state == SUSPEND_DONE);
@@ -372,7 +372,7 @@ apm_ioctl(struct inode * inode, struct f
 			 * threads.
 			 */
 			flags = current->flags;
-			current->flags |= PF_NOFREEZE;
+			freezer_exempt(current);
 
 			wait_event_interruptible(apm_suspend_waitqueue,
 					 as->suspend_state == SUSPEND_DONE);
@@ -601,7 +601,7 @@ static int __init apm_init(void)
 		kapmd_tsk = NULL;
 		return ret;
 	}
-	kapmd_tsk->flags |= PF_NOFREEZE;
+	freezer_exempt(kapmd_tsk);
 	wake_up_process(kapmd_tsk);
 
 #ifdef CONFIG_PROC_FS
Index: linux-2.6.21-rc6-mm1/drivers/ieee1394/ieee1394_core.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/ieee1394/ieee1394_core.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/ieee1394/ieee1394_core.c	2007-04-22 20:55:01.000000000 +0200
@@ -1134,7 +1134,7 @@ static int hpsbpkt_thread(void *__hi)
 	struct list_head tmp;
 	int may_schedule;
 
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	while (!kthread_should_stop()) {
 
Index: linux-2.6.21-rc6-mm1/drivers/md/md.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/md/md.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/md/md.c	2007-04-22 20:55:01.000000000 +0200
@@ -4541,7 +4541,7 @@ static int md_thread(void * arg)
 	 * many dirty RAID5 blocks.
 	 */
 
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 	allow_signal(SIGKILL);
 	while (!kthread_should_stop()) {
 
Index: linux-2.6.21-rc6-mm1/drivers/mmc/card/queue.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/mmc/card/queue.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/mmc/card/queue.c	2007-04-22 20:55:01.000000000 +0200
@@ -66,7 +66,8 @@ static int mmc_queue_thread(void *d)
 	 * Set iothread to ensure that we aren't put to sleep by
 	 * the process freezing.  We handle suspension ourselves.
 	 */
-	current->flags |= PF_MEMALLOC|PF_NOFREEZE;
+	current->flags |= PF_MEMALLOC;
+	freezer_exempt(current);
 
 	down(&mq->thread_sem);
 	do {
Index: linux-2.6.21-rc6-mm1/drivers/mtd/mtd_blkdevs.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/mtd/mtd_blkdevs.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/mtd/mtd_blkdevs.c	2007-04-22 20:55:01.000000000 +0200
@@ -82,7 +82,8 @@ static int mtd_blktrans_thread(void *arg
 	struct request_queue *rq = tr->blkcore_priv->rq;
 
 	/* we might get involved when memory gets low, so use PF_MEMALLOC */
-	current->flags |= PF_MEMALLOC | PF_NOFREEZE;
+	current->flags |= PF_MEMALLOC;
+	freezer_exempt(current);
 
 	daemonize("%sd", tr->name);
 
Index: linux-2.6.21-rc6-mm1/drivers/scsi/libsas/sas_scsi_host.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/scsi/libsas/sas_scsi_host.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/scsi/libsas/sas_scsi_host.c	2007-04-22 20:55:01.000000000 +0200
@@ -871,7 +871,7 @@ static int sas_queue_thread(void *_sas_h
 	struct scsi_core *core = &sas_ha->core;
 
 	daemonize("sas_queue_%d", core->shost->host_no);
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	complete(&queue_th_comp);
 
Index: linux-2.6.21-rc6-mm1/drivers/scsi/scsi_error.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/scsi/scsi_error.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/scsi/scsi_error.c	2007-04-22 20:55:01.000000000 +0200
@@ -1536,7 +1536,7 @@ int scsi_error_handler(void *data)
 {
 	struct Scsi_Host *shost = data;
 
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	/*
 	 * We use TASK_INTERRUPTIBLE so that the thread is not
Index: linux-2.6.21-rc6-mm1/drivers/usb/storage/usb.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/drivers/usb/storage/usb.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/drivers/usb/storage/usb.c	2007-04-22 20:55:01.000000000 +0200
@@ -301,7 +301,7 @@ static int usb_stor_control_thread(void 
 	struct us_data *us = (struct us_data *)__us;
 	struct Scsi_Host *host = us_to_host(us);
 
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	for(;;) {
 		try_to_freeze();
Index: linux-2.6.21-rc6-mm1/kernel/fork.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/fork.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/fork.c	2007-04-22 20:55:01.000000000 +0200
@@ -922,7 +922,7 @@ static inline void copy_flags(unsigned l
 {
 	unsigned long new_flags = p->flags;
 
-	new_flags &= ~(PF_SUPERPRIV | PF_NOFREEZE);
+	new_flags &= ~PF_SUPERPRIV;
 	new_flags |= PF_FORKNOEXEC;
 	new_flags |= PF_STARTING;
 	p->flags = new_flags;
Index: linux-2.6.21-rc6-mm1/kernel/rcutorture.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/rcutorture.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/rcutorture.c	2007-04-22 20:55:01.000000000 +0200
@@ -559,7 +559,7 @@ rcu_torture_fakewriter(void *arg)
 
 	VERBOSE_PRINTK_STRING("rcu_torture_fakewriter task started");
 	set_user_nice(current, 19);
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	do {
 		schedule_timeout_uninterruptible(1 + rcu_random(&rand)%10);
@@ -590,7 +590,7 @@ rcu_torture_reader(void *arg)
 
 	VERBOSE_PRINTK_STRING("rcu_torture_reader task started");
 	set_user_nice(current, 19);
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	do {
 		idx = cur_ops->readlock();
Index: linux-2.6.21-rc6-mm1/kernel/softirq.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/softirq.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/softirq.c	2007-04-22 20:55:01.000000000 +0200
@@ -490,7 +490,7 @@ void __init softirq_init(void)
 static int ksoftirqd(void * __bind_cpu)
 {
 	set_user_nice(current, 19);
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	set_current_state(TASK_INTERRUPTIBLE);
 
Index: linux-2.6.21-rc6-mm1/kernel/softlockup.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/softlockup.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/softlockup.c	2007-04-22 20:55:01.000000000 +0200
@@ -117,7 +117,7 @@ static int watchdog(void * __bind_cpu)
 	struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
 
 	sched_setscheduler(current, SCHED_FIFO, &param);
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 	/* initialize timestamp */
 	touch_softlockup_watchdog();
Index: linux-2.6.21-rc6-mm1/kernel/workqueue.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/workqueue.c	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/workqueue.c	2007-04-22 20:55:01.000000000 +0200
@@ -292,7 +292,7 @@ static int worker_thread(void *__cwq)
 	struct k_sigaction sa;
 
 	if (!cwq->wq->freezeable)
-		current->flags |= PF_NOFREEZE;
+		freezer_exempt(current);
 
 	/*
 	 * We inherited MPOL_INTERLEAVE from the booting kernel.
Index: linux-2.6.21-rc6-mm1/kernel/freezer.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/freezer.c	2007-04-22 20:54:59.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/freezer.c	2007-04-22 20:55:01.000000000 +0200
@@ -26,7 +26,7 @@
 static inline int freezeable(struct task_struct * p)
 {
 	if ((p == current) ||
-	    (p->flags & PF_NOFREEZE) ||
+	    freezer_should_exempt(p) ||
 	    (p->exit_state != 0))
 		return 0;
 	return 1;
Index: linux-2.6.21-rc6-mm1/Documentation/power/kernel_threads.txt
===================================================================
--- linux-2.6.21-rc6-mm1.orig/Documentation/power/kernel_threads.txt	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/Documentation/power/kernel_threads.txt	2007-04-22 20:55:01.000000000 +0200
@@ -32,7 +32,7 @@ like this:
 	 */
 	daemonize("usb-storage");
 
-	current->flags |= PF_NOFREEZE;
+	freezer_exempt(current);
 
 from drivers/usb/storage/usb.c::usb_stor_control_thread()
 
Index: linux-2.6.21-rc6-mm1/Documentation/power/swsusp.txt
===================================================================
--- linux-2.6.21-rc6-mm1.orig/Documentation/power/swsusp.txt	2007-04-22 19:37:42.000000000 +0200
+++ linux-2.6.21-rc6-mm1/Documentation/power/swsusp.txt	2007-04-22 20:55:01.000000000 +0200
@@ -152,8 +152,8 @@ add:
        try_to_freeze();
 
 If the thread is needed for writing the image to storage, you should
-instead set the PF_NOFREEZE process flag when creating the thread (and
-be very careful).
+instead use freezer_exempt() to mark the thread as nonfreezable when creating
+it (and be very careful).
 
 
 Q: What is the difference between "platform" and "shutdown"?

[RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Fix the problem with kthread_stop() that causes the freezer to fail if a
freezable thread is attempting to stop a frozen one and that may cause the
freezer to fail if the thread being stopped is freezable and
try_to_freeze_tasks() is running concurrently with kthread_stop().

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/kthread.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/kthread.c	2007-04-09 15:23:48.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/kthread.c	2007-04-22 19:05:29.000000000 +0200
@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 #include <asm/semaphore.h>
 
 /*
@@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!freezer_should_exempt(current)) {
+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);
+	}
 	wake_up_process(k);
 	put_task_struct(k);
 

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Paul Jackson]

Rafael wrote:
> Move all of the freezer-related flags to a separate field in task_struct and
> introduce functions to operate them using set_bit() etc.

It's getting time I learned what this freezer thing is.

What would you suggest I read?

I looked in include/linux/freezer.h and didn't see any explanations.
I found one Documenation file, power/kernel_threads.txt, that explained
the interaction of freezing and kernel threads.  I looked in the
comments for various 2.6.21-rc6-mm1 freezer* patches, and saw various
interesting details.

But I couldn't find any documentation telling me what a freezer was,
or what a refrigerator is.

Did I miss something?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

On Sunday, 22 April 2007 23:14, Paul Jackson wrote:
> Rafael wrote:
> > Move all of the freezer-related flags to a separate field in task_struct and
> > introduce functions to operate them using set_bit() etc.
> 
> It's getting time I learned what this freezer thing is.
> 
> What would you suggest I read?
> 
> I looked in include/linux/freezer.h and didn't see any explanations.
> I found one Documenation file, power/kernel_threads.txt, that explained
> the interaction of freezing and kernel threads.  I looked in the
> comments for various 2.6.21-rc6-mm1 freezer* patches, and saw various
> interesting details.
> 
> But I couldn't find any documentation telling me what a freezer was,
> or what a refrigerator is.
> 
> Did I miss something?

Well, unfortunately not.  The freezer is not separately documented, mainly
because (1) for a long time it's been a part of the suspend code and no one
else used it, and (2) because it's been changing a lot recently and it's been
a 'moving target' from the documentation point of view.

I'll try to explain how it works.

In short, we use the freezer to make tasks enter a specific function, called
the refrigerator, and stay there until they are let out.  This is done by
calling freeze_processes() (in 2.6.21-rc7 it is located in
kernel/power/process.c) that executes try_to_freeze_tasks() separately for
userland processes and kernel threads (the sync() in between is needed by
the suspend code).  try_to_freeze_tasks() sets the FREEZE flag (TIF_FREEZE in
2.6.21-rc7) for each freezable (I'll explain what that means in a while) task
and sends a fake signal to it.

Userland processes then go to get_signal_to_deliver(), where they execute
try_to_freeze() (defined in include/linux/freezer.h) and call refrigerator()
(in kernel/power/process.c), since the FREEZE flag is set.  In refrigerator()
they reset the FREEZE flag and set the FROZEN flag (in 2.6.21-rc7 it is a
process flag defined in sched.h).  Next, they loop in refrigerator() until
someone resets the FROZEN flag for them.  Then we say that they are 'frozen'.

Kernel threads don't call get_signal_to_deliver(), so they have to execute
try_to_freeze() directly to go to the refrigerator.  Moreover, kernel threads
may not want to go there at all, in which case they should set the NOFREEZE
flag (in 2.6.21-rc7 it is a process flag defined in sched.h), that makes
try_to_freeze_tasks() ignore them.

Apart from the kernel threads that have the NOFREEZE flag set,
try_to_freeze_tasks() ignores the task that's running it (its current task)
and the tasks that have exit_state different from 0.  They all are regarded
as 'not freezable'.

Of course, kernel threads that declare themselves as not freezable, by setting
the NOFREEZE flag, should be careful enough not to interfere with the subsystem
that is using the freezer (ie. has called freeze_processes()), which may be
quite difficult in practice, so only a few kernel threads do it (and even some
of them really shouldn't).

The subsystem that has called freeze_processes() is responsible for the
'thawing' of tasks, which is done by calling thaw_processes() (defined in
kernel/power/process.c).  It runs thaw_tasks() (in the same file) for kernel
threads and userland processes.  This function loops over all tasks and
resets the FROZEN flag for them, so that they can leave the refrigerator.

The suspend code uses the freezer to make sure that processes won't get
in the way when some suspend-related low-level operations are carried out
(like suspending devices, the creation of a suspend image by swsusp etc.).
Other subsystems may use it for similar purposes.

Greetings,
Rafael

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Paul Jackson]

Rafael wrote:
> I'll try to explain how it works.

Ok - thanks.  Good explanation of how it works.

One more question - why would I want to do this?

Is this like something that would be useful on a laptop, to suspend
activity and reduce battery drain, while preserving the current state
of ones sessions and avoiding having to logout or shutdown?

Or are there other good uses for this?

Is it useful for quietting a system down before doing hot plug or
unplug of key components, such as processors and memory?

Thanks.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Satyam Sharma]

On 4/23/07, Paul Jackson <pj@sgi.com> wrote:
> One more question - why would I want to do this?

Check out the FAQ in Documentation/power/swsusp.txt.

> Is this like something that would be useful on a laptop, to suspend
> activity and reduce battery drain, while preserving the current state
> of ones sessions and avoiding having to logout or shutdown?

Yes, the original purpose for the inclusion of the freezer code was to
support suspend-resume (mainly for laptops, but suspend-resume could
be useful in other circumstances too, see the FAQ).

> Is it useful for quietting a system down before doing hot plug or
> unplug of key components, such as processors and memory?

Yes, the freezer is (proposed to be, at least) moving on from being
merely a suspend-resume-only thing to other usage scenarios, such as
kprobes and hotlpug.

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Satyam Sharma]

Hi Rafael,

> +/*
> + *     Per task flags used by the freezer
> + *
> + *     They should not be referred to directly outside of this file.
> + */
> +#define TFF_NOFREEZE   0       /* task should not be frozen */
> +#define TFF_FREEZE     8       /* task should go to the refrigerator ASAP */
> +#define TFF_SKIP       9       /* do not count this task as freezable */
> +#define TFF_FROZEN     10      /* task is frozen */

Aren't NOFREEZE and SKIP doing the same thing? One of them appears
superfluous. I'm looking at 21-rc6-mm1 and vfork(2) seems to be its
only user. Seeing how vfork(2) used it, can't the call to
freezer_do_not_count() be replaced with a call to freezer_exempt()?
Similarly, the freezer_count() after the wait_for_completion might
just as well be a clear of the NOFREEZE bit followed by a
try_to_freeze(). Could you please explain the rationale behind the
SKIP flag?

I do see that SKIP seems to be relevant for only userspace threads and
presumably only kernel threads are allowed to set NOFREEZE, but why
this distinction between the two?

Also, I do have several gripes against the naming of some of these functions:

>  static inline int freezing(struct task_struct *p)

This could be called task_should_freeze().

>  /*
> - * Sometimes we may need to cancel the previous 'freeze' request
> + * Cancel the previous 'freeze' request
>   */
>  static inline void do_not_freeze(struct task_struct *p)

This definitely needs to be undo_freeze() or unfreeze().
do_not_freeze() sounds like what freeze_exempt() does.

>  static inline void frozen_process(struct task_struct *p)

frozen_process() sounds like what frozen() is supposed to do. This
could instead be mark_task_frozen(), or even mark_frozen(), because
only the current task can ever mark *itself* frozen before freezing
itself.

>  static inline void freezer_do_not_count(void)
>  static inline void freezer_count(void)

These could be called freezer_skip() and freezer_do_not_skip(). Better
to stick to consistent naming / terminology.

Cheers,
Satyam

Re: [RFC PATCH 1/2] Fix PF_NOFREEZE and freezeable race

[Gautham R Shenoy]

On Fri, Apr 20, 2007 at 10:02:08PM +0400, Oleg Nesterov wrote:
> On 04/19, Rafael J. Wysocki wrote:
> > 
> > On Thursday, 19 April 2007 14:02, Gautham R Shenoy wrote:
> > > This patch fixes the race pointed out by Oleg Nesterov.
> > > 
> > > * Freezer marks a thread as freezeable. 
> > > * The thread now marks itself PF_NOFREEZE causing it to
> > >   freeze on calling try_to_freeze(). Thus the task is frozen, even though
> > >   it doesn't want to.
> > > * Subsequent thaw_processes() will also fail to thaw the task since it is 
> > >   marked PF_NOFREEZE.
> > > 
> > > Avoid this problem by checking the current task's PF_NOFREEZE status in the 
> > > refrigerator before marking current as frozen.
> > > 
> > > Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
> > 
> > Looks good, although I'm not sure if we don't need to call recalc_sigpending()
> > for tasks that turn out to be PF_NOFREEZE.
> 
> I agree, we should clear TIF_SIGPENDING. It is not so critical for user-space
> tasks, but for the kernel thread it may remain pending forever, causing subtle
> failures.
> 
> Gautham, isn't it possible to make a more simpler patch ? Just add PF_NOFREEZE
> check to frozen_process,
> 
> 	static inline void frozen_process(struct task_struct *p)
> 	{
> 		if (!unlikely(current->flags & PF_NOFREEZE)) {
> 			p->flags |= PF_FROZEN;
> 			wmb();
> 		}
> 		clear_tsk_thread_flag(p, TIF_FREEZE);
> 	}
> 
> No?

Actually yes. The idea anyway was to check one last time before declaring
ourselves as frozen. So I thought the best place was inside refrigerator since
we are already holding the task_lock there.
I wasn't too sure about calling recalc_sigpending(), but now that you
mention it, I agree, this would be a nicer way to do it.

Btw, since frozen_process is currently being called only from
refrigerator, I am wondering if we still need the struct task_struct *p
parameter there. It's very unlikely that some other task would mark a
particular task as frozen. No?

Anyways, Andrew, Could you please replace the earlier sent patch titled
"fix_pf_nofreeze_and_freezeable_race.patch" with the following one?

Thanks and Regards
gautham.

-->
This patch fixes the race pointed out by Oleg Nesterov.

* Freezer marks a thread as freezeable.
* The thread now marks itself PF_NOFREEZE causing it to
  freeze on calling try_to_freeze(). Thus the task is frozen, even though
  it doesn't want to.
* Subsequent thaw_processes() will also fail to thaw the task since it is
  marked PF_NOFREEZE.

Avoid this problem by checking the task's PF_NOFREEZE status in 
frozen_processes() before marking the task as frozen.

Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
 include/linux/freezer.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6.21-rc6/include/linux/freezer.h
===================================================================
--- linux-2.6.21-rc6.orig/include/linux/freezer.h
+++ linux-2.6.21-rc6/include/linux/freezer.h
@@ -57,8 +57,10 @@ static inline int thaw_process(struct ta
  */
 static inline void frozen_process(struct task_struct *p)
 {
-	p->flags |= PF_FROZEN;
-	wmb();
+	if (!unlikely(p->flags & PF_NOFREEZE)) {
+		p->flags |= PF_FROZEN;
+		wmb();
+	}
 	clear_tsk_thread_flag(p, TIF_FREEZE);
 }
 
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Gautham R Shenoy]

On Sat, Apr 21, 2007 at 01:12:09AM +0400, Oleg Nesterov wrote:
> On 04/19, Gautham R Shenoy wrote:
> >
> > @@ -63,12 +74,16 @@ void refrigerator(void)
> >  	recalc_sigpending(); /* We sent fake signal, clean it up */
> >  	spin_unlock_irq(&current->sighand->siglock);
> >  
> > +	task_lock(current);
> >  	for (;;) {
> >  		set_current_state(TASK_UNINTERRUPTIBLE);
> >  		if (!frozen(current))
> >  			break;
> > +		task_unlock(current);
> >  		schedule();
> > +		task_lock(current);
> >  	}
> > +	task_unlock(current);
> >  	pr_debug("%s left refrigerator\n", current->comm);
> >  	current->state = save;
> 
> Just curious, why this change?
This can race with hold_freezer_for_task() calling thaw_process. Earlier
thaw_process(p) was called only after the process 'p' was frozen.
Now with hold_freezer_for_task(), we can as well call thaw_process(p)
when 'p' is in the freezing stage. Hence the task_lock.
I know it's ugly, but couldn't think of any other alternative at that time.

> 
> > +int hold_freezer_for_task(struct task_struct *p)
> > +{
> > +	int ret = 0;
> > +	spin_lock(&freezer_status.lock);
> > +	if (freezer_status.count >= 0)
> > +	{
> > +		set_tsk_thread_flag(p, TIF_FREEZER_HELD);
> > +		thaw_process(p);
> > +		freezer_status.count++;
> > +		ret = 1;
> > +	}
> > +	spin_unlock(&freezer_status.lock);
> > +
> > +	return ret;
> > +}
> 
> I think this can work if it is used only in kthread_stop(). But what if
> another task wants to do hold_freezer_for_task(p) ? freezer_status.count
> is recursive, but TIF_FREEZER_HELD is not. IOW, I believe this is not
> generic enough.

Yes. If more than one tasks want another task to be temporarily thawed, this
won't work. I hadn't anticipated such a case. 
> 
> Also, you are planning to add different freezing states (FE_HOTPLUG_CPU,
> FE_SUSPEND, etc). In that case each of them needs a separate .count, because
> it should be negative when try_to_freeze_tasks() returns. Now consider
> the case when we are doing freeze_processes(FE_A | FE_B) ...

So can't we in that case find out the weight of the freeze_event variable and
subtract that weight from the count (if the count is <=0 ) ?
> 
> Oleg.
> 

Thanks for the review.
Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Pavel Machek]

Hi!

> Fix the problem with kthread_stop() that causes the freezer to fail if a
> freezable thread is attempting to stop a frozen one and that may cause the
> freezer to fail if the thread being stopped is freezable and
> try_to_freeze_tasks() is running concurrently with kthread_stop().

Parse error.

> Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
> ===================================================================
> --- linux-2.6.21-rc6-mm1.orig/kernel/kthread.c	2007-04-09 15:23:48.000000000 +0200
> +++ linux-2.6.21-rc6-mm1/kernel/kthread.c	2007-04-22 19:05:29.000000000 +0200
> @@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
>  
>  	/* Now set kthread_should_stop() to true, and wake it up. */
>  	kthread_stop_info.k = k;
> +	if (!freezer_should_exempt(current)) {
> +		/* We are freezable, so we must make sure that the thread being
> +		 * stopped is not frozen and will not be frozen until it dies
> +		 */
> +		freezer_exempt(k);
> +		if (frozen(k))
> +			clear_frozen_flag(k);
> +	}
>  	wake_up_process(k);
>  	put_task_struct(k);
>  

Do we need to take some locks to access other process' flags? Or do
frozen_exempt() etc take enough locks, already?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH -mm 1/3] Separate freezer from PM code

[Pavel Machek]

Hi!

> Now that the freezer is used by kprobes, it is no longer a PM-specific piece of
> code.  Move the freezer code out of kernel/power and introduce the
> CONFIG_FREEZER option that will be chosen automatically if PM or KPROBES is set.
> 
> 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: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Move all of the freezer-related flags to a separate field in task_struct and
> introduce functions to operate them using set_bit() etc.

Looks sane to me.

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

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

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Gautham R Shenoy]

On Sun, Apr 22, 2007 at 09:40:59PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Fix the problem with kthread_stop() that causes the freezer to fail if a
> freezable thread is attempting to stop a frozen one and that may cause the
> freezer to fail if the thread being stopped is freezable and
> try_to_freeze_tasks() is running concurrently with kthread_stop().
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/kthread.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
> ===================================================================
> --- linux-2.6.21-rc6-mm1.orig/kernel/kthread.c	2007-04-09 15:23:48.000000000 +0200
> +++ linux-2.6.21-rc6-mm1/kernel/kthread.c	2007-04-22 19:05:29.000000000 +0200
> @@ -13,6 +13,7 @@
>  #include <linux/file.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/freezer.h>
>  #include <asm/semaphore.h>
> 
>  /*
> @@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
> 
>  	/* Now set kthread_should_stop() to true, and wake it up. */
>  	kthread_stop_info.k = k;
> +	if (!freezer_should_exempt(current)) {
> +		/* We are freezable, so we must make sure that the thread being
> +		 * stopped is not frozen and will not be frozen until it dies
> +		 */
> +		freezer_exempt(k);
> +		if (frozen(k))
> +			clear_frozen_flag(k);
> +	}

I'm trying hard to convince myself that this will work. May be I am
missing something here, but I find a potential race window (very small though) 
when k is entering the refrigerator.

Here's how.

kthread_stop(k)					k->refrigerator()
---------------------------------------------------------------------
						task_lock(k);
						1) check_if_exempted();
						/* not exempted. So 
						 * we will freeze
						 * ourself.
						 */
2) freezer_exempt(k);

3) if(frozen(k))
/* No, we're not yet frozen. So we
 * don't clear_frozen_flag(k) here
 */
						4) frozen_process(k);
						   task_unlock(k);
						
						5) for(;;) {
						 set_current_state(UNINTERRUPTIBLE);
						  if(!frozen_process(k))
						  /* k is frozen. We
						   * don't break :( 
						   */
						 
						  	schedule();
						}
						
>  	wake_up_process(k);
>  	put_task_struct(k);
> 

Thus the freezer can still fail, no?
IMO, we need the to take the task_lock for k here. Something like

> +	if (!freezer_should_exempt(current)) {
		task_lock(k);
> +		/* We are freezable, so we must make sure that the thread being
> +		 * stopped is not frozen and will not be frozen until it dies
> +		 */
> +		freezer_exempt(k);
> +		if (frozen(k))
> +			clear_frozen_flag(k);
		task_unlock(k);
> +	}


Thanks and Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Gautham R Shenoy]

On Sun, Apr 22, 2007 at 09:39:26PM +0200, Rafael J. Wysocki wrote:
> @@ -63,9 +100,9 @@ static inline int thaw_process(struct ta
>   */
>  static inline void frozen_process(struct task_struct *p)
>  {
> -	p->flags |= PF_FROZEN;
> +	set_frozen_flag(p);
>  	wmb();
> -	clear_tsk_thread_flag(p, TIF_FREEZE);
> +	do_not_freeze(p);

We may want to rename do_not_freeze to something else. It kind of
looks weird calling do_not_freeze(p) after setting the frozen flag!

Probably, just a matter of taste :-)

>  }
> 

Thanks and Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Gautham R Shenoy]

Hi Satyam, 
On Mon, Apr 23, 2007 at 09:39:30AM +0530, Satyam Sharma wrote:
> Hi Rafael,
> 
> >+/*
> >+ *     Per task flags used by the freezer
> >+ *
> >+ *     They should not be referred to directly outside of this file.
> >+ */
> >+#define TFF_NOFREEZE   0       /* task should not be frozen */
> >+#define TFF_FREEZE     8       /* task should go to the refrigerator ASAP 
> >*/
> >+#define TFF_SKIP       9       /* do not count this task as freezable */
> >+#define TFF_FROZEN     10      /* task is frozen */
> 
> Aren't NOFREEZE and SKIP doing the same thing? One of them appears
> superfluous. I'm looking at 21-rc6-mm1 and vfork(2) seems to be its
> only user. Seeing how vfork(2) used it, can't the call to
> freezer_do_not_count() be replaced with a call to freezer_exempt()?
> Similarly, the freezer_count() after the wait_for_completion might
> just as well be a clear of the NOFREEZE bit followed by a
> try_to_freeze(). Could you please explain the rationale behind the
> SKIP flag?

The difference between the NOFREEZE and the SKIP flag is a subtle one.

When a task (say p) sets it's NOFREEZE flag, it tells the freezer not to
consider it for freezing. Which means freezeable(p) will return 0.
So the freezer will not even mark it for freezing. 

However, when a task sets it SKIP flag, it tells the freezer - "I might
block at a safe place. So when you are counting the processes which
have been marked as freezeable, but have not frozen yet, please don't
count me in. IOW, please skip me."  
Thus such a task can still be marked for freezing.

The typical usage is 
freezer_do_not_count(current);
/* currents goes to an uninterruptible sleep, like wait_for_completion. */
freezer_count(current);

Once the task wakes up from it's uninterruptible sleep, it will 
call freezer_count which in turn calls try_to_freeze.
If the task was marked for freezing, it will be frozen now.

You may want to check the thread http://lkml.org/lkml/2007/2/18/47 
on how it came into existance.

> 
> Cheers,
> Satyam

Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC PATCH 1/2] Fix PF_NOFREEZE and freezeable race

[Oleg Nesterov]

On 04/23, Gautham R Shenoy wrote:
>
> On Fri, Apr 20, 2007 at 10:02:08PM +0400, Oleg Nesterov wrote:
> > 
> > Gautham, isn't it possible to make a more simpler patch ? Just add PF_NOFREEZE
> > check to frozen_process,
> > 
> > 	static inline void frozen_process(struct task_struct *p)
> > 	{
> > 		if (!unlikely(current->flags & PF_NOFREEZE)) {
> > 			p->flags |= PF_FROZEN;
> > 			wmb();
> > 		}
> > 		clear_tsk_thread_flag(p, TIF_FREEZE);
> > 	}
> > 
> 
> Btw, since frozen_process is currently being called only from
> refrigerator, I am wondering if we still need the struct task_struct *p
> parameter there. It's very unlikely that some other task would mark a
> particular task as frozen. No?

I agree. Only "current" can set PF_FROZEN anyway. I also think it is better
to move this function into kernel/power/process.c, no need to export it in
freezer.h. It is special, should be called from refrigerator() with task_lock()
held.

> Anyways, Andrew, Could you please replace the earlier sent patch titled
> "fix_pf_nofreeze_and_freezeable_race.patch" with the following one?
>
> ...
>
> --- linux-2.6.21-rc6.orig/include/linux/freezer.h
> +++ linux-2.6.21-rc6/include/linux/freezer.h
> @@ -57,8 +57,10 @@ static inline int thaw_process(struct ta
>   */
>  static inline void frozen_process(struct task_struct *p)
>  {
> -	p->flags |= PF_FROZEN;
> -	wmb();
> +	if (!unlikely(p->flags & PF_NOFREEZE)) {
> +		p->flags |= PF_FROZEN;
> +		wmb();
> +	}
>  	clear_tsk_thread_flag(p, TIF_FREEZE);
>  }

On the second thought, this patch doesn't help if the thread never
calls try_to_freeze() exactly because it marks itself PF_NOFREEZE.

Perhaps it is better to cancel freezing in soon-to-be-introduced
freezer_exempt().

In any case I agree, this one is better than current
	fix_pf_nofreeze_and_freezeable_race.patch

Oleg.

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Oleg Nesterov]

On 04/23, Gautham R Shenoy wrote:
>
> On Sat, Apr 21, 2007 at 01:12:09AM +0400, Oleg Nesterov wrote:
> > On 04/19, Gautham R Shenoy wrote:
> > >
> > > @@ -63,12 +74,16 @@ void refrigerator(void)
> > >  	recalc_sigpending(); /* We sent fake signal, clean it up */
> > >  	spin_unlock_irq(&current->sighand->siglock);
> > >  
> > > +	task_lock(current);
> > >  	for (;;) {
> > >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > >  		if (!frozen(current))
> > >  			break;
> > > +		task_unlock(current);
> > >  		schedule();
> > > +		task_lock(current);
> > >  	}
> > > +	task_unlock(current);
> > >  	pr_debug("%s left refrigerator\n", current->comm);
> > >  	current->state = save;
> > 
> > Just curious, why this change?
>
> This can race with hold_freezer_for_task() calling thaw_process. Earlier
> thaw_process(p) was called only after the process 'p' was frozen.
> Now with hold_freezer_for_task(), we can as well call thaw_process(p)
> when 'p' is in the freezing stage. Hence the task_lock.

hold_freezer_for_task()->thaw_process(p) will wake up the task. Or the
caller of refrigerator will notice "!frozen()". Note that refrigerator()
sets PF_FROZEN under task_lock().

In fact we have the same issue when thaw_tasks()->thaw_process(p) happens
when the freezing fails. In that case 'p' may be not frozen.

> > Also, you are planning to add different freezing states (FE_HOTPLUG_CPU,
> > FE_SUSPEND, etc). In that case each of them needs a separate .count, because
> > it should be negative when try_to_freeze_tasks() returns. Now consider
> > the case when we are doing freeze_processes(FE_A | FE_B) ...
> 
> So can't we in that case find out the weight of the freeze_event variable and
> subtract that weight from the count (if the count is <=0 ) ?

Probably yes... but if we are speaking about kthrad_stop() only, this could
be afaics solved in more simple way, as Rafael suggests.

Oleg.

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

On Monday, 23 April 2007 16:19, Gautham R Shenoy wrote:
> Hi Satyam, 
> On Mon, Apr 23, 2007 at 09:39:30AM +0530, Satyam Sharma wrote:
> > Hi Rafael,
> > 
> > >+/*
> > >+ *     Per task flags used by the freezer
> > >+ *
> > >+ *     They should not be referred to directly outside of this file.
> > >+ */
> > >+#define TFF_NOFREEZE   0       /* task should not be frozen */
> > >+#define TFF_FREEZE     8       /* task should go to the refrigerator ASAP 
> > >*/
> > >+#define TFF_SKIP       9       /* do not count this task as freezable */
> > >+#define TFF_FROZEN     10      /* task is frozen */
> > 
> > Aren't NOFREEZE and SKIP doing the same thing? One of them appears
> > superfluous. I'm looking at 21-rc6-mm1 and vfork(2) seems to be its
> > only user. Seeing how vfork(2) used it, can't the call to
> > freezer_do_not_count() be replaced with a call to freezer_exempt()?
> > Similarly, the freezer_count() after the wait_for_completion might
> > just as well be a clear of the NOFREEZE bit followed by a
> > try_to_freeze(). Could you please explain the rationale behind the
> > SKIP flag?
> 
> The difference between the NOFREEZE and the SKIP flag is a subtle one.
> 
> When a task (say p) sets it's NOFREEZE flag, it tells the freezer not to
> consider it for freezing. Which means freezeable(p) will return 0.
> So the freezer will not even mark it for freezing. 
> 
> However, when a task sets it SKIP flag, it tells the freezer - "I might
> block at a safe place. So when you are counting the processes which
> have been marked as freezeable, but have not frozen yet, please don't
> count me in. IOW, please skip me."  
> Thus such a task can still be marked for freezing.
> 
> The typical usage is 
> freezer_do_not_count(current);
> /* currents goes to an uninterruptible sleep, like wait_for_completion. */
> freezer_count(current);
> 
> Once the task wakes up from it's uninterruptible sleep, it will 
> call freezer_count which in turn calls try_to_freeze.
> If the task was marked for freezing, it will be frozen now.
> 
> You may want to check the thread http://lkml.org/lkml/2007/2/18/47 
> on how it came into existance.

Very well explained.  Thanks!

Greetings,
Rafael

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Oleg Nesterov]

On 04/23, Gautham R Shenoy wrote:
>
> On Sun, Apr 22, 2007 at 09:40:59PM +0200, Rafael J. Wysocki wrote:
> >  /*
> > @@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
> > 
> >  	/* Now set kthread_should_stop() to true, and wake it up. */
> >  	kthread_stop_info.k = k;
> > +	if (!freezer_should_exempt(current)) {
> > +		/* We are freezable, so we must make sure that the thread being
> > +		 * stopped is not frozen and will not be frozen until it dies
> > +		 */
> > +		freezer_exempt(k);
> > +		if (frozen(k))
> > +			clear_frozen_flag(k);
> > +	}
> 
> I'm trying hard to convince myself that this will work. May be I am
> missing something here, but I find a potential race window (very small though) 
> when k is entering the refrigerator.
> 
> [... snip ... ]
> 
> IMO, we need the to take the task_lock for k here. Something like
> 
> > +	if (!freezer_should_exempt(current)) {
> 		task_lock(k);
> > +		/* We are freezable, so we must make sure that the thread being
> > +		 * stopped is not frozen and will not be frozen until it dies
> > +		 */
> > +		freezer_exempt(k);
> > +		if (frozen(k))
> > +			clear_frozen_flag(k);
> 		task_unlock(k);
> > +	}

Well, probably I missed something, but why can't we do

	if (!freezer_should_exempt(current)) {
		freezer_exempt(k);
		thaw_process(k);
	}
?

thaw_process(k) is properly serialized with refrigerator(), and it checks
frozen(k). We can make an extra wake_up, but this should not matter.

Rafael, please check the recent changes in kthread.c, kthread_stop() was
reworked, we don't have kthread_stop_info any longer.

Oleg.

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

Hi,

On Monday, 23 April 2007 06:09, Satyam Sharma wrote:
> Hi Rafael,
> 
[--snip--]
> 
> Also, I do have several gripes against the naming of some of these functions:
> 
> >  static inline int freezing(struct task_struct *p)
> 
> This could be called task_should_freeze().
> 
> >  /*
> > - * Sometimes we may need to cancel the previous 'freeze' request
> > + * Cancel the previous 'freeze' request
> >   */
> >  static inline void do_not_freeze(struct task_struct *p)
> 
> This definitely needs to be undo_freeze() or unfreeze().
> do_not_freeze() sounds like what freeze_exempt() does.
> 
> >  static inline void frozen_process(struct task_struct *p)
> 
> frozen_process() sounds like what frozen() is supposed to do. This
> could instead be mark_task_frozen(), or even mark_frozen(), because
> only the current task can ever mark *itself* frozen before freezing
> itself.
> 
> >  static inline void freezer_do_not_count(void)
> >  static inline void freezer_count(void)
> 
> These could be called freezer_skip() and freezer_do_not_skip(). Better
> to stick to consistent naming / terminology.

Well, I agree that the names are not the best ones, but I'd like to avoid
changing the existing names in this particular patch.

We can change them later, on top of it.

Greetings,
Rafael

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Rafael J. Wysocki]

Hi,

On Monday, 23 April 2007 12:40, Pavel Machek wrote:
> Hi!
> 
> > Fix the problem with kthread_stop() that causes the freezer to fail if a
> > freezable thread is attempting to stop a frozen one and that may cause the
> > freezer to fail if the thread being stopped is freezable and
> > try_to_freeze_tasks() is running concurrently with kthread_stop().
> 
> Parse error.

OK, will fix.

> > Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
> > ===================================================================
> > --- linux-2.6.21-rc6-mm1.orig/kernel/kthread.c	2007-04-09 15:23:48.000000000 +0200
> > +++ linux-2.6.21-rc6-mm1/kernel/kthread.c	2007-04-22 19:05:29.000000000 +0200
> > @@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
> >  
> >  	/* Now set kthread_should_stop() to true, and wake it up. */
> >  	kthread_stop_info.k = k;
> > +	if (!freezer_should_exempt(current)) {
> > +		/* We are freezable, so we must make sure that the thread being
> > +		 * stopped is not frozen and will not be frozen until it dies
> > +		 */
> > +		freezer_exempt(k);
> > +		if (frozen(k))
> > +			clear_frozen_flag(k);
> > +	}
> >  	wake_up_process(k);
> >  	put_task_struct(k);
> >  
> 
> Do we need to take some locks to access other process' flags? Or do
> frozen_exempt() etc take enough locks, already?

After the previous patch we only use set_bit(), clear_bit() and test_bit() to
access freezer_falgs, so no special locking is needed to protect them.

Greetings,
Rafael

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Rafael J. Wysocki]

On Monday, 23 April 2007 14:35, Gautham R Shenoy wrote:
> On Sun, Apr 22, 2007 at 09:40:59PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Fix the problem with kthread_stop() that causes the freezer to fail if a
> > freezable thread is attempting to stop a frozen one and that may cause the
> > freezer to fail if the thread being stopped is freezable and
> > try_to_freeze_tasks() is running concurrently with kthread_stop().
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/kthread.c |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
> > ===================================================================
> > --- linux-2.6.21-rc6-mm1.orig/kernel/kthread.c	2007-04-09 15:23:48.000000000 +0200
> > +++ linux-2.6.21-rc6-mm1/kernel/kthread.c	2007-04-22 19:05:29.000000000 +0200
> > @@ -13,6 +13,7 @@
> >  #include <linux/file.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > +#include <linux/freezer.h>
> >  #include <asm/semaphore.h>
> > 
> >  /*
> > @@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
> > 
> >  	/* Now set kthread_should_stop() to true, and wake it up. */
> >  	kthread_stop_info.k = k;
> > +	if (!freezer_should_exempt(current)) {
> > +		/* We are freezable, so we must make sure that the thread being
> > +		 * stopped is not frozen and will not be frozen until it dies
> > +		 */
> > +		freezer_exempt(k);
> > +		if (frozen(k))
> > +			clear_frozen_flag(k);
> > +	}
> 
> I'm trying hard to convince myself that this will work. May be I am
> missing something here, but I find a potential race window (very small though) 
> when k is entering the refrigerator.
> 
> Here's how.
> 
> kthread_stop(k)					k->refrigerator()
> ---------------------------------------------------------------------
> 						task_lock(k);
> 						1) check_if_exempted();
> 						/* not exempted. So 
> 						 * we will freeze
> 						 * ourself.
> 						 */
> 2) freezer_exempt(k);
> 
> 3) if(frozen(k))
> /* No, we're not yet frozen. So we
>  * don't clear_frozen_flag(k) here
>  */
> 						4) frozen_process(k);
> 						   task_unlock(k);
> 						
> 						5) for(;;) {
> 						 set_current_state(UNINTERRUPTIBLE);
> 						  if(!frozen_process(k))
> 						  /* k is frozen. We
> 						   * don't break :( 
> 						   */
> 						 
> 						  	schedule();
> 						}
> 						
> >  	wake_up_process(k);
> >  	put_task_struct(k);
> > 
> 
> Thus the freezer can still fail, no?
> IMO, we need the to take the task_lock for k here. Something like
> 
> > +	if (!freezer_should_exempt(current)) {
> 		task_lock(k);
> > +		/* We are freezable, so we must make sure that the thread being
> > +		 * stopped is not frozen and will not be frozen until it dies
> > +		 */
> > +		freezer_exempt(k);
> > +		if (frozen(k))
> > +			clear_frozen_flag(k);
> 		task_unlock(k);
> > +	}

Yes, that's correct.  We need to take task_lock() to avoid the race with
refrigerator().

I'll fix the patch.

BTW, I think I should rediff the entire series against -mm with your patch from
http://lkml.org/lkml/2007/4/23/98 applied.

Greetings,
Rafael

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

On Monday, 23 April 2007 15:17, Gautham R Shenoy wrote:
> On Sun, Apr 22, 2007 at 09:39:26PM +0200, Rafael J. Wysocki wrote:
> > @@ -63,9 +100,9 @@ static inline int thaw_process(struct ta
> >   */
> >  static inline void frozen_process(struct task_struct *p)
> >  {
> > -	p->flags |= PF_FROZEN;
> > +	set_frozen_flag(p);
> >  	wmb();
> > -	clear_tsk_thread_flag(p, TIF_FREEZE);
> > +	do_not_freeze(p);
> 
> We may want to rename do_not_freeze to something else. It kind of
> looks weird calling do_not_freeze(p) after setting the frozen flag!
> 
> Probably, just a matter of taste :-)

Hmm, I wanted to avoid changing the existing names in this patch, but in this
particular case you are obviously right.  I'll change that to
clear_freeze_flag().

Greetings,
Rafael

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Rafael J. Wysocki]

On Monday, 23 April 2007 21:03, Oleg Nesterov wrote:
> On 04/23, Gautham R Shenoy wrote:
> >
> > On Sun, Apr 22, 2007 at 09:40:59PM +0200, Rafael J. Wysocki wrote:
> > >  /*
> > > @@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
> > > 
> > >  	/* Now set kthread_should_stop() to true, and wake it up. */
> > >  	kthread_stop_info.k = k;
> > > +	if (!freezer_should_exempt(current)) {
> > > +		/* We are freezable, so we must make sure that the thread being
> > > +		 * stopped is not frozen and will not be frozen until it dies
> > > +		 */
> > > +		freezer_exempt(k);
> > > +		if (frozen(k))
> > > +			clear_frozen_flag(k);
> > > +	}
> > 
> > I'm trying hard to convince myself that this will work. May be I am
> > missing something here, but I find a potential race window (very small though) 
> > when k is entering the refrigerator.
> > 
> > [... snip ... ]
> > 
> > IMO, we need the to take the task_lock for k here. Something like
> > 
> > > +	if (!freezer_should_exempt(current)) {
> > 		task_lock(k);
> > > +		/* We are freezable, so we must make sure that the thread being
> > > +		 * stopped is not frozen and will not be frozen until it dies
> > > +		 */
> > > +		freezer_exempt(k);
> > > +		if (frozen(k))
> > > +			clear_frozen_flag(k);
> > 		task_unlock(k);
> > > +	}
> 
> Well, probably I missed something, but why can't we do
> 
> 	if (!freezer_should_exempt(current)) {
> 		freezer_exempt(k);
> 		thaw_process(k);
> 	}
> ?
> 
> thaw_process(k) is properly serialized with refrigerator(), and it checks
> frozen(k). We can make an extra wake_up, but this should not matter.

Yes, I think you're right.

> Rafael, please check the recent changes in kthread.c, kthread_stop() was
> reworked, we don't have kthread_stop_info any longer.

OK, I will, thanks.

Greetings,
Rafael

Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

[Gautham R Shenoy]

On Mon, Apr 23, 2007 at 10:39:56PM +0400, Oleg Nesterov wrote:
> On 04/23, Gautham R Shenoy wrote:
> >
> > On Sat, Apr 21, 2007 at 01:12:09AM +0400, Oleg Nesterov wrote:
> > > On 04/19, Gautham R Shenoy wrote:
> > > >
> > > > @@ -63,12 +74,16 @@ void refrigerator(void)
> > > >  	recalc_sigpending(); /* We sent fake signal, clean it up */
> > > >  	spin_unlock_irq(&current->sighand->siglock);
> > > >  
> > > > +	task_lock(current);
> > > >  	for (;;) {
> > > >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > > >  		if (!frozen(current))
> > > >  			break;
> > > > +		task_unlock(current);
> > > >  		schedule();
> > > > +		task_lock(current);
> > > >  	}
> > > > +	task_unlock(current);
> > > >  	pr_debug("%s left refrigerator\n", current->comm);
> > > >  	current->state = save;
> > > 
> > > Just curious, why this change?
> >
> > This can race with hold_freezer_for_task() calling thaw_process. Earlier
> > thaw_process(p) was called only after the process 'p' was frozen.
> > Now with hold_freezer_for_task(), we can as well call thaw_process(p)
> > when 'p' is in the freezing stage. Hence the task_lock.
> 
> hold_freezer_for_task()->thaw_process(p) will wake up the task. Or the
> caller of refrigerator will notice "!frozen()". Note that refrigerator()
> sets PF_FROZEN under task_lock().
> 
> In fact we have the same issue when thaw_tasks()->thaw_process(p) happens
> when the freezing fails. In that case 'p' may be not frozen.

Yes. I guess I was being too cautious :)

> 
> > > Also, you are planning to add different freezing states (FE_HOTPLUG_CPU,
> > > FE_SUSPEND, etc). In that case each of them needs a separate .count, because
> > > it should be negative when try_to_freeze_tasks() returns. Now consider
> > > the case when we are doing freeze_processes(FE_A | FE_B) ...
> > 
> > So can't we in that case find out the weight of the freeze_event variable and
> > subtract that weight from the count (if the count is <=0 ) ?
> 
> Probably yes... but if we are speaking about kthrad_stop() only, this could
> be afaics solved in more simple way, as Rafael suggests.

I agree on this one. kthread_stop appears to be the only valid place
where such a correction is required. So Rafael's approach makes sense
here.

> 
> Oleg.
> 
Thanks and Regards
gautham.

-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Oleg Nesterov]

On 04/23, Rafael J. Wysocki wrote:
>
> On Monday, 23 April 2007 14:35, Gautham R Shenoy wrote:
> > > +	if (!freezer_should_exempt(current)) {
> > 		task_lock(k);
> > > +		/* We are freezable, so we must make sure that the thread being
> > > +		 * stopped is not frozen and will not be frozen until it dies
> > > +		 */
> > > +		freezer_exempt(k);
> > > +		if (frozen(k))
> > > +			clear_frozen_flag(k);
> > 		task_unlock(k);
> > > +	}
> 
> Yes, that's correct.  We need to take task_lock() to avoid the race with
> refrigerator().

Even if we use thaw_task() ?

Even if I am wrong, I think we should not use task_lock() for the freezing
related code, except in freezer.[ch]

Note also that without CONFIG_FREEZER freezer_should_exempt() == 0, so we
will do unneeded task_lock/task_unlock.

Oleg.

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Gautham R Shenoy]

On Tue, Apr 24, 2007 at 12:46:37AM +0400, Oleg Nesterov wrote:
> On 04/23, Rafael J. Wysocki wrote:
> >
> > On Monday, 23 April 2007 14:35, Gautham R Shenoy wrote:
> > > > +	if (!freezer_should_exempt(current)) {
> > > 		task_lock(k);
> > > > +		/* We are freezable, so we must make sure that the thread being
> > > > +		 * stopped is not frozen and will not be frozen until it dies
> > > > +		 */
> > > > +		freezer_exempt(k);
> > > > +		if (frozen(k))
> > > > +			clear_frozen_flag(k);
> > > 		task_unlock(k);
> > > > +	}
> > 
> > Yes, that's correct.  We need to take task_lock() to avoid the race with
> > refrigerator().
> 
> Even if we use thaw_task() ?

I don't think so. As you correctly pointed out, thaw_task() is race free
w.r.t the refrigerator(). 

> 
> Even if I am wrong, I think we should not use task_lock() for the freezing
> related code, except in freezer.[ch]
> 
> Note also that without CONFIG_FREEZER freezer_should_exempt() == 0, so we
> will do unneeded task_lock/task_unlock.
> 
> Oleg.
> 

Thanks and Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC][PATCH -mm 3/3] freezer: Fix problem with kthread_stop

[Rafael J. Wysocki]

On Monday, 23 April 2007 23:16, Gautham R Shenoy wrote:
> On Tue, Apr 24, 2007 at 12:46:37AM +0400, Oleg Nesterov wrote:
> > On 04/23, Rafael J. Wysocki wrote:
> > >
> > > On Monday, 23 April 2007 14:35, Gautham R Shenoy wrote:
> > > > > +	if (!freezer_should_exempt(current)) {
> > > > 		task_lock(k);
> > > > > +		/* We are freezable, so we must make sure that the thread being
> > > > > +		 * stopped is not frozen and will not be frozen until it dies
> > > > > +		 */
> > > > > +		freezer_exempt(k);
> > > > > +		if (frozen(k))
> > > > > +			clear_frozen_flag(k);
> > > > 		task_unlock(k);
> > > > > +	}
> > > 
> > > Yes, that's correct.  We need to take task_lock() to avoid the race with
> > > refrigerator().
> > 
> > Even if we use thaw_task() ?
> 
> I don't think so. As you correctly pointed out, thaw_task() is race free
> w.r.t the refrigerator(). 

Agreed.

Rafael

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Oleg Nesterov]

On 04/22, Rafael J. Wysocki wrote:
> 
> Move all of the freezer-related flags to a separate field in task_struct and
> introduce functions to operate them using set_bit() etc.
>
> [...snip...]
>
> --- linux-2.6.21-rc6-mm1.orig/kernel/fork.c	2007-04-22 19:37:42.000000000 +0200
> +++ linux-2.6.21-rc6-mm1/kernel/fork.c	2007-04-22 20:55:01.000000000 +0200
> @@ -922,7 +922,7 @@ static inline void copy_flags(unsigned l
>  {
>  	unsigned long new_flags = p->flags;
>  
> -	new_flags &= ~(PF_SUPERPRIV | PF_NOFREEZE);
> +	new_flags &= ~PF_SUPERPRIV;
>  	new_flags |= PF_FORKNOEXEC;
>  	new_flags |= PF_STARTING;
>  	p->flags = new_flags;

OK, but you forgot to clear p->freezer_flags on copy_process()->dup_task_struct()
path ?

Oleg.

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

On Tuesday, 24 April 2007 00:23, Oleg Nesterov wrote:
> On 04/22, Rafael J. Wysocki wrote:
> > 
> > Move all of the freezer-related flags to a separate field in task_struct and
> > introduce functions to operate them using set_bit() etc.
> >
> > [...snip...]
> >
> > --- linux-2.6.21-rc6-mm1.orig/kernel/fork.c	2007-04-22 19:37:42.000000000 +0200
> > +++ linux-2.6.21-rc6-mm1/kernel/fork.c	2007-04-22 20:55:01.000000000 +0200
> > @@ -922,7 +922,7 @@ static inline void copy_flags(unsigned l
> >  {
> >  	unsigned long new_flags = p->flags;
> >  
> > -	new_flags &= ~(PF_SUPERPRIV | PF_NOFREEZE);
> > +	new_flags &= ~PF_SUPERPRIV;
> >  	new_flags |= PF_FORKNOEXEC;
> >  	new_flags |= PF_STARTING;
> >  	p->flags = new_flags;
> 
> OK, but you forgot to clear p->freezer_flags on copy_process()->dup_task_struct()
> path ?

Yes, thanks for pointing that out.

Should I clear it in dup_task_struct() or is there a better place?

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Gautham R Shenoy]

On Tue, Apr 24, 2007 at 12:40:17AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, 24 April 2007 00:23, Oleg Nesterov wrote:
> > On 04/22, Rafael J. Wysocki wrote:
> > > 
> > > Move all of the freezer-related flags to a separate field in task_struct and
> > > introduce functions to operate them using set_bit() etc.
> > >
> > > [...snip...]
> > >
> > > --- linux-2.6.21-rc6-mm1.orig/kernel/fork.c	2007-04-22 19:37:42.000000000 +0200
> > > +++ linux-2.6.21-rc6-mm1/kernel/fork.c	2007-04-22 20:55:01.000000000 +0200
> > > @@ -922,7 +922,7 @@ static inline void copy_flags(unsigned l
> > >  {
> > >  	unsigned long new_flags = p->flags;
> > >  
> > > -	new_flags &= ~(PF_SUPERPRIV | PF_NOFREEZE);
> > > +	new_flags &= ~PF_SUPERPRIV;
> > >  	new_flags |= PF_FORKNOEXEC;
> > >  	new_flags |= PF_STARTING;
> > >  	p->flags = new_flags;
> > 
> > OK, but you forgot to clear p->freezer_flags on copy_process()->dup_task_struct()
> > path ?
> 
> Yes, thanks for pointing that out.

That reminds me, shouldn't we set the child's TFF_FREEZE flag if the parent
is freezing or frozen? 

> 
> Should I clear it in dup_task_struct() or is there a better place?

Thanks and Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

On Tuesday, 24 April 2007 00:41, Gautham R Shenoy wrote:
> On Tue, Apr 24, 2007 at 12:40:17AM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, 24 April 2007 00:23, Oleg Nesterov wrote:
> > > On 04/22, Rafael J. Wysocki wrote:
> > > > 
> > > > Move all of the freezer-related flags to a separate field in task_struct and
> > > > introduce functions to operate them using set_bit() etc.
> > > >
> > > > [...snip...]
> > > >
> > > > --- linux-2.6.21-rc6-mm1.orig/kernel/fork.c	2007-04-22 19:37:42.000000000 +0200
> > > > +++ linux-2.6.21-rc6-mm1/kernel/fork.c	2007-04-22 20:55:01.000000000 +0200
> > > > @@ -922,7 +922,7 @@ static inline void copy_flags(unsigned l
> > > >  {
> > > >  	unsigned long new_flags = p->flags;
> > > >  
> > > > -	new_flags &= ~(PF_SUPERPRIV | PF_NOFREEZE);
> > > > +	new_flags &= ~PF_SUPERPRIV;
> > > >  	new_flags |= PF_FORKNOEXEC;
> > > >  	new_flags |= PF_STARTING;
> > > >  	p->flags = new_flags;
> > > 
> > > OK, but you forgot to clear p->freezer_flags on copy_process()->dup_task_struct()
> > > path ?
> > 
> > Yes, thanks for pointing that out.
> 
> That reminds me, shouldn't we set the child's TFF_FREEZE flag if the parent
> is freezing or frozen? 

Well, usually we'll want to freeze the child too in such cases, but I'm not
sure if that's always true.

Greetings,
Rafael

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Oleg Nesterov]

On 04/24, Rafael J. Wysocki wrote:
>
> Should I clear it in dup_task_struct() or is there a better place?

I personally think we should do this in dup_task_struct(). In fact, I believe
it is better to replace the

	*tsk = *orig;

with some helper (like setup_thread_stack() below), and that helper clears
->freezer_flags. Say, copy_task_struct().

Oleg.

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

On Tuesday, 24 April 2007 00:55, Oleg Nesterov wrote:
> On 04/24, Rafael J. Wysocki wrote:
> >
> > Should I clear it in dup_task_struct() or is there a better place?
> 
> I personally think we should do this in dup_task_struct(). In fact, I believe
> it is better to replace the
> 
> 	*tsk = *orig;
> 
> with some helper (like setup_thread_stack() below), and that helper clears
> ->freezer_flags. Say, copy_task_struct().

Hmm, wouldn't that be overkill?  copy_task_struct() would have to do
*tsk = *orig anyway, and we only need to clear one field apart from this.

Some other fields are cleared towards the end of dup_task_struct(), so perhaps
we could clear freezer_flags in there too?

Rafael

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Oleg Nesterov]

On 04/24, Rafael J. Wysocki wrote:
>
> On Tuesday, 24 April 2007 00:55, Oleg Nesterov wrote:
> > On 04/24, Rafael J. Wysocki wrote:
> > >
> > > Should I clear it in dup_task_struct() or is there a better place?
> > 
> > I personally think we should do this in dup_task_struct(). In fact, I believe
> > it is better to replace the
> > 
> > 	*tsk = *orig;
> > 
> > with some helper (like setup_thread_stack() below), and that helper clears
> > ->freezer_flags. Say, copy_task_struct().
> 
> Hmm, wouldn't that be overkill?  copy_task_struct() would have to do
> *tsk = *orig anyway, and we only need to clear one field apart from this.
> 
> Some other fields are cleared towards the end of dup_task_struct(), so perhaps
> we could clear freezer_flags in there too?

Yes. And I strongly believe it is bad we don't have the helper which does some
random stuf like "p->did_exec = 0".

The same for thread_info. Could you answer quickly where do we clear TIF_FREEZE
currently? We don't.

Oleg.

Re: [RFC][PATCH -mm 2/3] freezer: Introduce freezer_flags

[Rafael J. Wysocki]

On Tuesday, 24 April 2007 01:19, Oleg Nesterov wrote:
> On 04/24, Rafael J. Wysocki wrote:
> >
> > On Tuesday, 24 April 2007 00:55, Oleg Nesterov wrote:
> > > On 04/24, Rafael J. Wysocki wrote:
> > > >
> > > > Should I clear it in dup_task_struct() or is there a better place?
> > > 
> > > I personally think we should do this in dup_task_struct(). In fact, I believe
> > > it is better to replace the
> > > 
> > > 	*tsk = *orig;
> > > 
> > > with some helper (like setup_thread_stack() below), and that helper clears
> > > ->freezer_flags. Say, copy_task_struct().
> > 
> > Hmm, wouldn't that be overkill?  copy_task_struct() would have to do
> > *tsk = *orig anyway, and we only need to clear one field apart from this.
> > 
> > Some other fields are cleared towards the end of dup_task_struct(), so perhaps
> > we could clear freezer_flags in there too?
> 
> Yes. And I strongly believe it is bad we don't have the helper which does some
> random stuf like "p->did_exec = 0".
> 
> The same for thread_info. Could you answer quickly where do we clear TIF_FREEZE
> currently? We don't.

Right.

That said, I think the right thing to do would be to add just one line
'freezer_flags = 0' to dup_task_struct() directly in this patch and then
move all of the random stuff (including this line) to the helper in a separate
patch.

I mean, the introduction of the helper seems to be logically unrelated to the
other things this patch does, so it should go in another patch IMHO.

Greetings,
Rafael

Re: [PATCH -mm] Move frozen_process() to kernel/power/process.c

[Oleg Nesterov]

On 04/26, Gautham R Shenoy wrote:
> 
> Other than refrigerator, no one else calls frozen_process(). So move it from
> include/linux/freezer.h to kernel/power/process.c.

Could you also remove this

	static inline void frozen_process(struct task_struct *p) { BUG(); }

from freezer.h ?

Oleg.

[PATCH -mm] Move frozen_process() to kernel/power/process.c

[Gautham R Shenoy]

Hi Andrew, 
Here's the patch against 2.6.21-rc7-mm2.

-->

Other than refrigerator, no one else calls frozen_process(). So move it from 
include/linux/freezer.h to kernel/power/process.c.

Also, since a task can be marked frozen by itself, we don't need to pass
the (struct task_struct *p) parameter to frozen_process().

Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
 include/linux/freezer.h |   12 ------------
 kernel/power/process.c  |   14 +++++++++++++-
 2 files changed, 13 insertions(+), 13 deletions(-)

Index: linux-2.6.21-rc7/include/linux/freezer.h
===================================================================
--- linux-2.6.21-rc7.orig/include/linux/freezer.h
+++ linux-2.6.21-rc7/include/linux/freezer.h
@@ -58,18 +58,6 @@ static inline int thaw_process(struct ta
 	return 0;
 }
 
-/*
- * freezing is complete, mark process as frozen
- */
-static inline void frozen_process(struct task_struct *p)
-{
-	if (!unlikely(p->flags & PF_NOFREEZE)) {
-		p->flags |= PF_FROZEN;
-		wmb();
-	}
-	clear_tsk_thread_flag(p, TIF_FREEZE);
-}
-
 extern void refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
Index: linux-2.6.21-rc7/kernel/power/process.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/power/process.c
+++ linux-2.6.21-rc7/kernel/power/process.c
@@ -32,6 +32,18 @@ static inline int freezeable(struct task
 	return 1;
 }
 
+/*
+ * freezing is complete, mark current process as frozen
+ */
+static inline void frozen_process(void)
+{
+	if (!unlikely(current->flags & PF_NOFREEZE)) {
+		current->flags |= PF_FROZEN;
+		wmb();
+	}
+	clear_tsk_thread_flag(current, TIF_FREEZE);
+}
+
 /* Refrigerator is place where frozen processes are stored :-). */
 void refrigerator(void)
 {
@@ -41,7 +53,7 @@ void refrigerator(void)
 
 	task_lock(current);
 	if (freezing(current)) {
-		frozen_process(current);
+		frozen_process();
 		task_unlock(current);
 	} else {
 		task_unlock(current);
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [PATCH -mm] Move frozen_process() to kernel/power/process.c

[Gautham R Shenoy]

On Thu, Apr 26, 2007 at 04:36:09PM +0400, Oleg Nesterov wrote:
> On 04/26, Gautham R Shenoy wrote:
> > 
> > Other than refrigerator, no one else calls frozen_process(). So move it from
> > include/linux/freezer.h to kernel/power/process.c.
> 
> Could you also remove this
> 
> 	static inline void frozen_process(struct task_struct *p) { BUG(); }
> 
> from freezer.h ?

Sure. Here's a more complete patch.

Thanks and Regards
gautham.

-->
Other than refrigerator, no one else calls frozen_process(). So move it
from include/linux/freezer.h to kernel/power/process.c.

Also, since a task can be marked frozen by itself, we don't need to pass
the (struct task_struct *p) parameter to frozen_process().

Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
 include/linux/freezer.h |   13 -------------
 kernel/power/process.c  |   14 +++++++++++++-
 2 files changed, 13 insertions(+), 14 deletions(-)

Index: linux-2.6.21-rc7/include/linux/freezer.h
===================================================================
--- linux-2.6.21-rc7.orig/include/linux/freezer.h
+++ linux-2.6.21-rc7/include/linux/freezer.h
@@ -58,18 +58,6 @@ static inline int thaw_process(struct ta
 	return 0;
 }
 
-/*
- * freezing is complete, mark process as frozen
- */
-static inline void frozen_process(struct task_struct *p)
-{
-	if (!unlikely(p->flags & PF_NOFREEZE)) {
-		p->flags |= PF_FROZEN;
-		wmb();
-	}
-	clear_tsk_thread_flag(p, TIF_FREEZE);
-}
-
 extern void refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
@@ -132,7 +120,6 @@ static inline int frozen(struct task_str
 static inline int freezing(struct task_struct *p) { return 0; }
 static inline void freeze(struct task_struct *p) { BUG(); }
 static inline int thaw_process(struct task_struct *p) { return 1; }
-static inline void frozen_process(struct task_struct *p) { BUG(); }
 
 static inline void refrigerator(void) {}
 static inline int freeze_processes(void) { BUG(); return 0; }
Index: linux-2.6.21-rc7/kernel/power/process.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/power/process.c
+++ linux-2.6.21-rc7/kernel/power/process.c
@@ -32,6 +32,18 @@ static inline int freezeable(struct task
 	return 1;
 }
 
+/*
+ * freezing is complete, mark current process as frozen
+ */
+static inline void frozen_process(void)
+{
+	if (!unlikely(current->flags & PF_NOFREEZE)) {
+		current->flags |= PF_FROZEN;
+		wmb();
+	}
+	clear_tsk_thread_flag(current, TIF_FREEZE);
+}
+
 /* Refrigerator is place where frozen processes are stored :-). */
 void refrigerator(void)
 {
@@ -41,7 +53,7 @@ void refrigerator(void)
 
 	task_lock(current);
 	if (freezing(current)) {
-		frozen_process(current);
+		frozen_process();
 		task_unlock(current);
 	} else {
 		task_unlock(current);

-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"