Messages in this thread | | | Date | Tue, 19 Feb 2008 10:53:28 +0100 | From | "Dmitry Adamushko" <> | Subject | Re: [PATCH, RFC] kthread: (possibly) a missing memory barrier in kthread_stop() |
| |
On 19/02/2008, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote: > [ ... ] > > > > > > 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(); > > Does this not also imply you need a matching barrier in > kthread_should_stop() ?
Yes, but only when it's used in combination with something that alters a state of the task.
So it's rather a question of the interface-design.
We currently impose a requirement on how a main loop of 'kthread' threads (ok, so it seems to dictate a policy :-) has to be orginized. Namely, the following sequence must be kept in order:
(1) set_current_task(TASK_INTERRUPTIBLE); (2) kthread_should_stop() ... - schedule()
and (1) already provides a mb which becomes a "matching barrier" on the kthread_should_stop() side.
-- Best regards, Dmitry Adamushko
| |