[PATCH, RFC] kthread: (possibly) a missing memory barrier in kthread_stop()

[Dmitry Adamushko <dmitry.adamushko@gmail.com>]

Hi,


[ description ]

Subject: kthread: add a memory barrier to kthread_stop()

'kthread' threads do a check in the following order:
- set_current_state(TASK_INTERRUPTIBLE);
- kthread_should_stop();

and set_current_state() implies an smp_mb().

on another side (kthread_stop), wake_up_process() does not seem to
guarantee a full mb.

And 'kthread_stop_info.k' must be visible before wake_up_process()
checks for/modifies a state of the 'kthread' task.

(the patch is at the end of the message)


[ more detailed description ]

the current code might well be safe in case a to-be-stopped 'kthread'
task is _not_ running on another CPU at the moment when kthread_stop()
is called (in this case, 'rq->lock' will act as a kind of synch.
point/barrier).

Another case is as follows:

CPU#0:

...
while (kthread_should_stop()) {

       if (condition)
             schedule();

       /* ... do something useful ... */   <--- EIP

       set_current_state(TASK_INTERRUPTIBLE);
}

so a 'kthread' task is about to call
set_current_state(TASK_INTERRUPTIBLE) ...


(in the mean time)

CPU#1:

kthread_stop()

-> kthread_stop_info.k = k                 (*)
-> wake_up_process()

wake_up_process() looks like:

(try_to_wake_up)

IRQ_OFF
LOCK

old_state = p->state;
if (!(old_state & state))                  (**)
         goto out;

...

UNLOCK
IRQ_ON


let's suppose (*) and (**) are reordered
(according to Documentation/memory-barriers.txt, neither IRQ_OFF nor
LOCK may prevent it from happening).

- the state is TASK_RUNNING, so we are about to return.

- CPU#1 is about to execute (*) (it's guaranteed to be done before
spin_unlock(&rq->lock) at the end of try_to_wake_up())


(in the mean time)

CPU#0:

- set_current_state(TASK_INTERRUPTIBLE);
- kthread_should_stop();

here, kthread_stop_info.k is not yet visible

- schedule()

... 

we missed a 'kthread_stop' event.

hum?


TIA,

---

From: Dmitry Adamushko <dmitry.adamushko@gmail.com>
Subject: kthread: add a memory barrier to kthread_stop()

'kthread' threads do a check in the following order:
- set_current_state(TASK_INTERRUPTIBLE);
- kthread_should_stop();

and set_current_state() implies an smp_mb().

on another side (kthread_stop), wake_up_process() is not guaranteed to
act as a full mb.

'kthread_stop_info.k' must be visible before wake_up_process() checks
for/modifies a state of the 'kthread' task.


Signed-off-by: Dmitry Adamushko <dmitry.adamushko@gmail.com>


diff --git a/kernel/kthread.c b/kernel/kthread.c
index 0ac8878..5167110 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -211,6 +211,10 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+
+	/* The previous store operation must not get ahead of the wakeup. */
+	smp_mb();
+
 	wake_up_process(k);
 	put_task_struct(k);