[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <200704201054.38037.rjw@sisk.pl>
Date: Fri, 20 Apr 2007 10:54:36 +0200
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: ego@...ibm.com, Oleg Nesterov <oleg@...sign.ru>,
linux-kernel@...r.kernel.org, mingo@...e.hu, vatsa@...ibm.com,
paulmck@...ibm.com, pavel@....cz
Subject: Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race
On Thursday, 19 April 2007 23:31, Andrew Morton wrote:
> On Thu, 19 Apr 2007 17:34:19 +0530
> Gautham R Shenoy <ego@...ibm.com> wrote:
>
> > Threads which wait for completion on a frozen thread might result in
> > causing the freezer to fail, if the waiting thread is freezeable.
> >
> > There are some well known cases where it's preferable to temporarily thaw
> > the frozen process, finish the wait for completion and allow both the
> > processes to call try_to_freeze.
> >
> > kthread_stop is one such case.
>
> hm.
>
> > flush_workqueue might be another.
>
> flush_workqueue() just needs to die. I think there are (almost) no
> legitimate users of it once cancel_work_sync() is merged.
>
> > This patch attempts to address such a situation with a fix for kthread_stop.
>
> Via wholly undescribed means :(
Yeah, I have the same problem with it. :-)
> > Strictly experimental. Compile tested on i386.
>
> Rather than doing <whatever you did>, perhaps we could make the freezing
> process a dual-pass thing. On pass 1, mark all the threads as "we'll be
> freezing you soon" and on the second pass, do the actual freezing. Then,
> in problematic places such as kthread_stop() we can look to see if we'll
> soon be asked to freeze and if so, run try_to_freeze().
>
> Of course, running try_to_freeze() in kthread_stop() would be very wrong,
> so we'd actually need to do it in callers, preferably via a new
> kthread_stop_freezeable() wrapper.
>
> And the two-pass-freeze thing is of course racy. It's also unnecessary:
> setting a flag on every task in the machine is equivalent to setting a
> global variable. So perhaps just use a global variable?
>
> int kthread_stop_freezeable(struct task_struct *k)
> {
> if (freeze_state == ABOUT_TO_START) {
> wait_for(freeze_state == STARTED);
> try_to_freeze();
> }
> kthread_stop(k);
> }
>
> which is theoretically racy if another freeze_processes() starts
> immediately. Anyway - please have a think about it ;)
Hmm, can't we do something like this instead:
---
kernel/kthread.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Index: linux-2.6.21-rc7/kernel/kthread.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/kthread.c
+++ linux-2.6.21-rc7/kernel/kthread.c
@@ -13,6 +13,7 @@
#include <linux/file.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/freezer.h>
#include <asm/semaphore.h>
/*
@@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
/* Now set kthread_should_stop() to true, and wake it up. */
kthread_stop_info.k = k;
+ if (!(current->flags & PF_NOFREEZE)) {
+ /* If we are freezable, the freezer will wait for us */
+ task_lock(k);
+ k->flags |= PF_NOFREEZE;
+ if (frozen(k))
+ k->flags &= ~PF_FROZEN;
+
+ task_unlock(k);
+ }
wake_up_process(k);
put_task_struct(k);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists