Re: [PATCH RESEND v4] locking/pvqspinlock: Fix double hash race

2016-08-03 11:28 GMT+08:00 Waiman Long [off-list ref]:

On 07/27/2016 07:30 AM, Wanpeng Li wrote:

quoted

From: Wanpeng Li<redacted>

When the lock holder vCPU is racing with the queue head vCPU:

lock holder vCPU             queue head vCPU
=====================        ==================

node->locked = 1;
<preemption>                  READ_ONCE(node->locked)
    ...                       pv_wait_head_or_lock():
                                SPIN_THRESHOLD loop;
                                pv_hash();
                                lock->locked = _Q_SLOW_VAL;
                                node->state  = vcpu_hashed;
pv_kick_node():
   cmpxchg(node->state,
      vcpu_halted, vcpu_hashed);
   lock->locked = _Q_SLOW_VAL;
   pv_hash();

With preemption at the right moment, it is possible that both the
lock holder and queue head vCPUs can be racing to set node->state
which can result in hash entry race. Making sure the state is never
set to vcpu_halted will prevent this racing from happening.

This patch fix it by setting vcpu_hashed after we did all hash thing.

Reviewed-by: Davidlohr Bueso<dave@stgolabs.net>
Reviewed-by: Pan Xinhui<redacted>
Cc: Peter Zijlstra (Intel)<peterz@infradead.org>
Cc: Ingo Molnar<mingo@kernel.org>
Cc: Waiman Long<redacted>
Cc: Davidlohr Bueso<dave@stgolabs.net>
Signed-off-by: Wanpeng Li<redacted>
---
v3 ->  v4:
  * update patch subject
  * add code comments
v2 ->  v3:
  * fix typo in patch description
v1 ->  v2:
  * adjust patch description

  kernel/locking/qspinlock_paravirt.h | 23 ++++++++++++++++++++++-
  1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/qspinlock_paravirt.h

b/kernel/locking/qspinlock_paravirt.h
index 21ede57..ca96db4 100644

--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h

@@ -450,7 +450,28 @@ pv_wait_head_or_lock(struct qspinlock *lock, struct

mcs_spinlock *node)
                                goto gotlock;
                        }
                }
-               WRITE_ONCE(pn->state, vcpu_halted);
+               /*
+                * lock holder vCPU             queue head vCPU
+                * ----------------             ---------------
+                * node->locked = 1;
+                *<preemption>                  READ_ONCE(node->locked)
+                *    ...                       pv_wait_head_or_lock():
+                *                                SPIN_THRESHOLD loop;
+                *                                pv_hash();
+                *                                lock->locked =
_Q_SLOW_VAL;
+                *                                node->state  =
vcpu_hashed;
+                * pv_kick_node():
+                *   cmpxchg(node->state,
+                *      vcpu_halted, vcpu_hashed);
+                *   lock->locked = _Q_SLOW_VAL;
+                *   pv_hash();
+                *
+                * With preemption at the right moment, it is possible
that both the
+                * lock holder and queue head vCPUs can be racing to set
node->state.
+                * Making sure the state is never set to vcpu_halted will
prevent this
+                * racing from happening.
+                */
+               WRITE_ONCE(pn->state, vcpu_hashed);
                qstat_inc(qstat_pv_wait_head, true);
                qstat_inc(qstat_pv_wait_again, waitcnt);
                pv_wait(&l->locked, _Q_SLOW_VAL);

Acked-by: Waiman Long <redacted>

Thanks. :)

Regards,
Wanpeng Li