From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753798AbdBHHOG (ORCPT ); Wed, 8 Feb 2017 02:14:06 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:32893 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753601AbdBHHOF (ORCPT ); Wed, 8 Feb 2017 02:14:05 -0500 Date: Wed, 8 Feb 2017 15:15:22 +0800 From: Boqun Feng To: Pan Xinhui Cc: Xinhui Pan , Waiman Long , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs Message-ID: <20170208071522.GD9178@tardis.cn.ibm.com> References: <1482697561-23848-1-git-send-email-longman@redhat.com> <20170208040540.GB9178@tardis.cn.ibm.com> <20170208060939.GC9178@tardis.cn.ibm.com> <9c22e27d-e80e-81f9-d7a0-992e80425f44@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="IMjqdzrDRly81ofr" Content-Disposition: inline In-Reply-To: <9c22e27d-e80e-81f9-d7a0-992e80425f44@linux.vnet.ibm.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --IMjqdzrDRly81ofr Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 08, 2017 at 03:09:33PM +0800, Pan Xinhui wrote: >=20 >=20 > =E5=9C=A8 2017/2/8 14:09, Boqun Feng =E5=86=99=E9=81=93: > > On Wed, Feb 08, 2017 at 12:05:40PM +0800, Boqun Feng wrote: > > > On Wed, Feb 08, 2017 at 11:39:10AM +0800, Xinhui Pan wrote: > > > > 2016-12-26 4:26 GMT+08:00 Waiman Long : > > > >=20 > > > > > A number of cmpxchg calls in qspinlock_paravirt.h were replaced b= y more > > > > > relaxed versions to improve performance on architectures that use= LL/SC. > > > > >=20 > > > > > All the locking related cmpxchg's are replaced with the _acquire > > > > > variants: > > > > > - pv_queued_spin_steal_lock() > > > > > - trylock_clear_pending() > > > > >=20 > > > > > The cmpxchg's related to hashing are replaced by either by the _r= elease > > > > > or the _relaxed variants. See the inline comment for details. > > > > >=20 > > > > > Signed-off-by: Waiman Long > > > > >=20 > > > > > v1->v2: > > > > > - Add comments in changelog and code for the rationale of the c= hange. > > > > >=20 > > > > > --- > > > > > kernel/locking/qspinlock_paravirt.h | 50 +++++++++++++++++++++++= +------ > > > > > ------- > > > > > 1 file changed, 33 insertions(+), 17 deletions(-) > > > > >=20 > > > > >=20 > > > > > @@ -323,8 +329,14 @@ static void pv_wait_node(struct mcs_spinlock= *node, > > > > > struct mcs_spinlock *prev) > > > > > * If pv_kick_node() changed us to vcpu_hashed, r= etain that > > > > > * value so that pv_wait_head_or_lock() knows to = not also > > > > > try > > > > > * to hash this lock. > > > > > + * > > > > > + * The smp_store_mb() and control dependency abov= e will > > > > > ensure > > > > > + * that state change won't happen before that. > > > > > Synchronizing > > > > > + * with pv_kick_node() wrt hashing by this waiter= or by the > > > > > + * lock holder is done solely by the state variab= le. There > > > > > is > > > > > + * no other ordering requirement. > > > > > */ > > > > > - cmpxchg(&pn->state, vcpu_halted, vcpu_running); > > > > > + cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_run= ning); > > > > >=20 > > > > > /* > > > > > * If the locked flag is still not set after wake= up, it is > > > > > a > > > > > @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *l= ock, > > > > > struct mcs_spinlock *node) > > > > > * pv_wait_node(). If OTOH this fails, the vCPU was runni= ng and > > > > > will > > > > > * observe its next->locked value and advance itself. > > > > > * > > > > > - * Matches with smp_store_mb() and cmpxchg() in pv_wait_n= ode() > > > > > + * Matches with smp_store_mb() and cmpxchg_relaxed() in > > > > > pv_wait_node(). > > > > > + * A release barrier is used here to ensure that node->lo= cked is > > > > > + * always set before changing the state. See comment in > > > > > pv_wait_node(). > > > > > */ > > > > > - if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) !=3D vc= pu_halted) > > > > > + if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed) > > > > > + !=3D vcpu_halted) > > > > > return; > > > > >=20 > > > > > hi, Waiman > > > > We can't use _release here, a full barrier is needed. > > > >=20 > > > > There is pv_kick_node vs pv_wait_head_or_lock > > > >=20 > > > > [w] l->locked =3D _Q_SLOW_VAL //reordered here > > > >=20 > > > > if (READ_ONCE(pn->state) =3D=3D vcpu_hashed) //False. > > > >=20 > > > > lp =3D (struct qspinlock **)1; > > > >=20 > > > > [STORE] pn->state =3D vcpu_hashed lp =3D pv_= hash(lock, > > > > pn); > > > > pv_hash() = if > > > > (xchg(&l->locked, _Q_SLOW_VAL) =3D=3D 0) // fasle, not unhashed. > > > >=20 > > >=20 > > > This analysis is correct, but.. > > >=20 > >=20 > > Hmm.. look at this again, I don't think this analysis is meaningful, > > let's say the reordering didn't happen, we still got(similar to your > > case): > >=20 > but there is > cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running); >=20 > > if (READ_ONCE(pn->state) =3D=3D vcpu_hashed) // false. > > lp =3D (struct qspinlock **)1; > >=20 > > cmpxchg(pn->state, vcpu_halted, vcpu_hashed); > this cmpxchg will observe the cmpxchg_relaxed above, so this cmpxchg will= fail as pn->state is vcpu_running. > No bug here.. >=20 And we got the same guarantee if we use cmpxchg_release(), no? Regards, Boqun --IMjqdzrDRly81ofr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAliaxYcACgkQSXnow7UH +rhJoQf/UXg/6lKUGsQ8mzojsBgfQRK/wueEMfMvQnLsInpOVXqU7DcoyeMRjo+/ bIkLiTubNtbouWioZXeN3ywgQsA8fQvj59+2uUttRysSMZepeAcvE7L+O4Kf8WGY TBeW5bDy9+ySZx8bql+vRLyhUh5Ta8TV21B5uc/kwzGK8VB5FPwcuSK2ORLCDlki zUF/BSDAYlINEwm0dWQhKKi1jfQ7aXSSFK5fXb38d/+XRaz59C+Auw2y0u1WhzZg RZ21oPNfcB5DOptmlEizZCXk82iUjmFgCEauBq58crJt780mwlrZHCaM4kfgAv1Y SXrShPl8xmw2HIyP4e95vYyFGLG0pw== =Yp1B -----END PGP SIGNATURE----- --IMjqdzrDRly81ofr--