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

Rafael J. Wysocki
2007-04-20T08:50:47Z

On Thursday, 19 April 2007 23:31, Andrew Morton wrote:
> On Thu, 19 Apr 2007 17:34:19 +0530
> Gautham R Shenoy <ego@in.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);