[RFC][PATCH 0/3] More freezer patches

[RFC][PATCH 0/3] More freezer patches

[Rafael J. Wysocki]

Hi,

I have separated the patch supposed to fix the freezer's vfork problem from the
previous series, because it seems to be controversial.  Moreover, I have one
more patch on top of it and a related, also a bit controversial one.

Please have a look and advise.

Greetings,
Rafael


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

[RFC][PATCH 1/3] Freezer: Fix vfork problem

[Rafael J. Wysocki]

The changelog explains it pretty well, I hope, but I have one more comment to
start with.  Namely, in this version of the patch I've added

+	if (is_user_space(current))
+		try_to_freeze();

to freezer_count() instead of just try_to_freeze().  This way kernel threads
can avoid being frozen where they don't want to.

Well, vatsa seems to think that calling try_to_freeze() in freezer_count() is
not a good idea anyway, but I don't see anything wrong in freezing the user
space processes here.

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

Currently try_to_freeze_tasks() has to wait until all of the vforked processes
exit and for this reason every user can make it fail.  To fix this problem
we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
that do not want to be counted as freezable by the freezer and want to have
TIF_FREEZE set nevertheless.  Then, this flag can be set by tasks using
sys_vfork() before they call wait_for_completion() and cleared after they have
woken up.  After clearing it, kernel threads should call try_to_freeze() in an
appropriate place.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |   41 +++++++++++++++++++++++++++++++++++++++--
 include/linux/sched.h   |    1 +
 kernel/fork.c           |    3 +++
 kernel/power/process.c  |   37 ++++++++-----------------------------
 4 files changed, 51 insertions(+), 31 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/sched.h	2007-02-26 08:40:22.000000000 +0100
+++ linux-2.6.20-mm2/include/linux/sched.h	2007-02-26 08:40:56.000000000 +0100
@@ -1189,6 +1189,7 @@ 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.20-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/freezer.h	2007-02-26 08:40:22.000000000 +0100
+++ linux-2.6.20-mm2/include/linux/freezer.h	2007-02-26 08:41:49.000000000 +0100
@@ -75,7 +75,42 @@ static inline int try_to_freeze(void)
 		return 0;
 }
 
-extern void thaw_some_processes(int all);
+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;
+}
+
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+	current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the freezer to count it as freezeable
+ * again
+ */
+static inline void freezer_count(void)
+{
+	current->flags &= ~PF_FREEZER_SKIP;
+	if (is_user_space(current))
+		try_to_freeze();
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+	return !!(p->flags & PF_FREEZER_SKIP);
+}
 
 #else
 static inline int frozen(struct task_struct *p) { return 0; }
@@ -90,5 +125,7 @@ static inline void thaw_processes(void) 
 
 static inline int try_to_freeze(void) { return 0; }
 
-
+static inline void freezer_do_not_count(void) {}
+static inline void freezer_count(void) {}
+static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 #endif
Index: linux-2.6.20-mm2/kernel/fork.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/fork.c	2007-02-26 08:40:22.000000000 +0100
+++ linux-2.6.20-mm2/kernel/fork.c	2007-02-26 08:40:56.000000000 +0100
@@ -50,6 +50,7 @@
 #include <linux/taskstats_kern.h>
 #include <linux/random.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
 		tracehook_report_clone_complete(clone_flags, nr, p);
 
 		if (clone_flags & CLONE_VFORK) {
+			freezer_do_not_count();
 			wait_for_completion(&vfork);
+			freezer_count();
 			tracehook_report_vfork_done(p, nr);
 		}
 	} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c	2007-02-26 08:40:22.000000000 +0100
+++ linux-2.6.20-mm2/kernel/power/process.c	2007-02-26 08:41:18.000000000 +0100
@@ -96,16 +96,6 @@ static void cancel_freezing(struct task_
 	}
 }
 
-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;
@@ -127,22 +117,12 @@ static unsigned int try_to_freeze_tasks(
 				cancel_freezing(p);
 				continue;
 			}
-			if (is_user_space(p)) {
-				if (!freeze_user_space)
-					continue;
-
-				/* Freeze the task unless there is a vfork
-				 * completion pending
-				 */
-				if (!p->vfork_done)
-					freeze_process(p);
-			} else {
-				if (freeze_user_space)
-					continue;
+			if (is_user_space(p) == !freeze_user_space)
+				continue;
 
-				freeze_process(p);
-			}
-			todo++;
+			freeze_process(p);
+			if (!freezer_should_skip(p))
+				todo++;
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 		yield();			/* Yield is okay here */
@@ -168,7 +148,8 @@ static unsigned int try_to_freeze_tasks(
 				continue;
 
 			task_lock(p);
-			if (freezeable(p) && !frozen(p))
+			if (freezeable(p) && !frozen(p) &&
+			    !freezer_should_skip(p))
 				printk(KERN_ERR " %s\n", p->comm);
 
 			cancel_freezing(p);
@@ -217,9 +198,7 @@ static void thaw_tasks(int thaw_user_spa
 		if (is_user_space(p) == !thaw_user_space)
 			continue;
 
-		if (!thaw_process(p))
-			printk(KERN_WARNING " Strange, %s not stopped\n",
-				p->comm );
+		thaw_process(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
 }

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Oleg Nesterov]

On 02/26, Rafael J. Wysocki wrote:
>
> The changelog explains it pretty well, I hope, but I have one more comment to
> start with.  Namely, in this version of the patch I've added
> 
> +	if (is_user_space(current))
> +		try_to_freeze();
> 
> to freezer_count() instead of just try_to_freeze().  This way kernel threads
> can avoid being frozen where they don't want to.

In that case we should also modify ____call_usermodehelper(), otherwise we have
the same "deadlock" if it is frozen. But this is not so easy to do as I thought
before.

Oleg.

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Srivatsa Vaddagiri]

On Mon, Feb 26, 2007 at 03:00:43PM +0300, Oleg Nesterov wrote:
> In that case we should also modify ____call_usermodehelper(), otherwise we have
> the same "deadlock" if it is frozen. But this is not so easy to do as I thought
> before.

Before ____call_usermodehelper can freeze, it should have entered userspace 
right? By that time, its vfork parent should have definitely woken up,
which should avoid the deadlock you point out?

-- 
Regards,
vatsa

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Oleg Nesterov]

On 02/26, Srivatsa Vaddagiri wrote:
>
> On Mon, Feb 26, 2007 at 03:00:43PM +0300, Oleg Nesterov wrote:
> > In that case we should also modify ____call_usermodehelper(), otherwise we have
> > the same "deadlock" if it is frozen. But this is not so easy to do as I thought
> > before.
> 
> Before ____call_usermodehelper can freeze, it should have entered userspace 
> right? By that time, its vfork parent should have definitely woken up,
> which should avoid the deadlock you point out?

Ah, yes, thanks for correcting me.

We are doing flush_old_exec() a way before entering userspace of course.

Oleg.

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Rafael J. Wysocki]

On Monday, 26 February 2007 17:11, Oleg Nesterov wrote:
> On 02/26, Srivatsa Vaddagiri wrote:
> >
> > On Mon, Feb 26, 2007 at 03:00:43PM +0300, Oleg Nesterov wrote:
> > > In that case we should also modify ____call_usermodehelper(), otherwise we have
> > > the same "deadlock" if it is frozen. But this is not so easy to do as I thought
> > > before.
> > 
> > Before ____call_usermodehelper can freeze, it should have entered userspace 
> > right? By that time, its vfork parent should have definitely woken up,
> > which should avoid the deadlock you point out?
> 
> Ah, yes, thanks for correcting me.
> 
> We are doing flush_old_exec() a way before entering userspace of course.

Well, does it mean the patch is acceptable or should I modify it somehow?

Rafael

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Oleg Nesterov]

On 02/26, Rafael J. Wysocki wrote:
>
> On Monday, 26 February 2007 17:11, Oleg Nesterov wrote:
> > On 02/26, Srivatsa Vaddagiri wrote:
> > >
> > > On Mon, Feb 26, 2007 at 03:00:43PM +0300, Oleg Nesterov wrote:
> > > > In that case we should also modify ____call_usermodehelper(), otherwise we have
> > > > the same "deadlock" if it is frozen. But this is not so easy to do as I thought
> > > > before.
> > > 
> > > Before ____call_usermodehelper can freeze, it should have entered userspace 
> > > right? By that time, its vfork parent should have definitely woken up,
> > > which should avoid the deadlock you point out?
> > 
> > Ah, yes, thanks for correcting me.
> > 
> > We are doing flush_old_exec() a way before entering userspace of course.
> 
> Well, does it mean the patch is acceptable or should I modify it somehow?

Oh, don't ask me, I don't have a time to study these patches currently :)

_Perhaps_ we can do something better than add explicit checks in freezer_...count,
but I can't suggest anything. Btw, we don't need task_lock() to test current->mm,
and I believe we don't need to check PF_BORROWED_MM there.

"[PATCH 2/3] Freezer: Take kernel_execve into consideration" looks a bit incomplete
to me... I agree, this is a good change for now. But, assuming that we can spawn
an "arbitrary" user-space process from kernel space, we may freeze some kernel
thread which is needed for that user-space task to proceed and notice a signal.

I am starting to suspect that call_usermodehelper() needs a special attention
from freezer, but again, I can't suggest anything, at least right now.

"[PATCH 3/3] Freezer: Prevent ___call_usermodehelper from missing freezing requests"
looks unneeded to me, we should imho drop flush_signals() instead. At least, please
don't call do_not_freeze() under sighand->siglock. This looks as if we have a subtle
reason for this lock, but we don't ? Oh, wait ...  ____call_usermodehelper() does
recalc_sigpending() after flush_signals()! This means we can't lost a "fake" signal
from freezer, so we don't need this patch. Agreed?

Apart from "[PATCH 3/3]", I have nothing against these patches, they fix real problems.

Oleg.

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Rafael J. Wysocki]

On Monday, 26 February 2007 22:25, Oleg Nesterov wrote:
> On 02/26, Rafael J. Wysocki wrote:
> >
> > On Monday, 26 February 2007 17:11, Oleg Nesterov wrote:
> > > On 02/26, Srivatsa Vaddagiri wrote:
> > > >
> > > > On Mon, Feb 26, 2007 at 03:00:43PM +0300, Oleg Nesterov wrote:
> > > > > In that case we should also modify ____call_usermodehelper(), otherwise we have
> > > > > the same "deadlock" if it is frozen. But this is not so easy to do as I thought
> > > > > before.
> > > > 
> > > > Before ____call_usermodehelper can freeze, it should have entered userspace 
> > > > right? By that time, its vfork parent should have definitely woken up,
> > > > which should avoid the deadlock you point out?
> > > 
> > > Ah, yes, thanks for correcting me.
> > > 
> > > We are doing flush_old_exec() a way before entering userspace of course.
> > 
> > Well, does it mean the patch is acceptable or should I modify it somehow?
> 
> Oh, don't ask me, I don't have a time to study these patches currently :)
> 
> _Perhaps_ we can do something better than add explicit checks in freezer_...count,
> but I can't suggest anything. Btw, we don't need task_lock() to test current->mm,
> and I believe we don't need to check PF_BORROWED_MM there.

OK, I have updated the patch accordingly (appended).

> "[PATCH 2/3] Freezer: Take kernel_execve into consideration" looks a bit incomplete
> to me... I agree, this is a good change for now. But, assuming that we can spawn
> an "arbitrary" user-space process from kernel space, we may freeze some kernel
> thread which is needed for that user-space task to proceed and notice a signal.

Well, yes, at least theoretically.

> I am starting to suspect that call_usermodehelper() needs a special attention
> from freezer, but again, I can't suggest anything, at least right now.
> 
> "[PATCH 3/3] Freezer: Prevent ___call_usermodehelper from missing freezing requests"
> looks unneeded to me, we should imho drop flush_signals() instead. At least, please
> don't call do_not_freeze() under sighand->siglock. This looks as if we have a subtle
> reason for this lock, but we don't ? Oh, wait ...  ____call_usermodehelper() does
> recalc_sigpending() after flush_signals()! This means we can't lost a "fake" signal
> from freezer, so we don't need this patch. Agreed?

Yup.  I'm dropping this one.

> Apart from "[PATCH 3/3]", I have nothing against these patches, they fix real problems.

Thanks for your comments.

Greetings,
Rafael

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

Currently try_to_freeze_tasks() has to wait until all of the vforked processes
exit and for this reason every user can make it fail.  To fix this problem
we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
that do not want to be counted as freezable by the freezer and want to have
TIF_FREEZE set nevertheless.  Then, this flag can be set by tasks using
sys_vfork() before they call wait_for_completion() and cleared after they have
woken up.  After clearing it, the tasks should call try_to_freeze() as soon as
possible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
 include/linux/freezer.h |   31 +++++++++++++++++++++++++++++--
 include/linux/sched.h   |    1 +
 kernel/fork.c           |    3 +++
 kernel/power/process.c  |   27 ++++++++-------------------
 4 files changed, 41 insertions(+), 21 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/sched.h	2007-02-26 08:40:22.000000000 +0100
+++ linux-2.6.20-mm2/include/linux/sched.h	2007-02-26 08:40:56.000000000 +0100
@@ -1189,6 +1189,7 @@ 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.20-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/freezer.h	2007-02-26 08:40:22.000000000 +0100
+++ linux-2.6.20-mm2/include/linux/freezer.h	2007-02-27 00:51:44.000000000 +0100
@@ -75,7 +75,32 @@ static inline int try_to_freeze(void)
 		return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+	current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Tell the freezer to count this task as freezeable again and if it's a user
+ * space one, try to freeze it
+ */
+static inline void freezer_count(void)
+{
+	current->flags &= ~PF_FREEZER_SKIP;
+	if (current->mm)
+		try_to_freeze();
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+	return !!(p->flags & PF_FREEZER_SKIP);
+}
 
 #else
 static inline int frozen(struct task_struct *p) { return 0; }
@@ -90,5 +115,7 @@ static inline void thaw_processes(void) 
 
 static inline int try_to_freeze(void) { return 0; }
 
-
+static inline void freezer_do_not_count(void) {}
+static inline void freezer_count(void) {}
+static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 #endif
Index: linux-2.6.20-mm2/kernel/fork.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/fork.c	2007-02-26 08:40:22.000000000 +0100
+++ linux-2.6.20-mm2/kernel/fork.c	2007-02-26 08:40:56.000000000 +0100
@@ -50,6 +50,7 @@
 #include <linux/taskstats_kern.h>
 #include <linux/random.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
 		tracehook_report_clone_complete(clone_flags, nr, p);
 
 		if (clone_flags & CLONE_VFORK) {
+			freezer_do_not_count();
 			wait_for_completion(&vfork);
+			freezer_count();
 			tracehook_report_vfork_done(p, nr);
 		}
 	} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c	2007-02-26 08:40:22.000000000 +0100
+++ linux-2.6.20-mm2/kernel/power/process.c	2007-02-27 00:47:49.000000000 +0100
@@ -127,22 +127,12 @@ static unsigned int try_to_freeze_tasks(
 				cancel_freezing(p);
 				continue;
 			}
-			if (is_user_space(p)) {
-				if (!freeze_user_space)
-					continue;
-
-				/* Freeze the task unless there is a vfork
-				 * completion pending
-				 */
-				if (!p->vfork_done)
-					freeze_process(p);
-			} else {
-				if (freeze_user_space)
-					continue;
+			if (is_user_space(p) == !freeze_user_space)
+				continue;
 
-				freeze_process(p);
-			}
-			todo++;
+			freeze_process(p);
+			if (!freezer_should_skip(p))
+				todo++;
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 		yield();			/* Yield is okay here */
@@ -168,7 +158,8 @@ static unsigned int try_to_freeze_tasks(
 				continue;
 
 			task_lock(p);
-			if (freezeable(p) && !frozen(p))
+			if (freezeable(p) && !frozen(p) &&
+			    !freezer_should_skip(p))
 				printk(KERN_ERR " %s\n", p->comm);
 
 			cancel_freezing(p);
@@ -217,9 +208,7 @@ static void thaw_tasks(int thaw_user_spa
 		if (is_user_space(p) == !thaw_user_space)
 			continue;
 
-		if (!thaw_process(p))
-			printk(KERN_WARNING " Strange, %s not stopped\n",
-				p->comm );
+		thaw_process(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
 }

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Aneesh Kumar]

>   * Only the _current_ task can read/write to tsk->flags, but other
> Index: linux-2.6.20-mm2/include/linux/freezer.h
> ===================================================================
> --- linux-2.6.20-mm2.orig/include/linux/freezer.h       2007-02-26 08:40:22.000000000 +0100
> +++ linux-2.6.20-mm2/include/linux/freezer.h    2007-02-27 00:51:44.000000000 +0100
> @@ -75,7 +75,32 @@ static inline int try_to_freeze(void)
>                 return 0;
>  }
>
> -extern void thaw_some_processes(int all);
> +/*
> + * Tell the freezer not to count current task as freezeable
> + */
> +static inline void freezer_do_not_count(void)
> +{
> +       current->flags |= PF_FREEZER_SKIP;
> +}
> +
> +/*
> + * Tell the freezer to count this task as freezeable again and if it's a user
> + * space one, try to freeze it
> + */
> +static inline void freezer_count(void)
> +{
> +       current->flags &= ~PF_FREEZER_SKIP;
> +       if (current->mm)
> +               try_to_freeze();
> +}
> +


This means that we are not going to wait for the kernel thread
(Parent) to freeze. I guess what vatsa suggested in previous mail is
better.

freeezer_do_not_count(void)
{
   if (current->mm) {

       current->flags |= PF_FREEZER_SKIP;
   }

}


freezer_count(void)
{
 if (current->mm) {
current->flags &= ~PF_FREEZER_SKIP;
try_to_freeze();
   }

}


Now if do_fork is called from kernel_thread with CLONE_VFORK we make
the freezer wait till that kernel thread is also frozen. I think this
is important.  with khelper thread it is okey because it is a single
threaded workqueue.

If do_fork is called via a user process we can skip the Parent if the
child got frozen before calling execve.

-aneesh

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Srivatsa Vaddagiri]

On Tue, Feb 27, 2007 at 10:03:53AM +0530, Aneesh Kumar wrote:
> This means that we are not going to wait for the kernel thread
> (Parent) to freeze. I guess what vatsa suggested in previous mail is
> better.
> 
> freeezer_do_not_count(void)
> {
>   if (current->mm) {
> 
>       current->flags |= PF_FREEZER_SKIP;
>   }
> 
> }
> 
> 
> freezer_count(void)
> {
> if (current->mm) {
> current->flags &= ~PF_FREEZER_SKIP;
> try_to_freeze();
>   }
> 
> }
> 
> 
> Now if do_fork is called from kernel_thread with CLONE_VFORK we make
> the freezer wait till that kernel thread is also frozen. I think this
> is important.  with khelper thread it is okey because it is a single
> threaded workqueue.

Yes, this assumes that vfork called by a kernel thread will usually wait
only until the vfork child exec's (and not until the child exit's).
That wait should be very short and hence we can wait for vfork parent as
well.

The advanatge of this is we let kernel threads get to freeze at a place of 
their choice (rather than in some common code like do_fork).

> If do_fork is called via a user process we can skip the Parent if the
> child got frozen before calling execve.

Yes, agreed.

-- 
Regards,
vatsa

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Oleg Nesterov]

On 02/27, Aneesh Kumar wrote:
>
> >+static inline void freezer_do_not_count(void)
> >+{
> >+       current->flags |= PF_FREEZER_SKIP;
> >+}
> >+
> >+/*
> >+ * Tell the freezer to count this task as freezeable again and if it's a 
> >user
> >+ * space one, try to freeze it
> >+ */
> >+static inline void freezer_count(void)
> >+{
> >+       current->flags &= ~PF_FREEZER_SKIP;
> >+       if (current->mm)
> >+               try_to_freeze();
> >+}
> >+
> 
> 
> This means that we are not going to wait for the kernel thread
> (Parent) to freeze. I guess what vatsa suggested in previous mail is
> better.

Not only better, but correct. This is very simple: if we set PF_FREEZER_SKIP,
we must call try_to_freeze(), otherwise we confuse the freezer.

Oleg.

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Rafael J. Wysocki]

On Tuesday, 27 February 2007 09:37, Oleg Nesterov wrote:
> On 02/27, Aneesh Kumar wrote:
> >
> > >+static inline void freezer_do_not_count(void)
> > >+{
> > >+       current->flags |= PF_FREEZER_SKIP;
> > >+}
> > >+
> > >+/*
> > >+ * Tell the freezer to count this task as freezeable again and if it's a 
> > >user
> > >+ * space one, try to freeze it
> > >+ */
> > >+static inline void freezer_count(void)
> > >+{
> > >+       current->flags &= ~PF_FREEZER_SKIP;
> > >+       if (current->mm)
> > >+               try_to_freeze();
> > >+}
> > >+
> > 
> > 
> > This means that we are not going to wait for the kernel thread
> > (Parent) to freeze. I guess what vatsa suggested in previous mail is
> > better.
> 
> Not only better, but correct. This is very simple: if we set PF_FREEZER_SKIP,
> we must call try_to_freeze(), otherwise we confuse the freezer.

Okay, patch updated, appended.

Rafael

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

Currently try_to_freeze_tasks() has to wait until all of the vforked processes
exit and for this reason every user can make it fail.  To fix this problem
we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
that do not want to be counted as freezable by the freezer and want to have
TIF_FREEZE set nevertheless.  Then, this flag can be set by tasks using
sys_vfork() before they call wait_for_completion() and cleared after they have
woken up.  After clearing it, the tasks should call try_to_freeze() as soon as
possible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |   34 ++++++++++++++++++++++++++++++++--
 include/linux/sched.h   |    1 +
 kernel/fork.c           |    3 +++
 kernel/power/process.c  |   27 ++++++++-------------------
 4 files changed, 44 insertions(+), 21 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ 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.20-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -75,7 +75,35 @@ static inline int try_to_freeze(void)
 		return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * If the current task is a user space one, tell the freezer not to count it as
+ * freezable.
+ */
+static inline void freezer_do_not_count(void)
+{
+	if (current->mm)
+		current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * If the current task is a user space one, tell the freezer to count it as
+ * freezable again and try to freeze it.
+ */
+static inline void freezer_count(void)
+{
+	if (current->mm) {
+		current->flags &= ~PF_FREEZER_SKIP;
+		try_to_freeze();
+	}
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+	return !!(p->flags & PF_FREEZER_SKIP);
+}
 
 #else
 static inline int frozen(struct task_struct *p) { return 0; }
@@ -90,5 +118,7 @@ static inline void thaw_processes(void) 
 
 static inline int try_to_freeze(void) { return 0; }
 
-
+static inline void freezer_do_not_count(void) {}
+static inline void freezer_count(void) {}
+static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 #endif
Index: linux-2.6.20-mm2/kernel/fork.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/fork.c
+++ linux-2.6.20-mm2/kernel/fork.c
@@ -50,6 +50,7 @@
 #include <linux/taskstats_kern.h>
 #include <linux/random.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
 		tracehook_report_clone_complete(clone_flags, nr, p);
 
 		if (clone_flags & CLONE_VFORK) {
+			freezer_do_not_count();
 			wait_for_completion(&vfork);
+			freezer_count();
 			tracehook_report_vfork_done(p, nr);
 		}
 	} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -127,22 +127,12 @@ static unsigned int try_to_freeze_tasks(
 				cancel_freezing(p);
 				continue;
 			}
-			if (is_user_space(p)) {
-				if (!freeze_user_space)
-					continue;
-
-				/* Freeze the task unless there is a vfork
-				 * completion pending
-				 */
-				if (!p->vfork_done)
-					freeze_process(p);
-			} else {
-				if (freeze_user_space)
-					continue;
+			if (is_user_space(p) == !freeze_user_space)
+				continue;
 
-				freeze_process(p);
-			}
-			todo++;
+			freeze_process(p);
+			if (!freezer_should_skip(p))
+				todo++;
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 		yield();			/* Yield is okay here */
@@ -168,7 +158,8 @@ static unsigned int try_to_freeze_tasks(
 				continue;
 
 			task_lock(p);
-			if (freezeable(p) && !frozen(p))
+			if (freezeable(p) && !frozen(p) &&
+			    !freezer_should_skip(p))
 				printk(KERN_ERR " %s\n", p->comm);
 
 			cancel_freezing(p);
@@ -217,9 +208,7 @@ static void thaw_tasks(int thaw_user_spa
 		if (is_user_space(p) == !thaw_user_space)
 			continue;
 
-		if (!thaw_process(p))
-			printk(KERN_WARNING " Strange, %s not stopped\n",
-				p->comm );
+		thaw_process(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
 }

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Oleg Nesterov]

On 02/27, Rafael J. Wysocki wrote:
>
> Okay, patch updated, appended.

I think it is good. Srivatsa?

Oleg.

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Srivatsa Vaddagiri]

On Wed, Feb 28, 2007 at 12:53:14AM +0300, Oleg Nesterov wrote:
> I think it is good. Srivatsa?

Maybe additional comments on why we don't skip vfork kernel tasks may be good. 
Otherwise looks ok to me. Thanks Rafael for making the changes!

-- 
Regards,
vatsa

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Rafael J. Wysocki]

On Wednesday, 28 February 2007 02:23, Srivatsa Vaddagiri wrote:
> On Wed, Feb 28, 2007 at 12:53:14AM +0300, Oleg Nesterov wrote:
> > I think it is good. Srivatsa?
> 
> Maybe additional comments on why we don't skip vfork kernel tasks may be good. 

Which is because we don't want the kernel threads to be frozen in unexpected
places, so we allow them to block freeze_processes() instead or to set
PF_NOFREEZE?

> Otherwise looks ok to me. Thanks Rafael for making the changes!

You're welcome. :-)

Greetings,
Rafael

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Oleg Nesterov]

On 02/28, Rafael J. Wysocki wrote:
>
> On Wednesday, 28 February 2007 02:23, Srivatsa Vaddagiri wrote:
> > On Wed, Feb 28, 2007 at 12:53:14AM +0300, Oleg Nesterov wrote:
> > > I think it is good. Srivatsa?
> > 
> > Maybe additional comments on why we don't skip vfork kernel tasks may be good. 
> 
> Which is because we don't want the kernel threads to be frozen in unexpected
> places, so we allow them to block freeze_processes() instead or to set
> PF_NOFREEZE?

... and because in fact it won't block freeze_processes(), ____call_usermodehelper
(the child) does a minimum before exec/exit, and it can't be frozen until it wakes
up the parent.

Oleg.

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Rafael J. Wysocki]

On Wednesday, 28 February 2007 12:00, Oleg Nesterov wrote:
> On 02/28, Rafael J. Wysocki wrote:
> >
> > On Wednesday, 28 February 2007 02:23, Srivatsa Vaddagiri wrote:
> > > On Wed, Feb 28, 2007 at 12:53:14AM +0300, Oleg Nesterov wrote:
> > > > I think it is good. Srivatsa?
> > > 
> > > Maybe additional comments on why we don't skip vfork kernel tasks may be good. 
> > 
> > Which is because we don't want the kernel threads to be frozen in unexpected
> > places, so we allow them to block freeze_processes() instead or to set
> > PF_NOFREEZE?
> 
> ... and because in fact it won't block freeze_processes(), ____call_usermodehelper
> (the child) does a minimum before exec/exit, and it can't be frozen until it wakes
> up the parent.

Okay, I have added a comment to freezer.h.  Please have a look.

Rafael

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

Currently try_to_freeze_tasks() has to wait until all of the vforked processes
exit and for this reason every user can make it fail.  To fix this problem
we can introduce the additional process flag PF_FREEZER_SKIP to be used by tasks
that do not want to be counted as freezable by the freezer and want to have
TIF_FREEZE set nevertheless.  Then, this flag can be set by tasks using
sys_vfork() before they call wait_for_completion() and cleared after they have
woken up.  After clearing it, the tasks should call try_to_freeze() as soon as
possible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
 include/linux/freezer.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/sched.h   |    1 +
 kernel/fork.c           |    3 +++
 kernel/power/process.c  |   27 ++++++++-------------------
 4 files changed, 58 insertions(+), 21 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ 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.20-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -75,7 +75,49 @@ static inline int try_to_freeze(void)
 		return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * The PF_FREEZER_SKIP flag should be set by a vfork parent right before it
+ * calls wait_for_completion(&vfork) and reset right after it returns from this
+ * function.  Next, the parent should call try_to_freeze() to freeze itself
+ * appropriately in case the child has exited before the freezing of tasks is
+ * complete.  However, we don't want kernel threads to be frozen in unexpected
+ * places, so we allow them to block freeze_processes() instead or to set
+ * PF_NOFREEZE if needed and PF_FREEZER_SKIP is only set for userland vfork
+ * parents.  Fortunately, in the ____call_usermodehelper() case the parent won't
+ * really block freeze_processes(), since ____call_usermodehelper() (the child)
+ * does a little before exec/exit and it can't be frozen before waking up the
+ * parent.
+ */
+
+/*
+ * If the current task is a user space one, tell the freezer not to count it as
+ * freezable.
+ */
+static inline void freezer_do_not_count(void)
+{
+	if (current->mm)
+		current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * If the current task is a user space one, tell the freezer to count it as
+ * freezable again and try to freeze it.
+ */
+static inline void freezer_count(void)
+{
+	if (current->mm) {
+		current->flags &= ~PF_FREEZER_SKIP;
+		try_to_freeze();
+	}
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+	return !!(p->flags & PF_FREEZER_SKIP);
+}
 
 #else
 static inline int frozen(struct task_struct *p) { return 0; }
@@ -90,5 +132,7 @@ static inline void thaw_processes(void) 
 
 static inline int try_to_freeze(void) { return 0; }
 
-
+static inline void freezer_do_not_count(void) {}
+static inline void freezer_count(void) {}
+static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 #endif
Index: linux-2.6.20-mm2/kernel/fork.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/fork.c
+++ linux-2.6.20-mm2/kernel/fork.c
@@ -50,6 +50,7 @@
 #include <linux/taskstats_kern.h>
 #include <linux/random.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
 		tracehook_report_clone_complete(clone_flags, nr, p);
 
 		if (clone_flags & CLONE_VFORK) {
+			freezer_do_not_count();
 			wait_for_completion(&vfork);
+			freezer_count();
 			tracehook_report_vfork_done(p, nr);
 		}
 	} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -127,22 +127,12 @@ static unsigned int try_to_freeze_tasks(
 				cancel_freezing(p);
 				continue;
 			}
-			if (is_user_space(p)) {
-				if (!freeze_user_space)
-					continue;
-
-				/* Freeze the task unless there is a vfork
-				 * completion pending
-				 */
-				if (!p->vfork_done)
-					freeze_process(p);
-			} else {
-				if (freeze_user_space)
-					continue;
+			if (is_user_space(p) == !freeze_user_space)
+				continue;
 
-				freeze_process(p);
-			}
-			todo++;
+			freeze_process(p);
+			if (!freezer_should_skip(p))
+				todo++;
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 		yield();			/* Yield is okay here */
@@ -168,7 +158,8 @@ static unsigned int try_to_freeze_tasks(
 				continue;
 
 			task_lock(p);
-			if (freezeable(p) && !frozen(p))
+			if (freezeable(p) && !frozen(p) &&
+			    !freezer_should_skip(p))
 				printk(KERN_ERR " %s\n", p->comm);
 
 			cancel_freezing(p);
@@ -217,9 +208,7 @@ static void thaw_tasks(int thaw_user_spa
 		if (is_user_space(p) == !thaw_user_space)
 			continue;
 
-		if (!thaw_process(p))
-			printk(KERN_WARNING " Strange, %s not stopped\n",
-				p->comm );
+		thaw_process(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
 }

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Oleg Nesterov]

On 02/28, Rafael J. Wysocki wrote:
>
> Okay, I have added a comment to freezer.h.  Please have a look.
>
>
> -extern void thaw_some_processes(int all);
> +/*
> + * The PF_FREEZER_SKIP flag should be set by a vfork parent right before it
> + * calls wait_for_completion(&vfork) and reset right after it returns from this
> + * function.  Next, the parent should call try_to_freeze() to freeze itself
> + * appropriately in case the child has exited before the freezing of tasks is
> + * complete.  However, we don't want kernel threads to be frozen in unexpected
> + * places, so we allow them to block freeze_processes() instead or to set
> + * PF_NOFREEZE if needed and PF_FREEZER_SKIP is only set for userland vfork
> + * parents.  Fortunately, in the ____call_usermodehelper() case the parent won't
> + * really block freeze_processes(), since ____call_usermodehelper() (the child)
> + * does a little before exec/exit and it can't be frozen before waking up the
> + * parent.
> + */

I think this comment is accurate and understandable, and I am not suggesting
to change it.

However, please note that PF_FREEZER_SKIP can be used not only for vfork().
For example, it seems to me we can also use freezer_...count() to solve the
problem with coredump. We can use the same "wait_for_completion_freezable"
pattern in exit_mm() and in coredump_wait(). (i do not claim this is a best
fix though).

Oleg.

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Rafael J. Wysocki]

On Wednesday, 28 February 2007 21:30, Oleg Nesterov wrote:
> On 02/28, Rafael J. Wysocki wrote:
> >
> > Okay, I have added a comment to freezer.h.  Please have a look.
> >
> >
> > -extern void thaw_some_processes(int all);
> > +/*
> > + * The PF_FREEZER_SKIP flag should be set by a vfork parent right before it
> > + * calls wait_for_completion(&vfork) and reset right after it returns from this
> > + * function.  Next, the parent should call try_to_freeze() to freeze itself
> > + * appropriately in case the child has exited before the freezing of tasks is
> > + * complete.  However, we don't want kernel threads to be frozen in unexpected
> > + * places, so we allow them to block freeze_processes() instead or to set
> > + * PF_NOFREEZE if needed and PF_FREEZER_SKIP is only set for userland vfork
> > + * parents.  Fortunately, in the ____call_usermodehelper() case the parent won't
> > + * really block freeze_processes(), since ____call_usermodehelper() (the child)
> > + * does a little before exec/exit and it can't be frozen before waking up the
> > + * parent.
> > + */
> 
> I think this comment is accurate and understandable, and I am not suggesting
> to change it.
> 
> However, please note that PF_FREEZER_SKIP can be used not only for vfork().

Yes, it can.

> For example, it seems to me we can also use freezer_...count() to solve the
> problem with coredump. We can use the same "wait_for_completion_freezable"
> pattern in exit_mm() and in coredump_wait(). (i do not claim this is a best
> fix though).

You're right, but in that comment I wanted to explain why it was done this way
rather than what else it could be used for.  There may be some uses of it that
we can't even anticipate right now. :-)

Rafael

Re: [RFC][PATCH 1/3] Freezer: Fix vfork problem

[Srivatsa Vaddagiri]

On Wed, Feb 28, 2007 at 11:57:27AM +0100, Rafael J. Wysocki wrote:
> Which is because we don't want the kernel threads to be frozen in unexpected
> places, so we allow them to block freeze_processes() instead or to set
> PF_NOFREEZE?

Looks good!

-- 
Regards,
vatsa

[RFC][PATCH 2/3] Freezer: Take kernel_execve into consideration

[Rafael J. Wysocki]

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

Kernel threads can become userland processes by calling kernel_execve().  In
particular, this may happen right after try_to_freeze_tasks(FREEZER_USER_SPACE)
has returned, so try_to_freeze_tasks() needs to take userspace processes
into consideration even if it is called with FREEZER_KERNEL_THREADS.

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

Index: linux-2.6.20-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/power/process.c	2007-02-26 08:41:18.000000000 +0100
+++ linux-2.6.20-mm2/kernel/power/process.c	2007-02-26 08:41:57.000000000 +0100
@@ -117,7 +117,7 @@ static unsigned int try_to_freeze_tasks(
 				cancel_freezing(p);
 				continue;
 			}
-			if (is_user_space(p) == !freeze_user_space)
+			if (freeze_user_space && !is_user_space(p))
 				continue;
 
 			freeze_process(p);
@@ -144,7 +144,7 @@ static unsigned int try_to_freeze_tasks(
 				TIMEOUT / HZ, todo);
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
-			if (is_user_space(p) == !freeze_user_space)
+			if (freeze_user_space && !is_user_space(p))
 				continue;
 
 			task_lock(p);

Re: [RFC][PATCH 2/3] Freezer: Take kernel_execve into consideration

[Pavel Machek]

On Mon 2007-02-26 11:49:09, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Kernel threads can become userland processes by calling kernel_execve().  In
> particular, this may happen right after try_to_freeze_tasks(FREEZER_USER_SPACE)
> has returned, so try_to_freeze_tasks() needs to take userspace processes
> into consideration even if it is called with FREEZER_KERNEL_THREADS.
> 
> 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

[RFC][PATCH 3/3] Freezer: Prevent ___call_usermodehelper from missing freezing requests

[Rafael J. Wysocki]

NOTE: Alternatively, we can just drop flush_signals() from there, but I'm not
sure that's the right thing to do.

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

Since ____call_usermodehelper() calls flush_signals(current), the task that
enters it may miss a freezing request.  However, if ____call_usermodehelper()
clears TIF_FREEZE for the current task after flush_signals(current) returns, the
freezer will generate one more freezing request for this task.

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

Index: linux-2.6.20-mm2/kernel/kmod.c
===================================================================
--- linux-2.6.20-mm2.orig/kernel/kmod.c	2007-02-25 12:07:15.000000000 +0100
+++ linux-2.6.20-mm2/kernel/kmod.c	2007-02-25 12:49:16.000000000 +0100
@@ -34,6 +34,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/resource.h>
+#include <linux/freezer.h>
 #include <asm/uaccess.h>
 
 extern int delete_module(const char *name, unsigned int flags);
@@ -258,6 +259,7 @@ static int ____call_usermodehelper(void 
 	new_session = key_get(sub_info->ring);
 	flush_signals(current);
 	spin_lock_irq(&current->sighand->siglock);
+	do_not_freeze(current);
 	old_session = __install_session_keyring(current, new_session);
 	flush_signal_handlers(current, 1);
 	sigemptyset(&current->blocked);

Re: [RFC][PATCH 3/3] Freezer: Prevent ___call_usermodehelper from missing freezing requests

[Oleg Nesterov]

On 02/26, Rafael J. Wysocki wrote:
>
> NOTE: Alternatively, we can just drop flush_signals() from there, but I'm not
> sure that's the right thing to do.

We should drop it, I believe. I forgot to send a patch.

Oleg.

> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Since ____call_usermodehelper() calls flush_signals(current), the task that
> enters it may miss a freezing request.  However, if ____call_usermodehelper()
> clears TIF_FREEZE for the current task after flush_signals(current) returns, the
> freezer will generate one more freezing request for this task.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/kmod.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6.20-mm2/kernel/kmod.c
> ===================================================================
> --- linux-2.6.20-mm2.orig/kernel/kmod.c	2007-02-25 12:07:15.000000000 +0100
> +++ linux-2.6.20-mm2/kernel/kmod.c	2007-02-25 12:49:16.000000000 +0100
> @@ -34,6 +34,7 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/resource.h>
> +#include <linux/freezer.h>
>  #include <asm/uaccess.h>
>  
>  extern int delete_module(const char *name, unsigned int flags);
> @@ -258,6 +259,7 @@ static int ____call_usermodehelper(void 
>  	new_session = key_get(sub_info->ring);
>  	flush_signals(current);
>  	spin_lock_irq(&current->sighand->siglock);
> +	do_not_freeze(current);
>  	old_session = __install_session_keyring(current, new_session);
>  	flush_signal_handlers(current, 1);
>  	sigemptyset(&current->blocked);

Re: [RFC][PATCH 3/3] Freezer: Prevent ___call_usermodehelper from missing freezing requests

[Aneesh Kumar]

On 2/26/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> NOTE: Alternatively, we can just drop flush_signals() from there, but I'm not
> sure that's the right thing to do.
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> Since ____call_usermodehelper() calls flush_signals(current), the task that
> enters it may miss a freezing request.  However, if ____call_usermodehelper()
> clears TIF_FREEZE for the current task after flush_signals(current) returns, the
> freezer will generate one more freezing request for this task.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/kmod.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> Index: linux-2.6.20-mm2/kernel/kmod.c
> ===================================================================
> --- linux-2.6.20-mm2.orig/kernel/kmod.c 2007-02-25 12:07:15.000000000 +0100
> +++ linux-2.6.20-mm2/kernel/kmod.c      2007-02-25 12:49:16.000000000 +0100
> @@ -34,6 +34,7 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/resource.h>
> +#include <linux/freezer.h>
>  #include <asm/uaccess.h>
>
>  extern int delete_module(const char *name, unsigned int flags);
> @@ -258,6 +259,7 @@ static int ____call_usermodehelper(void
>         new_session = key_get(sub_info->ring);
>         flush_signals(current);
>         spin_lock_irq(&current->sighand->siglock);
> +       do_not_freeze(current);
>         old_session = __install_session_keyring(current, new_session);
>         flush_signal_handlers(current, 1);
>         sigemptyset(&current->blocked);
>


this need a comment . Otherwise one reading will read it as we don't
want it to freeze.

How about
/*
clears TIF_FREEZE for the current task after flush_signals(current)
returns, so that the
freezer will generate one more freezing request for this task.

*/

-aneesh

Re: [RFC][PATCH 3/3] Freezer: Prevent ___call_usermodehelper from missing freezing requests

[Rafael J. Wysocki]

On Monday, 26 February 2007 12:58, Aneesh Kumar wrote:
> On 2/26/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > NOTE: Alternatively, we can just drop flush_signals() from there, but I'm not
> > sure that's the right thing to do.
> >
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > Since ____call_usermodehelper() calls flush_signals(current), the task that
> > enters it may miss a freezing request.  However, if ____call_usermodehelper()
> > clears TIF_FREEZE for the current task after flush_signals(current) returns, the
> > freezer will generate one more freezing request for this task.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/kmod.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > Index: linux-2.6.20-mm2/kernel/kmod.c
> > ===================================================================
> > --- linux-2.6.20-mm2.orig/kernel/kmod.c 2007-02-25 12:07:15.000000000 +0100
> > +++ linux-2.6.20-mm2/kernel/kmod.c      2007-02-25 12:49:16.000000000 +0100
> > @@ -34,6 +34,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/init.h>
> >  #include <linux/resource.h>
> > +#include <linux/freezer.h>
> >  #include <asm/uaccess.h>
> >
> >  extern int delete_module(const char *name, unsigned int flags);
> > @@ -258,6 +259,7 @@ static int ____call_usermodehelper(void
> >         new_session = key_get(sub_info->ring);
> >         flush_signals(current);
> >         spin_lock_irq(&current->sighand->siglock);
> > +       do_not_freeze(current);
> >         old_session = __install_session_keyring(current, new_session);
> >         flush_signal_handlers(current, 1);
> >         sigemptyset(&current->blocked);
> >
> 
> 
> this need a comment . Otherwise one reading will read it as we don't
> want it to freeze.
> 
> How about
> /*
> clears TIF_FREEZE for the current task after flush_signals(current)
> returns, so that the
> freezer will generate one more freezing request for this task.
> 
> */

Looks reasonable, but Oleg wants to drop the flush_signals(current) ...

Rafael