On Wed, 14 Jun 2017 09:10:15 -0400
Steven Rostedt <rost...@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.
-- Steve
