Re: [PATCH] doc: Update wake_up() & co. memory-barrier guarantees

Peter Zijlstra
2018-06-26T15:30:42Z

On Tue, Jun 26, 2018 at 12:09:42PM +0200, Andrea Parri wrote:
> > > Same RFC for the first comment:
> > >
> > > /*
> > > * The above implies an smp_mb(), which matches with the smp_wmb() from
> > > * woken_wake_function() such that if we observe WQ_FLAG_WOKEN we must
> > > * also observe all state before the wakeup.
> > > */
> > >
> > > What is the corresponding snippet & BUG_ON()?
> >
> > The comment there suggest:
> >
> > if (condition)
> > break;
> >
> > set_current_state(UNINTERRUPTIBLE); condition = true;
> > /* smp_mb() */ smp_wmb();
> > wq_entry->flags |= WQ_FLAG_WOKEN;
> > if (!wq_entry->flags & WQ_FLAG_WOKEN)
> > schedule();
> >
> >
> > BUG_ON((wq_entry->flags & WQ_FLAG_WOKEN) && !condition);
> >
> >
> > But looking at that now, I think that's wrong. Because the the purpose
> > was that, if we don't do the try_to_wake_up(), our task still needs to
> > observe the condition store.
> >
> > But for that we need a barrier between the wq_entry->flags load and the
> > second condition test, which would (again) be B, not A.
>
> Agreed. Now that I stared at the code a bit more, I think that (A) is
> still needed for the synchronization on "->state" and "->flags" (an SB
> pattern seems again to be hidden in the call to try_to_wake_up()):
>
> p->state = mode; wq_entry->flags |= WQ_FLAG_WOKEN;
> smp_mb(); // A try_to_wake_up():
> if (!(wq_entry->flags & WQ_FLAG_WOKEN)) <full barrier>
> schedule() if (!(p->state & mode))
> goto out;
>
> BUG_ON(!(wq_entry->flags & WQ_FLAG_WOKEN) && !(p->state & mode))
>
> So, I think that we should keep (A).

Yes, very much so. Once we actually get to use ttwu() that barrier is
required.

> I am planning to send these changes (smp_mb() in woken_wake_function()
> and fixes to the comments) as a separate patch.

Probably makes sense. Thanks for looking at this, I have vague memories
of being slightly confused when I wrote all that :-)