From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754682AbcLZFvI (ORCPT ); Mon, 26 Dec 2016 00:51:08 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:33354 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752421AbcLZFvG (ORCPT ); Mon, 26 Dec 2016 00:51:06 -0500 Date: Mon, 26 Dec 2016 13:50:02 +0800 From: Boqun Feng To: Waiman Long Cc: Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, Pan Xinhui Subject: Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs Message-ID: <20161226055002.GA8717@tardis.cn.ibm.com> References: <1482697561-23848-1-git-send-email-longman@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="45Z9DzgjV8m4Oswq" Content-Disposition: inline In-Reply-To: <1482697561-23848-1-git-send-email-longman@redhat.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 --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Wainman, On Sun, Dec 25, 2016 at 03:26:01PM -0500, Waiman Long wrote: > A number of cmpxchg calls in qspinlock_paravirt.h were replaced by 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 _release > 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 change. >=20 > --- > kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++-------= ------ > 1 file changed, 33 insertions(+), 17 deletions(-) >=20 > diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlo= ck_paravirt.h > index e3b5520..c31d1ab 100644 > --- a/kernel/locking/qspinlock_paravirt.h > +++ b/kernel/locking/qspinlock_paravirt.h > @@ -72,7 +72,7 @@ static inline bool pv_queued_spin_steal_lock(struct qsp= inlock *lock) > struct __qspinlock *l =3D (void *)lock; > =20 > if (!(atomic_read(&lock->val) & _Q_LOCKED_PENDING_MASK) && > - (cmpxchg(&l->locked, 0, _Q_LOCKED_VAL) =3D=3D 0)) { > + (cmpxchg_acquire(&l->locked, 0, _Q_LOCKED_VAL) =3D=3D 0)) { > qstat_inc(qstat_pv_lock_stealing, true); > return true; > } > @@ -101,16 +101,16 @@ static __always_inline void clear_pending(struct qs= pinlock *lock) > =20 > /* > * The pending bit check in pv_queued_spin_steal_lock() isn't a memory > - * barrier. Therefore, an atomic cmpxchg() is used to acquire the lock > - * just to be sure that it will get it. > + * barrier. Therefore, an atomic cmpxchg_acquire() is used to acquire the > + * lock to provide the proper memory barrier. > */ > static __always_inline int trylock_clear_pending(struct qspinlock *lock) > { > struct __qspinlock *l =3D (void *)lock; > =20 > return !READ_ONCE(l->locked) && > - (cmpxchg(&l->locked_pending, _Q_PENDING_VAL, _Q_LOCKED_VAL) > - =3D=3D _Q_PENDING_VAL); > + (cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL, > + _Q_LOCKED_VAL) =3D=3D _Q_PENDING_VAL); > } > #else /* _Q_PENDING_BITS =3D=3D 8 */ > static __always_inline void set_pending(struct qspinlock *lock) > @@ -138,7 +138,7 @@ static __always_inline int trylock_clear_pending(stru= ct qspinlock *lock) > */ > old =3D val; > new =3D (val & ~_Q_PENDING_MASK) | _Q_LOCKED_VAL; > - val =3D atomic_cmpxchg(&lock->val, old, new); > + val =3D atomic_cmpxchg_acquire(&lock->val, old, new); > =20 > if (val =3D=3D old) > return 1; > @@ -209,9 +209,15 @@ static struct qspinlock **pv_hash(struct qspinlock *= lock, struct pv_node *node) > struct pv_hash_entry *he; > int hopcnt =3D 0; > =20 > + /* > + * Synchronizing with the node state variable will control who does > + * the hashing - the lock holder or lock waiter. The control > + * dependency will ensure that node value is written after the lock > + * value. So we don't need other ordering guarantee. > + */ By this comment, you mean that =09 cmpxchg_relaxed(&he->lock, NULL, lock); r1 =3D ll he->lock; sc he->lock, lock // successed if (r1) WRITE_ONCE(he->node, node); the sc and WRITE_ONCE() can not be reordered because of the control dependency? I dont think this is true. Yes the sc must execute before the WRITE_ONCE(), but the memory/cache effects may be reordered. IOW, the following may happen 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=3D=3D=3D=3D=3D {x =3D 0, y =3D 0} if (!cmpxchg_relaxed(&y, 0, 1)) WRITE_ONCE(x, 1); r1 =3D READ_ONCE(x); smp_rmb(); r2 =3D READ_ONCE(y); The following result is possible: y =3D 1 && r1 =3D 1 && r2 =3D 0 Or I'm missing your point here? ;-)=20 Regards, Boqun > for_each_hash_entry(he, offset, hash) { > hopcnt++; > - if (!cmpxchg(&he->lock, NULL, lock)) { > + if (!cmpxchg_relaxed(&he->lock, NULL, lock)) { > WRITE_ONCE(he->node, node); > qstat_hop(hopcnt); > return &he->lock; > @@ -309,7 +315,7 @@ static void pv_wait_node(struct mcs_spinlock *node, s= truct mcs_spinlock *prev) > * MB MB > * [L] pn->locked [RmW] pn->state =3D vcpu_hashed > * > - * Matches the cmpxchg() from pv_kick_node(). > + * Matches the cmpxchg_release() from pv_kick_node(). > */ > smp_store_mb(pn->state, vcpu_halted); > =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, retain 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 above 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 variable. There is > + * no other ordering requirement. > */ > - cmpxchg(&pn->state, vcpu_halted, vcpu_running); > + cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running); > =20 > /* > * If the locked flag is still not set after wakeup, it is a > @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *lock, str= uct mcs_spinlock *node) > * pv_wait_node(). If OTOH this fails, the vCPU was running and will > * observe its next->locked value and advance itself. > * > - * Matches with smp_store_mb() and cmpxchg() in pv_wait_node() > + * Matches with smp_store_mb() and cmpxchg_relaxed() in pv_wait_node(). > + * A release barrier is used here to ensure that node->locked is > + * always set before changing the state. See comment in pv_wait_node(). > */ > - if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) !=3D vcpu_halted) > + if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed) > + !=3D vcpu_halted) > return; > =20 > /* > @@ -461,8 +476,8 @@ static void pv_kick_node(struct qspinlock *lock, stru= ct mcs_spinlock *node) > } > =20 > /* > - * The cmpxchg() or xchg() call before coming here provides the > - * acquire semantics for locking. The dummy ORing of _Q_LOCKED_VAL > + * The cmpxchg_acquire() or xchg() call before coming here provides > + * the acquire semantics for locking. The dummy ORing of _Q_LOCKED_VAL > * here is to indicate to the compiler that the value will always > * be nozero to enable better code optimization. > */ > @@ -488,11 +503,12 @@ static void pv_kick_node(struct qspinlock *lock, st= ruct mcs_spinlock *node) > } > =20 > /* > - * A failed cmpxchg doesn't provide any memory-ordering guarantees, > - * so we need a barrier to order the read of the node data in > - * pv_unhash *after* we've read the lock being _Q_SLOW_VAL. > + * A failed cmpxchg_release doesn't provide any memory-ordering > + * guarantees, so we need a barrier to order the read of the node > + * data in pv_unhash *after* we've read the lock being _Q_SLOW_VAL. > * > - * Matches the cmpxchg() in pv_wait_head_or_lock() setting _Q_SLOW_VAL. > + * Matches the cmpxchg_acquire() in pv_wait_head_or_lock() setting > + * _Q_SLOW_VAL. > */ > smp_rmb(); > =20 > --=20 > 1.8.3.1 >=20 --45Z9DzgjV8m4Oswq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAlhgr4QACgkQSXnow7UH +rh0owgAtm5Kfm2tFCSbFEIS3fJTPwamUlYnQjco47Ro/IUb7qhPE4Nn05lxs19c XNbHAHVlSBHmHLCe10QDlIdFRN+nbCLOEHb3m4EHOUBB12/qkDDi7i8Ct7VWS8H0 Ipz0To1KYZxXHvOCia2789WhHoUBGNib1Zp7G58l9+YNnNqTSsZfx3Wh0TzhP/X2 35jEkRLwi0e1XhGCyl1K9eZhMSNOR6BGNglU+xVdfSVXvIjvK1Io/8GjOt0yqhnA 2zqD/YKVpNP3w0aeqqNgrxuqLJ9lEssoyQL/1Ncb4kBdD1owCPTu6I0FJFo+7SNG Oba/HNaCBJY1b2/dgqvBX/z0ZMmGIw== =8NKI -----END PGP SIGNATURE----- --45Z9DzgjV8m4Oswq--