Messages in this thread Patch in this message |  | | Subject | [PATCH, RFC] kthread: (possibly) a missing memory barrier in kthread_stop() | | From | Dmitry Adamushko <> | | Date | Tue, 19 Feb 2008 00:03:37 +0100 |
| |
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);
|  |