> > For example, the second comment says:
> >
> > /*
> > * The below implies an smp_mb(), it too pairs with the smp_wmb() from
> > * woken_wake_function() such that we must either observe the wait
> > * condition being true _OR_ WQ_FLAG_WOKEN such that we will not miss
> > * an event.
> > */
> >
> > From this I understand:
> >
> > wq_entry->flags &= ~WQ_FLAG_WOKEN; condition = true;
> > smp_mb() // B smp_wmb(); // C
> > [next iteration of the loop] wq_entry->flags |= WQ_FLAG_WOKEN;
> > if (condition)
> > break;
> >
> > BUG_ON(!condition && !(wq_entry->flags & WQ_FLAG_WOKEN))
>
> Right, basically if we get a spurious wakeup and our ttwu() 'fails', we
> must either see condition on the next iteration, or ensure the next
> iteration doesn't sleep, so we'll loop around and test condition yet
> again.
>
> > IOW, this is an R-like pattern: if this is the case, the smp_wmb() does
> > _not_ prevent the BUG_ON() from firing; according to LKMM (and powerpc)
> > a full barrier would be needed.
>
> Hmmm, how come? store-store collision? Yes I suppose you're right.
Ehh, the corresponding powerpc test is architecturally allowed; in the
operational model, the BUG_ON() state can be reached by following the
following steps:
1. let the writes all reach the storage subsystem,
2. commit the partial coherence order from "->flags |= WQ_FLAG_WOKEN"
to "->flags &= ~WQ_FLAG_WOKEN"
3. propagate "->flags &= ~WQ_FLAG_WOKEN" to the other thread
4. commit and acknowledge the sync (B)
5. satisfy the read
6. propagate "condition = true" and the lwsync (C) to the other thread.
AFAICT, this state remains _unobserved_ via litmus7 experiments.
>
> > 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).
I am planning to send these changes (smp_mb() in woken_wake_function()
and fixes to the comments) as a separate patch.
Thanks,
Andrea
--
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
