Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1444721 > unrolled thread
| Started by | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| First post | 2016-07-16 04:00 +0200 |
| Last post | 2016-07-19 09:00 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v4] locking/pvqspinlock: Fix double hash race Wanpeng Li <kernellwp@gmail.com> - 2016-07-16 04:00 +0200
Re: [PATCH v4] locking/pvqspinlock: Fix double hash race Davidlohr Bueso <dave@stgolabs.net> - 2016-07-16 22:10 +0200
Re: [PATCH v4] locking/pvqspinlock: Fix double hash race Wanpeng Li <kernellwp@gmail.com> - 2016-07-25 12:40 +0200
Re: [PATCH v4] locking/pvqspinlock: Fix double hash race Wanpeng Li <kernellwp@gmail.com> - 2016-07-19 09:00 +0200
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2016-07-16 04:00 +0200 |
| Subject | [PATCH v4] locking/pvqspinlock: Fix double hash race |
| Message-ID | <rVn3z-5Yo-3@gated-at.bofh.it> |
From: Wanpeng Li <wanpeng.li@hotmail.com>
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: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Waiman Long <Waiman.Long@hpe.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
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);
--
2.1.0
[toc] | [next] | [standalone]
| From | Davidlohr Bueso <dave@stgolabs.net> |
|---|---|
| Date | 2016-07-16 22:10 +0200 |
| Message-ID | <rVE4p-8eJ-11@gated-at.bofh.it> |
| In reply to | #1444721 |
On Sat, 16 Jul 2016, Wanpeng Li wrote: >From: Wanpeng Li <wanpeng.li@hotmail.com> > >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: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> >Cc: Peter Zijlstra (Intel) <peterz@infradead.org> >Cc: Ingo Molnar <mingo@kernel.org> >Cc: Waiman Long <Waiman.Long@hpe.com> >Cc: Davidlohr Bueso <dave@stgolabs.net> >Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
[toc] | [prev] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2016-07-25 12:40 +0200 |
| Message-ID | <rYLsJ-3s1-3@gated-at.bofh.it> |
| In reply to | #1444906 |
Ping Ingo, Peterz. :) 2016-07-17 4:03 GMT+08:00 Davidlohr Bueso <dave@stgolabs.net>: > On Sat, 16 Jul 2016, Wanpeng Li wrote: > >> From: Wanpeng Li <wanpeng.li@hotmail.com> >> >> 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: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> >> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@kernel.org> >> Cc: Waiman Long <Waiman.Long@hpe.com> >> Cc: Davidlohr Bueso <dave@stgolabs.net> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > > > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
[toc] | [prev] | [next] | [standalone]
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2016-07-19 09:00 +0200 |
| Message-ID | <rWxax-FB-7@gated-at.bofh.it> |
| In reply to | #1444721 |
Sorry for the quick ping, but could we catch the merge window? Ingo. :) 2016-07-16 9:51 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > 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: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> > Cc: Peter Zijlstra (Intel) <peterz@infradead.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Waiman Long <Waiman.Long@hpe.com> > Cc: Davidlohr Bueso <dave@stgolabs.net> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > --- > 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); > -- > 2.1.0 > -- Regards, Wanpeng Li
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web