2016-08-03 11:28 GMT+08:00 Waiman Long <waiman.long@xxxxxxx>: > On 07/27/2016 07:30 AM, Wanpeng Li wrote: >> >> From: Wanpeng Li<wanpeng.li@xxxxxxxxxxx> >> >> 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@xxxxxxxxxxxx> >> Reviewed-by: Pan Xinhui<xinhui.pan@xxxxxxxxxxxxxxxxxx> >> Cc: Peter Zijlstra (Intel)<peterz@xxxxxxxxxxxxx> >> Cc: Ingo Molnar<mingo@xxxxxxxxxx> >> Cc: Waiman Long<Waiman.Long@xxxxxxx> >> Cc: Davidlohr Bueso<dave@xxxxxxxxxxxx> >> Signed-off-by: Wanpeng Li<wanpeng.li@xxxxxxxxxxx> >> --- >> 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 <Waiman.Long@xxxxxxx> Thanks. :) Regards, Wanpeng Li -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
