From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932537AbcE0Hjy (ORCPT ); Fri, 27 May 2016 03:39:54 -0400 Received: from mail-oi0-f65.google.com ([209.85.218.65]:34662 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932228AbcE0Hjw (ORCPT ); Fri, 27 May 2016 03:39:52 -0400 Date: Fri, 27 May 2016 15:43:31 +0800 From: Boqun Feng To: Waiman Long Cc: Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, Pan Xinhui , Scott J Norton , Douglas Hatch Subject: Re: [PATCH 1/2] locking/pvqspinlock: Fix missed PV wakeup problem Message-ID: <20160527074331.GB8096@insomnia> References: <1464286918-39748-1-git-send-email-Waiman.Long@hpe.com> <1464286918-39748-2-git-send-email-Waiman.Long@hpe.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="s2ZSL+KKDSLx8OML" Content-Disposition: inline In-Reply-To: <1464286918-39748-2-git-send-email-Waiman.Long@hpe.com> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --s2ZSL+KKDSLx8OML Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Waiman, On Thu, May 26, 2016 at 02:21:57PM -0400, Waiman Long wrote: > Currently, calling pv_hash() and setting _Q_SLOW_VAL is only > done once for any pv_node. It is either in pv_kick_node() or in > pv_wait_head_or_lock(). Because of lock stealing, a pv_kick'ed node is > not guaranteed to get the lock before the spinning threshold expires > and has to call pv_wait() again. As a result, the new lock holder > won't see _Q_SLOW_VAL and so won't wake up the sleeping vCPU. >=20 > This patch fixes this missed PV wakeup problem by allowing multiple > _Q_SLOW_VAL settings within pv_wait_head_or_lock() and matching each > successful setting of _Q_SLOW_VAL to a pv_hash() call. >=20 > Reported-by: Pan Xinhui > Signed-off-by: Waiman Long > --- > kernel/locking/qspinlock_paravirt.h | 48 ++++++++++++++++++++++-------= ----- > 1 files changed, 31 insertions(+), 17 deletions(-) >=20 > diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlo= ck_paravirt.h > index 21ede57..452d06d 100644 > --- a/kernel/locking/qspinlock_paravirt.h > +++ b/kernel/locking/qspinlock_paravirt.h > @@ -369,12 +369,16 @@ static void pv_kick_node(struct qspinlock *lock, st= ruct mcs_spinlock *node) > /* > * Put the lock into the hash table and set the _Q_SLOW_VAL. > * > - * As this is the same vCPU that will check the _Q_SLOW_VAL value and > - * the hash table later on at unlock time, no atomic instruction is > - * needed. > + * It is very unlikely that this will race with the _Q_SLOW_VAL setting > + * in pv_wait_head_or_lock(). However, we use cmpxchg() here to be > + * sure that we won't do a double pv_hash(). > + * > + * As it is the lock holder, it won't race with > + * __pv_queued_spin_unlock(). > */ > - WRITE_ONCE(l->locked, _Q_SLOW_VAL); > - (void)pv_hash(lock, pn); > + if (likely(cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL) > + =3D=3D _Q_LOCKED_VAL)) > + pv_hash(lock, pn); > } > =20 > /* > @@ -389,18 +393,10 @@ pv_wait_head_or_lock(struct qspinlock *lock, struct= mcs_spinlock *node) > { > struct pv_node *pn =3D (struct pv_node *)node; > struct __qspinlock *l =3D (void *)lock; > - struct qspinlock **lp =3D NULL; > int waitcnt =3D 0; > int loop; > =20 > /* > - * If pv_kick_node() already advanced our state, we don't need to > - * insert ourselves into the hash table anymore. > - */ > - if (READ_ONCE(pn->state) =3D=3D vcpu_hashed) > - lp =3D (struct qspinlock **)1; > - > - /* > * Tracking # of slowpath locking operations > */ > qstat_inc(qstat_pv_lock_slowpath, true); > @@ -422,11 +418,19 @@ pv_wait_head_or_lock(struct qspinlock *lock, struct= mcs_spinlock *node) > goto gotlock; > cpu_relax(); > } > - clear_pending(lock); > =20 > + /* > + * Make sure the lock value check below is executed after > + * all the previous loads. > + */ > + smp_rmb(); > =20 > - if (!lp) { /* ONCE */ > - lp =3D pv_hash(lock, pn); > + /* > + * Set _Q_SLOW_VAL and hash the PV node, if necessary. > + */ > + if (READ_ONCE(l->locked) !=3D _Q_SLOW_VAL) { > + struct qspinlock **lp =3D pv_hash(lock, pn); > + u8 locked; > =20 Just out of curiosity, what if the following sequence happens: CPU 0 CPU 1 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D spin_lock(): spin_lock(): pv_kick_node(): pv_wait_head_or_lock(): if (READ_ONCE(l->locked) !=3D _Q_SLOW_VAL) { // True pv_hash(); cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL); pv_hash(); locked =3D xchg(&l->locked, _Q_SLOW_VAL); do_something(); if(...) { } spin_unlock(): pv_unhash(); else if (unlikely(locked =3D=3D _Q_SLOW_VAL)) { WRITE_ONCE(*lp, NULL); because pv_hash() on CPU 1 called before the one on CPU 0, therefore the hash entry from CPU 1 is preceding the hash entry from CPU 0 in the hash table, so that when pv_unhash() called, hash entry from CPU 1 will be unhashed, however, the WRITE_ONCE(*lp, NULL) on CPU 1 will also unhash the same entry, leaving that hash entry from CPU 0 not unhashed. This could result in several interesting problems, right? ;-) Am I missing something here? Regards, Boqun > /* > * We must hash before setting _Q_SLOW_VAL, such that > @@ -439,7 +443,8 @@ pv_wait_head_or_lock(struct qspinlock *lock, struct m= cs_spinlock *node) > * > * Matches the smp_rmb() in __pv_queued_spin_unlock(). > */ > - if (xchg(&l->locked, _Q_SLOW_VAL) =3D=3D 0) { > + locked =3D xchg(&l->locked, _Q_SLOW_VAL); > + if (locked =3D=3D 0) { > /* > * The lock was free and now we own the lock. > * Change the lock value back to _Q_LOCKED_VAL > @@ -447,9 +452,18 @@ pv_wait_head_or_lock(struct qspinlock *lock, struct = mcs_spinlock *node) > */ > WRITE_ONCE(l->locked, _Q_LOCKED_VAL); > WRITE_ONCE(*lp, NULL); > + clear_pending(lock); > goto gotlock; > + } else if (unlikely(locked =3D=3D _Q_SLOW_VAL)) { > + /* > + * Racing with pv_kick_node(), need to undo > + * the pv_hash(). > + */ > + WRITE_ONCE(*lp, NULL); > } > } > + clear_pending(lock); /* Enable lock stealing */ > + > WRITE_ONCE(pn->state, vcpu_halted); > qstat_inc(qstat_pv_wait_head, true); > qstat_inc(qstat_pv_wait_again, waitcnt); > --=20 > 1.7.1 >=20 --s2ZSL+KKDSLx8OML Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJXR/qgAAoJEEl56MO1B/q4dCsIAKHKbTEoqMGzkuwN9XCTs0WQ gv24AF6FzdKeYfBk9siO7iOBjx8bDwJ3mCf/d08hvxLk2QASVd6isVKHBT0c2+OS SnbwpM3wimHXab/XXwwHDvYUWDI9lj40Vh7WLUIQRpBpgD77h9CgmKpkE4IP+SsY OgAVILXcCc2EfZL2NBwlBjvcEFEgmK2uhDXRo/yct2dTmgFcN2ZFG+GEbaK5sulp mKCw5FwKfeO0al35fH+tieRUieNGA6oTZYwDVRLAg2X3uOtMBUR3Edp3oVZCOIj3 pq/U3i9DpjAiRBYVRdfYZtOUxxH1B4tvoYQMcC4wvbR1DaTp58LXx8HWg95bDmc= =HyTK -----END PGP SIGNATURE----- --s2ZSL+KKDSLx8OML--