Messages in this thread | | | Date | Thu, 12 Dec 2013 09:42:27 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH 1/3] wait-simple: Introduce the simple waitqueue implementation |
| |
On Thu, 12 Dec 2013 12:44:47 +0100 Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Dec 11, 2013 at 08:06:37PM -0500, Paul Gortmaker wrote: > > +/* Adds w to head->task_list. Must be called with head->lock locked. */ > > +static inline void __swait_enqueue(struct swait_queue_head *head, > > + struct swaiter *w) > > +{ > > + list_add(&w->node, &head->task_list); > > + /* We can't let the condition leak before the setting of head */ > > + smp_mb(); > > +} > > > +unsigned int > > +__swake_up_locked(struct swait_queue_head *head, unsigned int state, > > + unsigned int num) > > +{ > > + struct swaiter *curr, *next; > > + int woken = 0; > > + > > + list_for_each_entry_safe(curr, next, &head->task_list, node) { > > + if (wake_up_state(curr->task, state)) { > > + __swait_dequeue(curr); > > + /* > > + * The waiting task can free the waiter as > > + * soon as curr->task = NULL is written, > > + * without taking any locks. A memory barrier > > + * is required here to prevent the following > > + * store to curr->task from getting ahead of > > + * the dequeue operation. > > + */ > > + smp_wmb(); > > + curr->task = NULL; > > + if (++woken == num) > > + break; > > + } > > + } > > + return woken; > > +} > > Are these two barriers matched or are they both unmatched and thus > probabyl wrong?
Nope, the two are unrelated. The the smp_wmb() is to synchronize with the swait_finish() code. When the task wakes up, it checks if w->task is NULL, and if it is it does not grab the head->lock and does not dequeue it, it simply exits, where the caller can then free the swaiter structure.
Without the smp_wmb(), the curr->task can be set to NULL before we dequeue it, and if the woken up process sees that NULL, it can jump right to freeing the swaiter structure and cause havoc with this __swait_dequeue().
The first smp_mb() is about the condition in:
+#define __swait_event(wq, condition) \ +do { \ + DEFINE_SWAITER(__wait); \ + \ + for (;;) { \ + swait_prepare(&wq, &__wait, TASK_UNINTERRUPTIBLE); \ + if (condition) \ + break; \ + schedule(); \ + } \ + swait_finish(&wq, &__wait); \ +} while (0)
without the smp_mb(), it is possible that the condition can leak into the critical section of swait_prepare() and have the old condition seen before the task is added to the wait list. My submission of this patch described it in more details:
https://lkml.org/lkml/2013/8/19/275
> > In any case the comments need updating to be more explicit.
Yeah, I can add documentation about this as well. The smp_wmb() I think is probably documented enough, but the two smp_mb()s need a little more explanation.
-- Steve
| |