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

[Peter Zijlstra]

On Mon, Jun 25, 2018 at 04:56:11PM +0200, Andrea Parri wrote:

> Ah, before sending v2, I'd really appreciate some comments on the XXXs
> I've added to wait_woken() as I'm not sure I understand the pattern in
> questions.

Oh man, lemme see if I can remember how all that was supposed to work.

So the basic idea was that we cannot rely on the normal task->state
rules because testing @condition can schedule itself. So instead we add
more state. But then we need to ensure that if we either don't loose a
wake or loose the wakeup state.

> 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.

> 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.