Messages in this thread | | | Date | Wed, 14 Jun 2017 11:02:40 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH tip/sched/core] Add comments to aid in safer usage of swake_up. |
| |
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.
-- Steve
| |