On Mon, Apr 10, 2017 at 12:20:53PM -0400, Alan Stern wrote:
> On Mon, 10 Apr 2017, Paul E. McKenney wrote:
>
> > > But I would like to get this matter settled first. Is the explicit
> > > barrier truly necessary?
> >
> > If you are using wait_event()/wake_up() or friends, the explicit
> > barrier -is- necessary. To see this, look at v4.10's wait_event():
> >
> > #define wait_event(wq, condition) \
> > do { \
> > might_sleep(); \
> > if (condition) \
> > break; \
> > __wait_event(wq, condition); \
> > } while (0)
> >
> > As you can see, if the condition is set just before the wait_event()
> > macro checks it, there is no ordering whatsoever.
>
> This is true, but it is not relevant to the question I was asking.
Apologies! What I get for answering email too early on Monday, I guess...
> > And if wake_up()
> > finds nothing to wake up, there is no relevant ordering on that side,
> > either.
> >
> > So you had better supply your own ordering, period, end of story.
>
> The question is: Exactly what ordering do I need to supply? The
> ordering among my own variables is okay; I know how to deal with that.
> But what about the ordering between my variables and current->state?
The ordering with current->state is sadly not relevant because it is
only touched if wake_up() actually wakes the process up.
> For example, __wait_event() calls prepare_to_wait(), which calls
> set_current_state(), which calls smp_store_mb(), thereby inserting a
> full memory barrier between setting current->state and checking the
> condition. But I didn't see any comparable barrier inserted by
> wake_up(), between setting the condition and checking task->state.
>
> However, now that I look more closely, I do see that wakeup_process()
> calls try_to_wake_up(), which begins with:
>
> /*
> * If we are going to wake up a thread waiting for CONDITION we
> * need to ensure that CONDITION=1 done by the caller can not be
> * reordered with p->state check below. This pairs with mb() in
> * set_current_state() the waiting thread does.
> */
> smp_mb__before_spinlock();
> raw_spin_lock_irqsave(&p->pi_lock, flags);
> if (!(p->state & state))
>
> So it does insert a full barrier after all, and there is nothing to
> worry about.
Nice!
Hmmm...
Another valid (and I believe more common) idiom is this:
spin_lock(&mylock);
changes_that_must_be_visible_to_woken_thread();
WRITE_ONCE(need_wake_up, true);
spin_unlock(&mylock);
---
wait_event(wq, READ_ONCE(need_wake_up));
spin_lock(&mylock);
access_variables_used_by_waking_thread();
spin_unlock(&mylock);
In this case, the locks do all the required ordering.
> This also means that the analysis provided by Thinh Nguyen in the
> original patch description is wrong.
And that the bug is elsewhere?
Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
