On Tue, Feb 19, 2008 at 12:03:37AM +0100, Dmitry Adamushko wrote: > > 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);
The rules as written do seem to support your theory. The CPU has every right to delay the .k = k as late as the UNLOCK operation. On the read-side there is a full barrier implied by the set_current_state(TASK_INTERRUPTIBLE), however this synchronises us with the current global state, which may well not have the updated version of .k. That seems to imply that a write memory barrier would be sufficient to cover this. So three comments. First, should this not be an smp_wmb. Second, this memory barrier is paired with the barrier in set_current_state(TASK_INTERRUPTIBLE) and that probabally should be documented as part of this patch. Finally, I think the comment as is is hard to understand I got the sense of it backwards on first reading; perhaps something like this: /* * Ensure kthread_stop_info.k is visible before wakeup, paired * with barrier in set_current_state(). */ -apw -- 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/