lkml.org 
[lkml]   [2013]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2013-12-13 08:01    [W:0.208 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site