Re: [PATCH tip/sched/core] Add comments to aid in safer usage of swake_up.

Krister Johansen
2017-06-14T16:25:58Z

On Wed, Jun 14, 2017 at 11:02:40AM -0400, Steven Rostedt wrote:
> On Wed, 14 Jun 2017 09:10:15 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Now let's make it simpler. I'll even add the READ_ONCE and WRITE_ONCE
> > where applicable.
> >
> >
> > CPU0 CPU1
> > ---- ----
> > LOCK(A)
> >
> > LOCK(B)
> > WRITE_ONCE(X, INIT)
> >
> > (the cpu may postpone writing X)
> >
> > (the cpu can fetch wq list here)
> > list_add(wq, q)
> >
> > UNLOCK(B)
> >
> > (the cpu may fetch old value of X)
> >
> > (write of X happens here)
> >
> > if (READ_ONCE(X) != init)
> > schedule();
> >
> > UNLOCK(A)
> >
> > if (list_empty(wq))
> > return;
> >
> > Tell me again how the READ_ONCE() and WRITE_ONCE() helps in this
> > scenario?
> >
> > Because we are using spinlocks, this wont be an issue for most
> > architectures. The bug happens if the fetching of the list_empty()
> > leaks into before the UNLOCK(A).
> >
> > If the reading/writing of the list and the reading/writing of gp_flags
> > gets reversed in either direction by the CPU, then we have a problem.
>
> FYI..
>
> Both sides need a memory barrier. Otherwise, even with a memory barrier
> on CPU1 we can still have:
>
>
> CPU0 CPU1
> ---- ----
>
> LOCK(A)
> LOCK(B)
>
> list_add(wq, q)
>
> (cpu waits to write wq list)
>
> (cpu fetches X)
>
> WRITE_ONCE(X, INIT)
>
> UNLOCK(A)
>
> smp_mb();
>
> if (list_empty(wq))
> return;
>
> (cpu writes wq list)
>
> UNLOCK(B)
>
> if (READ_ONCE(X) != INIT)
> schedule()
>
>
> Luckily for us, there is a memory barrier on CPU0. In
> prepare_to_swait() we have:
>
> raw_spin_lock_irqsave(&q->lock, flags);
> __prepare_to_swait(q, wait);
> set_current_state(state);
> raw_spin_unlock_irqrestore(&q->lock, flags);
>
> And that set_current_state() call includes a memory barrier, which will
> prevent the above from happening, as the addition to the wq list must
> be flushed before fetching X.
>
> I still strongly believe that the swait_active() requires a memory
> barrier.

FWLIW, I agree. There was a smb_mb() in RT-linux's equivalent of
swait_activate().

https://www.spinics.net/lists/linux-rt-users/msg10340.html

If the barrier goes in swait_active() then we don't have to require all
of the callers of swait_active and swake_up to issue the barrier
instead. Handling this in swait_active is likely to be less error
prone. Though, we could also do something like wq_has_sleeper() and use
that preferentially in swake_up and its variants.

-K