From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753292AbdBHEKj (ORCPT ); Tue, 7 Feb 2017 23:10:39 -0500 Received: from mail-pg0-f67.google.com ([74.125.83.67]:36608 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751532AbdBHEKi (ORCPT ); Tue, 7 Feb 2017 23:10:38 -0500 Date: Wed, 8 Feb 2017 12:05:40 +0800 From: Boqun Feng To: Xinhui Pan Cc: Waiman Long , 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: <20170208040540.GB9178@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="wq9mPyueHGvFACwf" Content-Disposition: inline In-Reply-To: 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 --wq9mPyueHGvFACwf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 by more > > relaxed versions to improve performance on architectures that use LL/SC. > > > > All the locking related cmpxchg's are replaced with the _acquire > > variants: > > - pv_queued_spin_steal_lock() > > - trylock_clear_pending() > > > > The cmpxchg's related to hashing are replaced by either by the _release > > or the _relaxed variants. See the inline comment for details. > > > > Signed-off-by: Waiman Long > > > > v1->v2: > > - Add comments in changelog and code for the rationale of the change. > > > > --- > > kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++------ > > ------- > > 1 file changed, 33 insertions(+), 17 deletions(-) > > > > > > @@ -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 al= so > > 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. Th= ere > > is > > + * no other ordering requirement. > > */ > > - cmpxchg(&pn->state, vcpu_halted, vcpu_running); > > + cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running); > > > > /* > > * 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, > > struct 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_hal= ted) > > + if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed) > > + !=3D vcpu_halted) > > return; > > > > 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(l= ock, > pn); > pv_hash() = if > (xchg(&l->locked, _Q_SLOW_VAL) =3D=3D 0) // fasle, not unhashed. >=20 This analysis is correct, but.. > Then the same lock has hashed twice but only unhashed once. So at last as > the hash table grows big, we hit RCU stall. >=20 > I hit RCU stall when I run netperf benchmark >=20 how will a big hash table hit RCU stall? Do you have the call trace for your RCU stall? Regards, Boqun > thanks > xinhui >=20 >=20 > > -- > > 1.8.3.1 > > > > --wq9mPyueHGvFACwf Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAliamRAACgkQSXnow7UH +rhh+wf9HAlnuQ6UWWsxyoCA91tDVabcmU8+Zl1Fm0EH04LYiJUIrGxOEZ93ZBXO jhqdGgxo6AznWIByPoZquB56wJBsg/NyxIEHr6nPRyBWc8yZ/LDAByjUT8QkErYV 0Q6RUBY+yZ3UnJw1z3SIPjv+FXRy5Vw3dKckdolGIXzozXIVMRWM6k9ALTaW5usp 07QuVLG/R0i3xS0iHAy5Kg2QTzWL/23R2BBFH++XBuJpU1MT7ydnebATb37sNm4j wNGSWqrRQ+kcFHVV30wk17EX8d4o/9Qn9GbNNRtWbd4j0yqouvO6r1VLlUg46/Gh a2X5Z6KZ0hRfkJ8MP0Ka4jtnWjh/Kg== =OnDZ -----END PGP SIGNATURE----- --wq9mPyueHGvFACwf--