On Wed, Jun 14, 2017 at 11:02:40AM -0400, Steven Rostedt wrote: > On Wed, 14 Jun 2017 09:10:15 -0400 > Steven Rostedt <[email protected]> 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
