[RFC][PATCH 0/2] Freezer face lifting

[RFC][PATCH 0/2] Freezer face lifting

[Rafael J. Wysocki]

Hi,

Although the freezer is generally considered as a bad thing, it still is
being used for suspend and hibernation and will be used for these purposes in
the near future.  For this reason, it seems reasonable to try to reduce some
known problems with it.  The following two patches attempt to do that.

First, it is the known weakness of the freezer that it tries to distinguish
user space processes from kernel threads in a slightly artificial way.  The
first patch is intended to change that.

Second, in principle, the freezer can use the recently introduced TASK_KILLABLE
state to handle some cases that it couldn't have handled previously, which is
the purpose of the second patch.

Please review and tell me what you think.

Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[RFC][PATCH 1/2] Freezer: Introduce PF_FREEZER_NOSIG

[Rafael J. Wysocki]

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

The freezer currently attempts to distinguish kernel threads from
user space tasks by checking if their mm pointer is unset and it
does not send fake signals to kernel threads.  However, there are
kernel threads, mostly related to networking, that behave like
user space tasks and may want to be sent a fake signal to be frozen.

Introduce the new process flag PF_FREEZER_NOSIG that will be set
by default for all kernel threads and make the freezer only send
fake signals to the tasks having PF_FREEZER_NOSIG unset.  Provide
the set_freezable_with_signal() function to be called by the kernel
threads that want to be sent a fake signal for freezing.

This patch should not change the freezer's observable behavior.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |   10 ++++
 include/linux/sched.h   |    1 
 kernel/kthread.c        |    2 
 kernel/power/process.c  |   97 ++++++++++++++++++++----------------------------
 4 files changed, 54 insertions(+), 56 deletions(-)

Index: linux-2.6/include/linux/freezer.h
===================================================================
--- linux-2.6.orig/include/linux/freezer.h
+++ linux-2.6/include/linux/freezer.h
@@ -128,6 +128,15 @@ static inline void set_freezable(void)
 }
 
 /*
+ * Tell the freezer that the current task should be frozen by it and that it
+ * should send a fake signal to the task to freeze it.
+ */
+static inline void set_freezable_with_signal(void)
+{
+	current->flags &= ~(PF_NOFREEZE | PF_FREEZER_NOSIG);
+}
+
+/*
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */
@@ -174,6 +183,7 @@ static inline void freezer_do_not_count(
 static inline void freezer_count(void) {}
 static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
+static inline void set_freezable_with_signal(void) {}
 
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)
Index: linux-2.6/include/linux/sched.h
===================================================================
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -1508,6 +1508,7 @@ static inline void put_task_struct(struc
 #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 */
+#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
Index: linux-2.6/kernel/power/process.c
===================================================================
--- linux-2.6.orig/kernel/power/process.c
+++ linux-2.6/kernel/power/process.c
@@ -19,9 +19,6 @@
  */
 #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) ||
@@ -84,63 +81,53 @@ static void fake_signal_wake_up(struct t
 	spin_unlock_irqrestore(&p->sighand->siglock, flags);
 }
 
-static int has_mm(struct task_struct *p)
+static inline bool should_send_signal(struct task_struct *p)
 {
-	return (p->mm && !(p->flags & PF_BORROWED_MM));
+	return !(current->flags & PF_FREEZER_NOSIG);
 }
 
 /**
  *	freeze_task - send a freeze request to given task
  *	@p: task to send the request to
- *	@with_mm_only: if set, the request will only be sent if the task has its
- *		own mm
- *	Return value: 0, if @with_mm_only is set and the task has no mm of its
- *		own or the task is frozen, 1, otherwise
+ *	@sig_only: if set, the request will only be sent if the task has the
+ *		PF_FREEZER_NOSIG flag unset
+ *	Return value: 'false', if @sig_only is set and the task has
+ *		PF_FREEZER_NOSIG set or the task is frozen, 'true', otherwise
  *
- *	The freeze request is sent by seting the tasks's TIF_FREEZE flag and
+ *	The freeze request is sent by setting the tasks's TIF_FREEZE flag and
  *	either sending a fake signal to it or waking it up, depending on whether
- *	or not it has its own mm (ie. it is a user land task).  If @with_mm_only
- *	is set and the task has no mm of its own (ie. it is a kernel thread),
- *	its TIF_FREEZE flag should not be set.
- *
- *	The task_lock() is necessary to prevent races with exit_mm() or
- *	use_mm()/unuse_mm() from occuring.
+ *	or not it has PF_FREEZER_NOSIG set.  If @sig_only is set and the task
+ *	has PF_FREEZER_NOSIG set (ie. it is a typical kernel thread), its
+ *	TIF_FREEZE flag will not be set.
  */
-static int freeze_task(struct task_struct *p, int with_mm_only)
+static bool freeze_task(struct task_struct *p, bool sig_only)
 {
-	int ret = 1;
+	/*
+	 * We first check if the task is freezing and next if it has already
+	 * been frozen to avoid the race with frozen_process() which first marks
+	 * the task as frozen and next clears its TIF_FREEZE.
+	 */
+	if (!freezing(p)) {
+		rmb();
+		if (frozen(p))
+			return false;
 
-	task_lock(p);
-	if (freezing(p)) {
-		if (has_mm(p)) {
-			if (!signal_pending(p))
-				fake_signal_wake_up(p);
-		} else {
-			if (with_mm_only)
-				ret = 0;
-			else
-				wake_up_state(p, TASK_INTERRUPTIBLE);
-		}
+		if (!sig_only || should_send_signal(p))
+			set_freeze_flag(p);
+		else
+			return false;
+	}
+
+	if (should_send_signal(p)) {
+		if (!signal_pending(p))
+			fake_signal_wake_up(p);
+	} else if (sig_only) {
+		return false;
 	} else {
-		rmb();
-		if (frozen(p)) {
-			ret = 0;
-		} else {
-			if (has_mm(p)) {
-				set_freeze_flag(p);
-				fake_signal_wake_up(p);
-			} else {
-				if (with_mm_only) {
-					ret = 0;
-				} else {
-					set_freeze_flag(p);
-					wake_up_state(p, TASK_INTERRUPTIBLE);
-				}
-			}
-		}
+		wake_up_state(p, TASK_INTERRUPTIBLE);
 	}
-	task_unlock(p);
-	return ret;
+
+	return true;
 }
 
 static void cancel_freezing(struct task_struct *p)
@@ -156,7 +143,7 @@ static void cancel_freezing(struct task_
 	}
 }
 
-static int try_to_freeze_tasks(int freeze_user_space)
+static int try_to_freeze_tasks(bool sig_only)
 {
 	struct task_struct *g, *p;
 	unsigned long end_time;
@@ -175,7 +162,7 @@ static int try_to_freeze_tasks(int freez
 			if (frozen(p) || !freezeable(p))
 				continue;
 
-			if (!freeze_task(p, freeze_user_space))
+			if (!freeze_task(p, sig_only))
 				continue;
 
 			/*
@@ -235,13 +222,13 @@ int freeze_processes(void)
 	int error;
 
 	printk("Freezing user space processes ... ");
-	error = try_to_freeze_tasks(FREEZER_USER_SPACE);
+	error = try_to_freeze_tasks(true);
 	if (error)
 		goto Exit;
 	printk("done.\n");
 
 	printk("Freezing remaining freezable tasks ... ");
-	error = try_to_freeze_tasks(FREEZER_KERNEL_THREADS);
+	error = try_to_freeze_tasks(false);
 	if (error)
 		goto Exit;
 	printk("done.");
@@ -251,7 +238,7 @@ int freeze_processes(void)
 	return error;
 }
 
-static void thaw_tasks(int thaw_user_space)
+static void thaw_tasks(bool sig_only)
 {
 	struct task_struct *g, *p;
 
@@ -260,7 +247,7 @@ static void thaw_tasks(int thaw_user_spa
 		if (!freezeable(p))
 			continue;
 
-		if (!p->mm == thaw_user_space)
+		if (sig_only && !should_send_signal(p))
 			continue;
 
 		thaw_process(p);
@@ -271,8 +258,8 @@ static void thaw_tasks(int thaw_user_spa
 void thaw_processes(void)
 {
 	printk("Restarting tasks ... ");
-	thaw_tasks(FREEZER_KERNEL_THREADS);
-	thaw_tasks(FREEZER_USER_SPACE);
+	thaw_tasks(true);
+	thaw_tasks(false);
 	schedule();
 	printk("done.\n");
 }
Index: linux-2.6/kernel/kthread.c
===================================================================
--- linux-2.6.orig/kernel/kthread.c
+++ linux-2.6/kernel/kthread.c
@@ -234,7 +234,7 @@ int kthreadd(void *unused)
 	set_user_nice(tsk, KTHREAD_NICE_LEVEL);
 	set_cpus_allowed(tsk, CPU_MASK_ALL);
 
-	current->flags |= PF_NOFREEZE;
+	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
 
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);

Re: [linux-pm] [RFC][PATCH 1/2] Freezer: Introduce PF_FREEZER_NOSIG

[Gautham R Shenoy]

Hi Rafael,

On Wed, May 07, 2008 at 12:05:19AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The freezer currently attempts to distinguish kernel threads from
> user space tasks by checking if their mm pointer is unset and it
> does not send fake signals to kernel threads.  However, there are
> kernel threads, mostly related to networking, that behave like
> user space tasks and may want to be sent a fake signal to be frozen.
> 
> Introduce the new process flag PF_FREEZER_NOSIG that will be set
> by default for all kernel threads and make the freezer only send
> fake signals to the tasks having PF_FREEZER_NOSIG unset.  Provide
> the set_freezable_with_signal() function to be called by the kernel
> threads that want to be sent a fake signal for freezing.
> 
> This patch should not change the freezer's observable behavior.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/freezer.h |   10 ++++
>  include/linux/sched.h   |    1 
>  kernel/kthread.c        |    2 
>  kernel/power/process.c  |   97 ++++++++++++++++++++----------------------------
>  4 files changed, 54 insertions(+), 56 deletions(-)
> 
> Index: linux-2.6/include/linux/freezer.h
> ===================================================================
> --- linux-2.6.orig/include/linux/freezer.h
> +++ linux-2.6/include/linux/freezer.h
> @@ -128,6 +128,15 @@ static inline void set_freezable(void)
>  }
> 
>  /*
> + * Tell the freezer that the current task should be frozen by it and that it
> + * should send a fake signal to the task to freeze it.
> + */
> +static inline void set_freezable_with_signal(void)
> +{
> +	current->flags &= ~(PF_NOFREEZE | PF_FREEZER_NOSIG);
> +}
> +
> +/*
>   * Freezer-friendly wrappers around wait_event_interruptible() and
>   * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
>   */
> @@ -174,6 +183,7 @@ static inline void freezer_do_not_count(
>  static inline void freezer_count(void) {}
>  static inline int freezer_should_skip(struct task_struct *p) { return 0; }
>  static inline void set_freezable(void) {}
> +static inline void set_freezable_with_signal(void) {}
> 
>  #define wait_event_freezable(wq, condition)				\
>  		wait_event_interruptible(wq, condition)
> Index: linux-2.6/include/linux/sched.h
> ===================================================================
> --- linux-2.6.orig/include/linux/sched.h
> +++ linux-2.6/include/linux/sched.h
> @@ -1508,6 +1508,7 @@ static inline void put_task_struct(struc
>  #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 */
> +#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
> 
>  /*
>   * Only the _current_ task can read/write to tsk->flags, but other
> Index: linux-2.6/kernel/power/process.c
> ===================================================================
> --- linux-2.6.orig/kernel/power/process.c
> +++ linux-2.6/kernel/power/process.c
> @@ -19,9 +19,6 @@
>   */
>  #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) ||
> @@ -84,63 +81,53 @@ static void fake_signal_wake_up(struct t
>  	spin_unlock_irqrestore(&p->sighand->siglock, flags);
>  }
> 
> -static int has_mm(struct task_struct *p)
> +static inline bool should_send_signal(struct task_struct *p)
>  {
> -	return (p->mm && !(p->flags & PF_BORROWED_MM));
> +	return !(current->flags & PF_FREEZER_NOSIG);
		^^^^^^^^^^^^^^
		p->flags ?


>  }
> 
>  /**
>   *	freeze_task - send a freeze request to given task
>   *	@p: task to send the request to
> - *	@with_mm_only: if set, the request will only be sent if the task has its
> - *		own mm
> - *	Return value: 0, if @with_mm_only is set and the task has no mm of its
> - *		own or the task is frozen, 1, otherwise
> + *	@sig_only: if set, the request will only be sent if the task has the
> + *		PF_FREEZER_NOSIG flag unset
> + *	Return value: 'false', if @sig_only is set and the task has
> + *		PF_FREEZER_NOSIG set or the task is frozen, 'true', otherwise
>   *
> - *	The freeze request is sent by seting the tasks's TIF_FREEZE flag and
> + *	The freeze request is sent by setting the tasks's TIF_FREEZE flag and
>   *	either sending a fake signal to it or waking it up, depending on whether
> - *	or not it has its own mm (ie. it is a user land task).  If @with_mm_only
> - *	is set and the task has no mm of its own (ie. it is a kernel thread),
> - *	its TIF_FREEZE flag should not be set.
> - *
> - *	The task_lock() is necessary to prevent races with exit_mm() or
> - *	use_mm()/unuse_mm() from occuring.
> + *	or not it has PF_FREEZER_NOSIG set.  If @sig_only is set and the task
> + *	has PF_FREEZER_NOSIG set (ie. it is a typical kernel thread), its
> + *	TIF_FREEZE flag will not be set.
>   */
> -static int freeze_task(struct task_struct *p, int with_mm_only)
> +static bool freeze_task(struct task_struct *p, bool sig_only)
>  {
> -	int ret = 1;
> +	/*
> +	 * We first check if the task is freezing and next if it has already
> +	 * been frozen to avoid the race with frozen_process() which first marks
> +	 * the task as frozen and next clears its TIF_FREEZE.
> +	 */
> +	if (!freezing(p)) {
> +		rmb();
> +		if (frozen(p))
> +			return false;
> 
> -	task_lock(p);
> -	if (freezing(p)) {
> -		if (has_mm(p)) {
> -			if (!signal_pending(p))
> -				fake_signal_wake_up(p);
> -		} else {
> -			if (with_mm_only)
> -				ret = 0;
> -			else
> -				wake_up_state(p, TASK_INTERRUPTIBLE);
> -		}
> +		if (!sig_only || should_send_signal(p))
> +			set_freeze_flag(p);
> +		else
> +			return false;
> +	}
> +
> +	if (should_send_signal(p)) {
> +		if (!signal_pending(p))
> +			fake_signal_wake_up(p);
> +	} else if (sig_only) {
> +		return false;
>  	} else {
> -		rmb();
> -		if (frozen(p)) {
> -			ret = 0;
> -		} else {
> -			if (has_mm(p)) {
> -				set_freeze_flag(p);
> -				fake_signal_wake_up(p);
> -			} else {
> -				if (with_mm_only) {
> -					ret = 0;
> -				} else {
> -					set_freeze_flag(p);
> -					wake_up_state(p, TASK_INTERRUPTIBLE);
> -				}
> -			}
> -		}
> +		wake_up_state(p, TASK_INTERRUPTIBLE);
>  	}
> -	task_unlock(p);
> -	return ret;
> +
> +	return true;
>  }
> 
>  static void cancel_freezing(struct task_struct *p)
> @@ -156,7 +143,7 @@ static void cancel_freezing(struct task_
>  	}
>  }
> 
> -static int try_to_freeze_tasks(int freeze_user_space)
> +static int try_to_freeze_tasks(bool sig_only)
>  {
>  	struct task_struct *g, *p;
>  	unsigned long end_time;
> @@ -175,7 +162,7 @@ static int try_to_freeze_tasks(int freez
>  			if (frozen(p) || !freezeable(p))
>  				continue;
> 
> -			if (!freeze_task(p, freeze_user_space))
> +			if (!freeze_task(p, sig_only))
>  				continue;
> 
>  			/*
> @@ -235,13 +222,13 @@ int freeze_processes(void)
>  	int error;
> 
>  	printk("Freezing user space processes ... ");
> -	error = try_to_freeze_tasks(FREEZER_USER_SPACE);
> +	error = try_to_freeze_tasks(true);
>  	if (error)
>  		goto Exit;
>  	printk("done.\n");
> 
>  	printk("Freezing remaining freezable tasks ... ");
> -	error = try_to_freeze_tasks(FREEZER_KERNEL_THREADS);
> +	error = try_to_freeze_tasks(false);
>  	if (error)
>  		goto Exit;
>  	printk("done.");
> @@ -251,7 +238,7 @@ int freeze_processes(void)
>  	return error;
>  }
> 
> -static void thaw_tasks(int thaw_user_space)
> +static void thaw_tasks(bool sig_only)
>  {
>  	struct task_struct *g, *p;
> 
> @@ -260,7 +247,7 @@ static void thaw_tasks(int thaw_user_spa
>  		if (!freezeable(p))
>  			continue;
> 
> -		if (!p->mm == thaw_user_space)
> +		if (sig_only && !should_send_signal(p))
>  			continue;
> 
>  		thaw_process(p);
> @@ -271,8 +258,8 @@ static void thaw_tasks(int thaw_user_spa
>  void thaw_processes(void)
>  {
>  	printk("Restarting tasks ... ");
> -	thaw_tasks(FREEZER_KERNEL_THREADS);
> -	thaw_tasks(FREEZER_USER_SPACE);
> +	thaw_tasks(true);
> +	thaw_tasks(false);

Shouldn't the order be 
	thaw_tasks(false);
	thaw_tasks(true);


>  	schedule();
>  	printk("done.\n");
>  }
> Index: linux-2.6/kernel/kthread.c
> ===================================================================
> --- linux-2.6.orig/kernel/kthread.c
> +++ linux-2.6/kernel/kthread.c
> @@ -234,7 +234,7 @@ int kthreadd(void *unused)
>  	set_user_nice(tsk, KTHREAD_NICE_LEVEL);
>  	set_cpus_allowed(tsk, CPU_MASK_ALL);
> 
> -	current->flags |= PF_NOFREEZE;
> +	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
> 
>  	for (;;) {
>  		set_current_state(TASK_INTERRUPTIBLE);
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

-- 
Thanks and Regards
gautham

Re: [linux-pm] [RFC][PATCH 1/2] Freezer: Introduce PF_FREEZER_NOSIG

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Gautham R Shenoy wrote:
> Hi Rafael,

Hi,

> On Wed, May 07, 2008 at 12:05:19AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The freezer currently attempts to distinguish kernel threads from
> > user space tasks by checking if their mm pointer is unset and it
> > does not send fake signals to kernel threads.  However, there are
> > kernel threads, mostly related to networking, that behave like
> > user space tasks and may want to be sent a fake signal to be frozen.
> > 
> > Introduce the new process flag PF_FREEZER_NOSIG that will be set
> > by default for all kernel threads and make the freezer only send
> > fake signals to the tasks having PF_FREEZER_NOSIG unset.  Provide
> > the set_freezable_with_signal() function to be called by the kernel
> > threads that want to be sent a fake signal for freezing.
> > 
> > This patch should not change the freezer's observable behavior.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  include/linux/freezer.h |   10 ++++
> >  include/linux/sched.h   |    1 
> >  kernel/kthread.c        |    2 
> >  kernel/power/process.c  |   97 ++++++++++++++++++++----------------------------
> >  4 files changed, 54 insertions(+), 56 deletions(-)
> > 
> > Index: linux-2.6/include/linux/freezer.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/freezer.h
> > +++ linux-2.6/include/linux/freezer.h
> > @@ -128,6 +128,15 @@ static inline void set_freezable(void)
> >  }
> > 
> >  /*
> > + * Tell the freezer that the current task should be frozen by it and that it
> > + * should send a fake signal to the task to freeze it.
> > + */
> > +static inline void set_freezable_with_signal(void)
> > +{
> > +	current->flags &= ~(PF_NOFREEZE | PF_FREEZER_NOSIG);
> > +}
> > +
> > +/*
> >   * Freezer-friendly wrappers around wait_event_interruptible() and
> >   * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
> >   */
> > @@ -174,6 +183,7 @@ static inline void freezer_do_not_count(
> >  static inline void freezer_count(void) {}
> >  static inline int freezer_should_skip(struct task_struct *p) { return 0; }
> >  static inline void set_freezable(void) {}
> > +static inline void set_freezable_with_signal(void) {}
> > 
> >  #define wait_event_freezable(wq, condition)				\
> >  		wait_event_interruptible(wq, condition)
> > Index: linux-2.6/include/linux/sched.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/sched.h
> > +++ linux-2.6/include/linux/sched.h
> > @@ -1508,6 +1508,7 @@ static inline void put_task_struct(struc
> >  #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 */
> > +#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
> > 
> >  /*
> >   * Only the _current_ task can read/write to tsk->flags, but other
> > Index: linux-2.6/kernel/power/process.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/power/process.c
> > +++ linux-2.6/kernel/power/process.c
> > @@ -19,9 +19,6 @@
> >   */
> >  #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) ||
> > @@ -84,63 +81,53 @@ static void fake_signal_wake_up(struct t
> >  	spin_unlock_irqrestore(&p->sighand->siglock, flags);
> >  }
> > 
> > -static int has_mm(struct task_struct *p)
> > +static inline bool should_send_signal(struct task_struct *p)
> >  {
> > -	return (p->mm && !(p->flags & PF_BORROWED_MM));
> > +	return !(current->flags & PF_FREEZER_NOSIG);
> 		^^^^^^^^^^^^^^
> 		p->flags ?

Yes, nice catch, thanks!

I wonder how on Earth that wasn't caught by testing (current->flags is always
false here) ...

[--snip--]
> > 
> > -static void thaw_tasks(int thaw_user_space)
> > +static void thaw_tasks(bool sig_only)
> >  {
> >  	struct task_struct *g, *p;
> > 
> > @@ -260,7 +247,7 @@ static void thaw_tasks(int thaw_user_spa
> >  		if (!freezeable(p))
> >  			continue;
> > 
> > -		if (!p->mm == thaw_user_space)
> > +		if (sig_only && !should_send_signal(p))
> >  			continue;
> > 
> >  		thaw_process(p);
> > @@ -271,8 +258,8 @@ static void thaw_tasks(int thaw_user_spa
> >  void thaw_processes(void)
> >  {
> >  	printk("Restarting tasks ... ");
> > -	thaw_tasks(FREEZER_KERNEL_THREADS);
> > -	thaw_tasks(FREEZER_USER_SPACE);
> > +	thaw_tasks(true);
> > +	thaw_tasks(false);
> 
> Shouldn't the order be 
> 	thaw_tasks(false);
> 	thaw_tasks(true);

Ouch, thanks again!

But, that's supposed to be "first thaw tasks we did not send fake signals to
and then thaw everybody else", which translates to something different from
what I wrote in thaw_tasks(), too.

[Well, frankly I'm not sure if there is a practical situation in which that
matters.]

Corrected patch is appended.

Thanks,
Rafael


---
 include/linux/freezer.h |   10 ++++
 include/linux/sched.h   |    1 
 kernel/kthread.c        |    2 
 kernel/power/process.c  |   97 ++++++++++++++++++++----------------------------
 4 files changed, 54 insertions(+), 56 deletions(-)

Index: linux-2.6/include/linux/freezer.h
===================================================================
--- linux-2.6.orig/include/linux/freezer.h
+++ linux-2.6/include/linux/freezer.h
@@ -128,6 +128,15 @@ static inline void set_freezable(void)
 }
 
 /*
+ * Tell the freezer that the current task should be frozen by it and that it
+ * should send a fake signal to the task to freeze it.
+ */
+static inline void set_freezable_with_signal(void)
+{
+	current->flags &= ~(PF_NOFREEZE | PF_FREEZER_NOSIG);
+}
+
+/*
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */
@@ -174,6 +183,7 @@ static inline void freezer_do_not_count(
 static inline void freezer_count(void) {}
 static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
+static inline void set_freezable_with_signal(void) {}
 
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)
Index: linux-2.6/include/linux/sched.h
===================================================================
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -1508,6 +1508,7 @@ static inline void put_task_struct(struc
 #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 */
+#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
Index: linux-2.6/kernel/power/process.c
===================================================================
--- linux-2.6.orig/kernel/power/process.c
+++ linux-2.6/kernel/power/process.c
@@ -19,9 +19,6 @@
  */
 #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) ||
@@ -84,63 +81,53 @@ static void fake_signal_wake_up(struct t
 	spin_unlock_irqrestore(&p->sighand->siglock, flags);
 }
 
-static int has_mm(struct task_struct *p)
+static inline bool should_send_signal(struct task_struct *p)
 {
-	return (p->mm && !(p->flags & PF_BORROWED_MM));
+	return !(p->flags & PF_FREEZER_NOSIG);
 }
 
 /**
  *	freeze_task - send a freeze request to given task
  *	@p: task to send the request to
- *	@with_mm_only: if set, the request will only be sent if the task has its
- *		own mm
- *	Return value: 0, if @with_mm_only is set and the task has no mm of its
- *		own or the task is frozen, 1, otherwise
+ *	@sig_only: if set, the request will only be sent if the task has the
+ *		PF_FREEZER_NOSIG flag unset
+ *	Return value: 'false', if @sig_only is set and the task has
+ *		PF_FREEZER_NOSIG set or the task is frozen, 'true', otherwise
  *
- *	The freeze request is sent by seting the tasks's TIF_FREEZE flag and
+ *	The freeze request is sent by setting the tasks's TIF_FREEZE flag and
  *	either sending a fake signal to it or waking it up, depending on whether
- *	or not it has its own mm (ie. it is a user land task).  If @with_mm_only
- *	is set and the task has no mm of its own (ie. it is a kernel thread),
- *	its TIF_FREEZE flag should not be set.
- *
- *	The task_lock() is necessary to prevent races with exit_mm() or
- *	use_mm()/unuse_mm() from occuring.
+ *	or not it has PF_FREEZER_NOSIG set.  If @sig_only is set and the task
+ *	has PF_FREEZER_NOSIG set (ie. it is a typical kernel thread), its
+ *	TIF_FREEZE flag will not be set.
  */
-static int freeze_task(struct task_struct *p, int with_mm_only)
+static bool freeze_task(struct task_struct *p, bool sig_only)
 {
-	int ret = 1;
+	/*
+	 * We first check if the task is freezing and next if it has already
+	 * been frozen to avoid the race with frozen_process() which first marks
+	 * the task as frozen and next clears its TIF_FREEZE.
+	 */
+	if (!freezing(p)) {
+		rmb();
+		if (frozen(p))
+			return false;
 
-	task_lock(p);
-	if (freezing(p)) {
-		if (has_mm(p)) {
-			if (!signal_pending(p))
-				fake_signal_wake_up(p);
-		} else {
-			if (with_mm_only)
-				ret = 0;
-			else
-				wake_up_state(p, TASK_INTERRUPTIBLE);
-		}
+		if (!sig_only || should_send_signal(p))
+			set_freeze_flag(p);
+		else
+			return false;
+	}
+
+	if (should_send_signal(p)) {
+		if (!signal_pending(p))
+			fake_signal_wake_up(p);
+	} else if (sig_only) {
+		return false;
 	} else {
-		rmb();
-		if (frozen(p)) {
-			ret = 0;
-		} else {
-			if (has_mm(p)) {
-				set_freeze_flag(p);
-				fake_signal_wake_up(p);
-			} else {
-				if (with_mm_only) {
-					ret = 0;
-				} else {
-					set_freeze_flag(p);
-					wake_up_state(p, TASK_INTERRUPTIBLE);
-				}
-			}
-		}
+		wake_up_state(p, TASK_INTERRUPTIBLE);
 	}
-	task_unlock(p);
-	return ret;
+
+	return true;
 }
 
 static void cancel_freezing(struct task_struct *p)
@@ -156,7 +143,7 @@ static void cancel_freezing(struct task_
 	}
 }
 
-static int try_to_freeze_tasks(int freeze_user_space)
+static int try_to_freeze_tasks(bool sig_only)
 {
 	struct task_struct *g, *p;
 	unsigned long end_time;
@@ -175,7 +162,7 @@ static int try_to_freeze_tasks(int freez
 			if (frozen(p) || !freezeable(p))
 				continue;
 
-			if (!freeze_task(p, freeze_user_space))
+			if (!freeze_task(p, sig_only))
 				continue;
 
 			/*
@@ -235,13 +222,13 @@ int freeze_processes(void)
 	int error;
 
 	printk("Freezing user space processes ... ");
-	error = try_to_freeze_tasks(FREEZER_USER_SPACE);
+	error = try_to_freeze_tasks(true);
 	if (error)
 		goto Exit;
 	printk("done.\n");
 
 	printk("Freezing remaining freezable tasks ... ");
-	error = try_to_freeze_tasks(FREEZER_KERNEL_THREADS);
+	error = try_to_freeze_tasks(false);
 	if (error)
 		goto Exit;
 	printk("done.");
@@ -251,7 +238,7 @@ int freeze_processes(void)
 	return error;
 }
 
-static void thaw_tasks(int thaw_user_space)
+static void thaw_tasks(bool nosig_only)
 {
 	struct task_struct *g, *p;
 
@@ -260,7 +247,7 @@ static void thaw_tasks(int thaw_user_spa
 		if (!freezeable(p))
 			continue;
 
-		if (!p->mm == thaw_user_space)
+		if (nosig_only && should_send_signal(p))
 			continue;
 
 		thaw_process(p);
@@ -271,8 +258,8 @@ static void thaw_tasks(int thaw_user_spa
 void thaw_processes(void)
 {
 	printk("Restarting tasks ... ");
-	thaw_tasks(FREEZER_KERNEL_THREADS);
-	thaw_tasks(FREEZER_USER_SPACE);
+	thaw_tasks(true);
+	thaw_tasks(false);
 	schedule();
 	printk("done.\n");
 }
Index: linux-2.6/kernel/kthread.c
===================================================================
--- linux-2.6.orig/kernel/kthread.c
+++ linux-2.6/kernel/kthread.c
@@ -234,7 +234,7 @@ int kthreadd(void *unused)
 	set_user_nice(tsk, KTHREAD_NICE_LEVEL);
 	set_cpus_allowed(tsk, CPU_MASK_ALL);
 
-	current->flags |= PF_NOFREEZE;
+	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
 
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);

Re: [RFC][PATCH 1/2] Freezer: Introduce PF_FREEZER_NOSIG

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The freezer currently attempts to distinguish kernel threads from
> user space tasks by checking if their mm pointer is unset and it
> does not send fake signals to kernel threads.  However, there are
> kernel threads, mostly related to networking, that behave like
> user space tasks and may want to be sent a fake signal to be frozen.
> 
> Introduce the new process flag PF_FREEZER_NOSIG that will be set
> by default for all kernel threads and make the freezer only send
> fake signals to the tasks having PF_FREEZER_NOSIG unset.  Provide
> the set_freezable_with_signal() function to be called by the kernel
> threads that want to be sent a fake signal for freezing.
> 
> This patch should not change the freezer's observable behavior.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.


> -static int has_mm(struct task_struct *p)
> +static inline bool should_send_signal(struct task_struct *p)
>  {
> -	return (p->mm && !(p->flags & PF_BORROWED_MM));
> +	return !(current->flags & PF_FREEZER_NOSIG);
>  }
>

Note that we used to tell kernel threads by ->mm, and now you assume
that anything created by ktrheadd is kernel thread, ->mm or not.

I'm not sure if those can differ (->mm = NULL somewhere? Or ->mm =
something somewhere else?). I guess this should go to -mm for a long
test...

> @@ -234,7 +234,7 @@ int kthreadd(void *unused)
>  	set_user_nice(tsk, KTHREAD_NICE_LEVEL);
>  	set_cpus_allowed(tsk, CPU_MASK_ALL);
>  
> -	current->flags |= PF_NOFREEZE;
> +	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
>  
>  	for (;;) {
>  		set_current_state(TASK_INTERRUPTIBLE);

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

Re: [RFC][PATCH 1/2] Freezer: Introduce PF_FREEZER_NOSIG

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Pavel Machek wrote:
> Hi!

Hi,

> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The freezer currently attempts to distinguish kernel threads from
> > user space tasks by checking if their mm pointer is unset and it
> > does not send fake signals to kernel threads.  However, there are
> > kernel threads, mostly related to networking, that behave like
> > user space tasks and may want to be sent a fake signal to be frozen.
> > 
> > Introduce the new process flag PF_FREEZER_NOSIG that will be set
> > by default for all kernel threads and make the freezer only send
> > fake signals to the tasks having PF_FREEZER_NOSIG unset.  Provide
> > the set_freezable_with_signal() function to be called by the kernel
> > threads that want to be sent a fake signal for freezing.
> > 
> > This patch should not change the freezer's observable behavior.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> ACK.

Thanks!

> > -static int has_mm(struct task_struct *p)
> > +static inline bool should_send_signal(struct task_struct *p)
> >  {
> > -	return (p->mm && !(p->flags & PF_BORROWED_MM));
> > +	return !(current->flags & PF_FREEZER_NOSIG);
> >  }
> >
> 
> Note that we used to tell kernel threads by ->mm, and now you assume
> that anything created by ktrheadd is kernel thread, ->mm or not.
> 
> I'm not sure if those can differ (->mm = NULL somewhere? Or ->mm =
> something somewhere else?).

It's done in analogy with PF_NOFREEZE, ie. everything that has
set PF_NOFREEZE by default also has PF_FREEZER_NOSIG set by default, so I
really don't expect any problems here.

> I guess this should go to -mm for a long test...

Yes, it should.

Thanks,
Rafael

[RFC][PATCH 2/2] Freezer: Try to handle killable tasks

[Rafael J. Wysocki]

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

The introduction of TASK_KILLABLE allows the freezer to work in some situation
that it could not handle before.

Make the freezer handle killable tasks and add try_to_freeze() in some places
where it is safe to freeze a (killable) task.  Introduce the
wait_event_killable_freezable() macro to be used wherever the freezing of
a waiting killable task is desirable.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 fs/nfs/inode.c          |    2 ++
 fs/nfs/nfs3proc.c       |    2 ++
 fs/nfs/nfs4proc.c       |    4 ++++
 fs/nfs/pagelist.c       |    8 ++++++--
 fs/smbfs/request.c      |    2 ++
 include/linux/freezer.h |   20 +++++++++++++++++---
 kernel/mutex.c          |    3 +++
 kernel/power/process.c  |    6 ++++--
 kernel/sched.c          |    2 ++
 net/sunrpc/sched.c      |    2 ++
 10 files changed, 44 insertions(+), 7 deletions(-)

Index: linux-2.6/fs/nfs/nfs3proc.c
===================================================================
--- linux-2.6.orig/fs/nfs/nfs3proc.c
+++ linux-2.6/fs/nfs/nfs3proc.c
@@ -17,6 +17,7 @@
 #include <linux/nfs_page.h>
 #include <linux/lockd/bind.h>
 #include <linux/nfs_mount.h>
+#include <linux/freezer.h>
 
 #include "iostat.h"
 #include "internal.h"
@@ -33,6 +34,7 @@ nfs3_rpc_wrapper(struct rpc_clnt *clnt, 
 		if (res != -EJUKEBOX)
 			break;
 		schedule_timeout_killable(NFS_JUKEBOX_RETRY_TIME);
+		try_to_freeze();
 		res = -ERESTARTSYS;
 	} while (!fatal_signal_pending(current));
 	return res;
Index: linux-2.6/fs/nfs/nfs4proc.c
===================================================================
--- linux-2.6.orig/fs/nfs/nfs4proc.c
+++ linux-2.6/fs/nfs/nfs4proc.c
@@ -48,6 +48,7 @@
 #include <linux/smp_lock.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
+#include <linux/freezer.h>
 
 #include "nfs4_fs.h"
 #include "delegation.h"
@@ -2788,6 +2789,7 @@ static int nfs4_wait_bit_killable(void *
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
 	schedule();
+	try_to_freeze();
 	return 0;
 }
 
@@ -2819,6 +2821,8 @@ static int nfs4_delay(struct rpc_clnt *c
 	schedule_timeout_killable(*timeout);
 	if (fatal_signal_pending(current))
 		res = -ERESTARTSYS;
+	else
+		try_to_freeze();
 	*timeout <<= 1;
 	return res;
 }
Index: linux-2.6/fs/nfs/pagelist.c
===================================================================
--- linux-2.6.orig/fs/nfs/pagelist.c
+++ linux-2.6/fs/nfs/pagelist.c
@@ -18,6 +18,7 @@
 #include <linux/nfs_page.h>
 #include <linux/nfs_fs.h>
 #include <linux/nfs_mount.h>
+#include <linux/freezer.h>
 
 #include "internal.h"
 
@@ -68,6 +69,7 @@ nfs_create_request(struct nfs_open_conte
 
 		if (fatal_signal_pending(current))
 			return ERR_PTR(-ERESTARTSYS);
+		try_to_freeze();
 		yield();
 	}
 
@@ -180,10 +182,12 @@ static int nfs_wait_bit_killable(void *w
 {
 	int ret = 0;
 
-	if (fatal_signal_pending(current))
+	if (fatal_signal_pending(current)) {
 		ret = -ERESTARTSYS;
-	else
+	} else {
 		schedule();
+		try_to_freeze();
+	}
 	return ret;
 }
 
Index: linux-2.6/fs/smbfs/request.c
===================================================================
--- linux-2.6.orig/fs/smbfs/request.c
+++ linux-2.6/fs/smbfs/request.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/net.h>
 #include <linux/sched.h>
+#include <linux/freezer.h>
 
 #include <linux/smb_fs.h>
 #include <linux/smbno.h>
@@ -109,6 +110,7 @@ struct smb_request *smb_alloc_request(st
 			return ERR_PTR(-ERESTARTSYS);
 		current->policy = SCHED_YIELD;
 		schedule();
+		try_to_freeze();
 #else
 		/* FIXME: we want something like nfs does above, but that
 		   requires changes to all callers and can wait. */
Index: linux-2.6/include/linux/freezer.h
===================================================================
--- linux-2.6.orig/include/linux/freezer.h
+++ linux-2.6/include/linux/freezer.h
@@ -137,8 +137,9 @@ static inline void set_freezable_with_si
 }
 
 /*
- * Freezer-friendly wrappers around wait_event_interruptible() and
- * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
+ * Freezer-friendly wrappers around wait_event_interruptible(),
+ * wait_event_interruptible_timeout(), wait_event_killable(),
+ * originally defined in <linux/wait.h>
  */
 
 #define wait_event_freezable(wq, condition)				\
@@ -155,7 +156,6 @@ static inline void set_freezable_with_si
 	__retval;							\
 })
 
-
 #define wait_event_freezable_timeout(wq, condition, timeout)		\
 ({									\
 	long __retval = timeout;					\
@@ -166,6 +166,20 @@ static inline void set_freezable_with_si
 	} while (try_to_freeze());					\
 	__retval;							\
 })
+
+#define wait_event_killable_freezable(wq, condition)				\
+({									\
+	int __retval;							\
+	do {								\
+		__retval = wait_event_killable(wq, 			\
+				(condition) || freezing(current));	\
+		if (__retval && !freezing(current))			\
+			break;						\
+		else if (!(condition))					\
+			__retval = -ERESTARTSYS;			\
+	} while (try_to_freeze());					\
+	__retval;							\
+})
 #else /* !CONFIG_PM_SLEEP */
 static inline int frozen(struct task_struct *p) { return 0; }
 static inline int freezing(struct task_struct *p) { return 0; }
Index: linux-2.6/kernel/mutex.c
===================================================================
--- linux-2.6.orig/kernel/mutex.c
+++ linux-2.6/kernel/mutex.c
@@ -18,6 +18,7 @@
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/debug_locks.h>
+#include <linux/freezer.h>
 
 /*
  * In the DEBUG case we are using the "NULL fastpath" for mutexes,
@@ -182,6 +183,8 @@ __mutex_lock_common(struct mutex *lock, 
 		/* didnt get the lock, go to sleep: */
 		spin_unlock_mutex(&lock->wait_lock, flags);
 		schedule();
+		if (state == TASK_KILLABLE)
+			try_to_freeze();
 		spin_lock_mutex(&lock->wait_lock, flags);
 	}
 
Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c
+++ linux-2.6/kernel/sched.c
@@ -4769,6 +4769,8 @@ do_wait_for_common(struct completion *x,
 			__set_current_state(state);
 			spin_unlock_irq(&x->wait.lock);
 			timeout = schedule_timeout(timeout);
+			if (state == TASK_KILLABLE)
+				try_to_freeze();
 			spin_lock_irq(&x->wait.lock);
 			if (!timeout) {
 				__remove_wait_queue(&x->wait, &wait);
Index: linux-2.6/net/sunrpc/sched.c
===================================================================
--- linux-2.6.orig/net/sunrpc/sched.c
+++ linux-2.6/net/sunrpc/sched.c
@@ -19,6 +19,7 @@
 #include <linux/smp_lock.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 
 #include <linux/sunrpc/clnt.h>
 
@@ -227,6 +228,7 @@ static int rpc_wait_bit_killable(void *w
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
 	schedule();
+	try_to_freeze();
 	return 0;
 }
 
Index: linux-2.6/fs/nfs/inode.c
===================================================================
--- linux-2.6.orig/fs/nfs/inode.c
+++ linux-2.6/fs/nfs/inode.c
@@ -37,6 +37,7 @@
 #include <linux/vfs.h>
 #include <linux/inet.h>
 #include <linux/nfs_xdr.h>
+#include <linux/freezer.h>
 
 #include <asm/system.h>
 #include <asm/uaccess.h>
@@ -426,6 +427,7 @@ static int nfs_wait_schedule(void *word)
 	if (signal_pending(current))
 		return -ERESTARTSYS;
 	schedule();
+	try_to_freeze();
 	return 0;
 }
 
Index: linux-2.6/kernel/power/process.c
===================================================================
--- linux-2.6.orig/kernel/power/process.c
+++ linux-2.6/kernel/power/process.c
@@ -77,7 +77,9 @@ static void fake_signal_wake_up(struct t
 	unsigned long flags;
 
 	spin_lock_irqsave(&p->sighand->siglock, flags);
-	signal_wake_up(p, 0);
+	set_tsk_thread_flag(p, TIF_SIGPENDING);
+	if (!wake_up_state(p, TASK_INTERRUPTIBLE | TASK_KILLABLE))
+		kick_process(p);
 	spin_unlock_irqrestore(&p->sighand->siglock, flags);
 }
 
@@ -124,7 +126,7 @@ static bool freeze_task(struct task_stru
 	} else if (sig_only) {
 		return false;
 	} else {
-		wake_up_state(p, TASK_INTERRUPTIBLE);
+		wake_up_state(p, TASK_INTERRUPTIBLE | TASK_KILLABLE);
 	}
 
 	return true;

Re: [RFC][PATCH 2/2] Freezer: Try to handle killable tasks

[Pavel Machek]

On Wed 2008-05-07 00:07:55, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The introduction of TASK_KILLABLE allows the freezer to work in some situation
> that it could not handle before.
> 
> Make the freezer handle killable tasks and add try_to_freeze() in some places
> where it is safe to freeze a (killable) task.  Introduce the
> wait_event_killable_freezable() macro to be used wherever the freezing of
> a waiting killable task is desirable.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  fs/nfs/inode.c          |    2 ++
>  fs/nfs/nfs3proc.c       |    2 ++
>  fs/nfs/nfs4proc.c       |    4 ++++
>  fs/nfs/pagelist.c       |    8 ++++++--
>  fs/smbfs/request.c      |    2 ++
>  include/linux/freezer.h |   20 +++++++++++++++++---
>  kernel/mutex.c          |    3 +++
>  kernel/power/process.c  |    6 ++++--
>  kernel/sched.c          |    2 ++
>  net/sunrpc/sched.c      |    2 ++
>  10 files changed, 44 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6/fs/nfs/nfs3proc.c
> ===================================================================
> --- linux-2.6.orig/fs/nfs/nfs3proc.c
> +++ linux-2.6/fs/nfs/nfs3proc.c
> @@ -17,6 +17,7 @@
>  #include <linux/nfs_page.h>
>  #include <linux/lockd/bind.h>
>  #include <linux/nfs_mount.h>
> +#include <linux/freezer.h>
>  
>  #include "iostat.h"
>  #include "internal.h"
> @@ -33,6 +34,7 @@ nfs3_rpc_wrapper(struct rpc_clnt *clnt, 
>  		if (res != -EJUKEBOX)
>  			break;
>  		schedule_timeout_killable(NFS_JUKEBOX_RETRY_TIME);
> +		try_to_freeze();
>  		res = -ERESTARTSYS;
>  	} while (!fatal_signal_pending(current));
>  	return res;
> Index: linux-2.6/fs/nfs/nfs4proc.c
> ===================================================================
> --- linux-2.6.orig/fs/nfs/nfs4proc.c
> +++ linux-2.6/fs/nfs/nfs4proc.c
> @@ -48,6 +48,7 @@
>  #include <linux/smp_lock.h>
>  #include <linux/namei.h>
>  #include <linux/mount.h>
> +#include <linux/freezer.h>
>  
>  #include "nfs4_fs.h"
>  #include "delegation.h"
> @@ -2788,6 +2789,7 @@ static int nfs4_wait_bit_killable(void *
>  	if (fatal_signal_pending(current))
>  		return -ERESTARTSYS;
>  	schedule();
> +	try_to_freeze();
>  	return 0;
>  }

I'd say try_to_freeze() belongs close to the other signal check,
i.e. before schedule?

> @@ -180,10 +182,12 @@ static int nfs_wait_bit_killable(void *w
>  {
>  	int ret = 0;
>  
> -	if (fatal_signal_pending(current))
> +	if (fatal_signal_pending(current)) {
>  		ret = -ERESTARTSYS;
> -	else
> +	} else {
>  		schedule();
> +		try_to_freeze();
> +	}

Same here?

> @@ -109,6 +110,7 @@ struct smb_request *smb_alloc_request(st
>  			return ERR_PTR(-ERESTARTSYS);
>  		current->policy = SCHED_YIELD;
>  		schedule();
> +		try_to_freeze();

And here?

> @@ -182,6 +183,8 @@ __mutex_lock_common(struct mutex *lock, 
>  		/* didnt get the lock, go to sleep: */
>  		spin_unlock_mutex(&lock->wait_lock, flags);
>  		schedule();
> +		if (state == TASK_KILLABLE)
> +			try_to_freeze();
>  		spin_lock_mutex(&lock->wait_lock, flags);
>  	}
>  

I'm not comfortable with this one. Can the task be killable, but still
hold some _other_ mutex? (and then release it only if it actually gets
the signal?)


> Index: linux-2.6/kernel/sched.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched.c
> +++ linux-2.6/kernel/sched.c
> @@ -4769,6 +4769,8 @@ do_wait_for_common(struct completion *x,
>  			__set_current_state(state);
>  			spin_unlock_irq(&x->wait.lock);
>  			timeout = schedule_timeout(timeout);
> +			if (state == TASK_KILLABLE)
> +				try_to_freeze();
>  			spin_lock_irq(&x->wait.lock);
>  			if (!timeout) {
>  				__remove_wait_queue(&x->wait, &wait);

Same here. I can't see why we could not be holding mutexes.

> @@ -227,6 +228,7 @@ static int rpc_wait_bit_killable(void *w
>  	if (fatal_signal_pending(current))
>  		return -ERESTARTSYS;
>  	schedule();
> +	try_to_freeze();
>  	return 0;
>  }

Swap?

> @@ -426,6 +427,7 @@ static int nfs_wait_schedule(void *word)
>  	if (signal_pending(current))
>  		return -ERESTARTSYS;
>  	schedule();
> +	try_to_freeze();
>  	return 0;
>  }
>  

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

Re: [RFC][PATCH 2/2] Freezer: Try to handle killable tasks

[Matthew Wilcox]

On Wed, May 07, 2008 at 11:41:50AM +0200, Pavel Machek wrote:
> > @@ -182,6 +183,8 @@ __mutex_lock_common(struct mutex *lock, 
> >  		/* didnt get the lock, go to sleep: */
> >  		spin_unlock_mutex(&lock->wait_lock, flags);
> >  		schedule();
> > +		if (state == TASK_KILLABLE)
> > +			try_to_freeze();
> >  		spin_lock_mutex(&lock->wait_lock, flags);
> >  	}
> >  
> 
> I'm not comfortable with this one. Can the task be killable, but still
> hold some _other_ mutex? (and then release it only if it actually gets
> the signal?)

Yes, that's exactly what's supposed to happen.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC][PATCH 2/2] Freezer: Try to handle killable tasks

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Matthew Wilcox wrote:
> On Wed, May 07, 2008 at 11:41:50AM +0200, Pavel Machek wrote:
> > > @@ -182,6 +183,8 @@ __mutex_lock_common(struct mutex *lock, 
> > >  		/* didnt get the lock, go to sleep: */
> > >  		spin_unlock_mutex(&lock->wait_lock, flags);
> > >  		schedule();
> > > +		if (state == TASK_KILLABLE)
> > > +			try_to_freeze();
> > >  		spin_lock_mutex(&lock->wait_lock, flags);
> > >  	}
> > >  
> > 
> > I'm not comfortable with this one. Can the task be killable, but still
> > hold some _other_ mutex? (and then release it only if it actually gets
> > the signal?)
> 
> Yes, that's exactly what's supposed to happen.

The question, though, is whether there is a driver that will try to lock this
mutex in its .suspend() or .resume() callback.  If there is one, TASK_KILLABLE
won't help the freezer indeed.

Thanks,
Rafael

Re: [RFC][PATCH 2/2] Freezer: Try to handle killable tasks

[Matthew Wilcox]

On Wed, May 07, 2008 at 12:07:55AM +0200, Rafael J. Wysocki wrote:
> The introduction of TASK_KILLABLE allows the freezer to work in some situation
> that it could not handle before.
> 
> Make the freezer handle killable tasks and add try_to_freeze() in some places
> where it is safe to freeze a (killable) task.  Introduce the
> wait_event_killable_freezable() macro to be used wherever the freezing of
> a waiting killable task is desirable.

Why do you say that TASK_KILLABLE allows the freezer to work in some
situations where it couldn't before?  If something's using one of the
killable functions, it means that it knows how to clean up and unwind
gracefully if the task receives a fatal signal.  I don't understand what
connection there is to the freezer.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC][PATCH 2/2] Freezer: Try to handle killable tasks

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Matthew Wilcox wrote:
> On Wed, May 07, 2008 at 12:07:55AM +0200, Rafael J. Wysocki wrote:
> > The introduction of TASK_KILLABLE allows the freezer to work in some situation
> > that it could not handle before.
> > 
> > Make the freezer handle killable tasks and add try_to_freeze() in some places
> > where it is safe to freeze a (killable) task.  Introduce the
> > wait_event_killable_freezable() macro to be used wherever the freezing of
> > a waiting killable task is desirable.
> 
> Why do you say that TASK_KILLABLE allows the freezer to work in some
> situations where it couldn't before?  If something's using one of the
> killable functions, it means that it knows how to clean up and unwind
> gracefully if the task receives a fatal signal.  I don't understand what
> connection there is to the freezer.

The reason why we don't freeze uninterruptible tasks is that we don't know
why they are in that state.  If one of tasks is uninterruptible for a
relatively long time, that may indicate a non-recoverable error making it
dangerous to put the system into a sleep state.  If the task is killable,
though, the situation is recoverable.

Thanks,
Rafael

Re: [RFC][PATCH 2/2] Freezer: Try to handle killable tasks

[Pavel Machek]

On Wed 2008-05-07 20:41:19, Rafael J. Wysocki wrote:
> On Wednesday, 7 of May 2008, Matthew Wilcox wrote:
> > On Wed, May 07, 2008 at 12:07:55AM +0200, Rafael J. Wysocki wrote:
> > > The introduction of TASK_KILLABLE allows the freezer to work in some situation
> > > that it could not handle before.
> > > 
> > > Make the freezer handle killable tasks and add try_to_freeze() in some places
> > > where it is safe to freeze a (killable) task.  Introduce the
> > > wait_event_killable_freezable() macro to be used wherever the freezing of
> > > a waiting killable task is desirable.
> > 
> > Why do you say that TASK_KILLABLE allows the freezer to work in some
> > situations where it couldn't before?  If something's using one of the
> > killable functions, it means that it knows how to clean up and unwind
> > gracefully if the task receives a fatal signal.  I don't understand what
> > connection there is to the freezer.
> 
> The reason why we don't freeze uninterruptible tasks is that we don't know
> why they are in that state.  If one of tasks is uninterruptible for a
> relatively long time, that may indicate a non-recoverable error making it
> dangerous to put the system into a sleep state.  If the task is killable,
> though, the situation is recoverable.

....but the task may still hold some locks, so we can't "just freeze
it".
								Pavel

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