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

[Rafael J. Wysocki]

On Thursday, 19 April 2007 23:31, Andrew Morton wrote:
> On Thu, 19 Apr 2007 17:34:19 +0530
> Gautham R Shenoy <ego@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/