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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/