Messages in this thread | | | Date | Thu, 14 Jul 2016 10:52:08 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v3] locking/pvqspinlock: restore/set vcpu_hashed state after failing adaptive locking spinning |
| |
On 07/14/2016 07:39 AM, Wanpeng Li wrote: > From: Wanpeng Li<wanpeng.li@hotmail.com> > > When the lock holder vCPU is racing with the queue head: > > CPU 0 (lock holder) CPU 1 (queue head) > =================== ================= > spin_lock(); spin_lock(); > pv_kick_node(): pv_wait_head_or_lock(): > if (!lp) { > lp = pv_hash(lock, pn); > xchg(&l->locked, _Q_SLOW_VAL); > } > WRITE_ONCE(pn->state, vcpu_halted); > cmpxchg(&pn->state, > vcpu_halted, vcpu_hashed); > WRITE_ONCE(l->locked, _Q_SLOW_VAL); > (void)pv_hash(lock, pn); > > In this case, lock holder inserts the pv_node of queue head into the > hash table and set _Q_SLOW_VAL which can result in hash entry leak. > This patch avoids it by restoring/setting vcpu_hashed state after > failing adaptive locking spinning. > > 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> > --- > v2 -> v3: > * fix typo in patch description > v1 -> v2: > * adjust patch description > > kernel/locking/qspinlock_paravirt.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h > index 21ede57..ac7d20b 100644 > --- a/kernel/locking/qspinlock_paravirt.h > +++ b/kernel/locking/qspinlock_paravirt.h > @@ -450,7 +450,7 @@ pv_wait_head_or_lock(struct qspinlock *lock, struct mcs_spinlock *node) > goto gotlock; > } > } > - WRITE_ONCE(pn->state, vcpu_halted); > + 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);
As pv_kick_node() is called immediately after designating the next node as the queue head, the chance of this racing is possible, but is not likely unless the lock holder vCPU gets preempted for a long time at that right moment. This change does not do any harm though, so I am OK with that. However, I do want you to add a comment about the possible race in the code as it isn't that obvious or likely.
Cheers, Longman
| |