From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752180AbcHLCr2 (ORCPT ); Thu, 11 Aug 2016 22:47:28 -0400 Received: from mail-io0-f194.google.com ([209.85.223.194]:34006 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751264AbcHLCr0 (ORCPT ); Thu, 11 Aug 2016 22:47:26 -0400 Date: Fri, 12 Aug 2016 10:47:12 +0800 From: Boqun Feng To: Davidlohr Bueso Cc: Manfred Spraul , Benjamin Herrenschmidt , Michael Ellerman , Andrew Morton , Linux Kernel Mailing List , Susanne Spraul <1vier1@web.de>, "Paul E. McKenney" , Peter Zijlstra Subject: Re: spin_lock implicit/explicit memory barrier Message-ID: <20160812024712.GA31159@tardis.cn.ibm.com> References: <1470787537.3015.83.camel@kernel.crashing.org> <4bd34301-0c63-66ae-71b1-6fd68c9fecdd@colorfullife.com> <20160810191757.GA4952@linux-80c1.suse> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="7AUc2qLy4jB3hD7Z" Content-Disposition: inline In-Reply-To: <20160810191757.GA4952@linux-80c1.suse> User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --7AUc2qLy4jB3hD7Z Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 10, 2016 at 12:17:57PM -0700, Davidlohr Bueso wrote: > On Wed, 10 Aug 2016, Manfred Spraul wrote: >=20 > > On 08/10/2016 02:05 AM, Benjamin Herrenschmidt wrote: > > > On Tue, 2016-08-09 at 20:52 +0200, Manfred Spraul wrote: > > > > Hi Benjamin, Hi Michael, > > > >=20 > > > > regarding commit 51d7d5205d33 ("powerpc: Add smp_mb() to > > > > arch_spin_is_locked()"): > > > >=20 > > > > For the ipc/sem code, I would like to replace the spin_is_locked() = with > > > > a smp_load_acquire(), see: > > > >=20 > > > > http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n367 > > > >=20 > > > > http://www.ozlabs.org/~akpm/mmots/broken-out/ipc-semc-fix-complex_c= ount-vs-simple-op-race.patch > > > >=20 > > > > To my understanding, I must now add a smp_mb(), otherwise it would = be > > > > broken on PowerPC: > > > >=20 > > > > The approach that the memory barrier is added into spin_is_locked() > > > > doesn't work because the code doesn't use spin_is_locked(). > > > >=20 > > > > Correct? > > > Right, otherwise you aren't properly ordered. The current powerpc loc= ks provide > > > good protection between what's inside vs. what's outside the lock but= not vs. > > > the lock *value* itself, so if, like you do in the sem code, use the = lock > > > value as something that is relevant in term of ordering, you probably= need > > > an explicit full barrier. >=20 > But the problem here is with spin_unlock_wait() (for ll/sc spin_lock) not= seeing the > store that makes the lock visibly taken and both threads end up exiting o= ut of sem_lock(); > similar scenario to the spin_is_locked commit mentioned above, which is c= rossing of > locks. >=20 > Now that spin_unlock_wait() always implies at least an load-acquire barri= er (for both > ticket and qspinlocks, which is still x86 only), we wait on the full crit= ical region. >=20 > So this patch takes this locking scheme: >=20 > CPU0 CPU1 > spin_lock(l) spin_lock(L) > spin_unlock_wait(L) if (spin_is_locked(l)) > foo() foo() >=20 > ... and converts it now to: >=20 > CPU0 CPU1 > complex_mode =3D true spin_lock(l) > smp_mb() <--- do we want a smp_mb() here? > spin_unlock_wait(l) if (!smp_load_acquire(complex_mode)) > foo() foo() >=20 > We should not be doing an smp_mb() right after a spin_lock(), makes no se= nse. The > spinlock machinery should guarantee us the barriers in the unorthodox loc= king cases, > such as this. >=20 Right. If you have: 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()") you don't need smp_mb() after spin_lock() on PPC. And, IIUC, if you have: 3a5facd09da8 ("arm64: spinlock: fix spin_unlock_wait for LSE atomics") d86b8da04dfa ("arm64: spinlock: serialise spin_unlock_wait against concurrent lockers") you don't need smp_mb() after spin_lock() on ARM64. And, IIUC, if you have: 2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more") you don't need smp_mb() after spin_lock() on x86 with qspinlock. Regards, Boqun > Thanks, > Davidlohr --7AUc2qLy4jB3hD7Z Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJXrTitAAoJEEl56MO1B/q4jRQH/RBnwS4R78zqlRsN9KrHjTMQ pvH+j1HPuP2+e10LsheL6REUuo/2K86ybmYerG2cEvJFPMU21SaslQK32hD09ujz nfIpbJYhLsxOeg5KIdCYbC/qSoaTwqn261UxcThDyuagFO8CfRTiepOTnmIzcD0L HuR6dcao33eSK8SrqjXwnoK8+Dq8Gyt+xEgpmtAF0BRuAPFSkDGJBGy4GkfRUBKU 342AfC3KY3dPeETQcvaOnm+Hq/j/0qqzm1SSWHMPrC6pHJgsfae804yFcW18P6TF p5Cm5p5BQ53n0K0IhAT1zGrBG6/dxFvRupR/AS9AQjHmTDsoFNNoW5AcvUrm5Fk= =Ysgf -----END PGP SIGNATURE----- --7AUc2qLy4jB3hD7Z--