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
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 <[email protected]>
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 <[email protected]>
---
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);
}
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 <[email protected]>
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 <[email protected]>
---
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(¤t->sighand->siglock);
+ do_not_freeze(current);
old_session = __install_session_keyring(current, new_session);
flush_signal_handlers(current, 1);
sigemptyset(¤t->blocked);
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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);
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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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(¤t->sighand->siglock);
> + do_not_freeze(current);
> old_session = __install_session_keyring(current, new_session);
> flush_signal_handlers(current, 1);
> sigemptyset(¤t->blocked);
On 2/26/07, Rafael J. Wysocki <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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(¤t->sighand->siglock);
> + do_not_freeze(current);
> old_session = __install_session_keyring(current, new_session);
> flush_signal_handlers(current, 1);
> sigemptyset(¤t->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
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.
On Mon 2007-02-26 11:49:09, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
ACK.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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
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.
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
On Monday, 26 February 2007 12:58, Aneesh Kumar wrote:
> On 2/26/07, Rafael J. Wysocki <[email protected]> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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(¤t->sighand->siglock);
> > + do_not_freeze(current);
> > old_session = __install_session_keyring(current, new_session);
> > flush_signal_handlers(current, 1);
> > sigemptyset(¤t->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
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.
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 <[email protected]>
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 <[email protected]>
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);
}
> * 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
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
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.
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 <[email protected]>
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 <[email protected]>
---
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);
}
On 02/27, Rafael J. Wysocki wrote:
>
> Okay, patch updated, appended.
I think it is good. Srivatsa?
Oleg.
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
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
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.
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
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 <[email protected]>
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 <[email protected]>
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);
}
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.
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