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 :-) -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
