From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754817AbcHVJPr (ORCPT ); Mon, 22 Aug 2016 05:15:47 -0400 Received: from mail-it0-f52.google.com ([209.85.214.52]:37166 "EHLO mail-it0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751754AbcHVJPq (ORCPT ); Mon, 22 Aug 2016 05:15:46 -0400 Date: Mon, 22 Aug 2016 17:15:41 +0800 From: Boqun Feng To: Manfred Spraul Cc: Davidlohr Bueso , 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: <20160822091541.GA28226@tardis.cn.ibm.com> References: <1470787537.3015.83.camel@kernel.crashing.org> <4bd34301-0c63-66ae-71b1-6fd68c9fecdd@colorfullife.com> <20160810191757.GA4952@linux-80c1.suse> <20160812024712.GA31159@tardis.cn.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mP3DRpeJDSE+ciuQ" Content-Disposition: inline In-Reply-To: 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 --mP3DRpeJDSE+ciuQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 12, 2016 at 08:43:55PM +0200, Manfred Spraul wrote: > Hi Boqun, >=20 > On 08/12/2016 04:47 AM, Boqun Feng wrote: > > > We should not be doing an smp_mb() right after a spin_lock(), makes n= o sense. The > > > spinlock machinery should guarantee us the barriers in the unorthodox= locking cases, > > > such as this. > > >=20 > Do we really want to go there? > Trying to handle all unorthodox cases will end up as an endless list of > patches, and guaranteed to be stale architectures. >=20 To be honest, the only unorthodox case here is we try to use spin_unlock_wait() to achieve some kind of ordering with a lock critical section, like we do in sem code and do_exit(). Spinlocks are mostly used for exclusive lock critical sections, which are what we care about most and what we should make as fast as possible. > > Right. > >=20 > > If you have: > >=20 > > 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()") > >=20 > > you don't need smp_mb() after spin_lock() on PPC. > >=20 > > And, IIUC, if you have: > >=20 > > 3a5facd09da8 ("arm64: spinlock: fix spin_unlock_wait for LSE atomics") > > d86b8da04dfa ("arm64: spinlock: serialise spin_unlock_wait against > > concurrent lockers") > >=20 > > you don't need smp_mb() after spin_lock() on ARM64. > >=20 > > And, IIUC, if you have: > >=20 > > 2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more") > >=20 > > you don't need smp_mb() after spin_lock() on x86 with qspinlock. >=20 > I would really prefer the other approach: > - spin_lock() is an acquire, that's it. No further guarantees, e.g. order= ing > of writing the lock. > - spin_unlock() is a release, that's it. No objection to these. That's how we implement and document locks now. > - generic smp_mb__after_before_whatever(). And architectures can override > the helpers. > E.g. if qspinlocks on x86 can implement the smp_mb__after_spin_lock() for > free, then the helper can be a nop. >=20 If you are going to use smp_mb__after_before_whatever() to fix the spin_unlock_wait() problem, the helper for qspinlocks on x86 can not be free, because qspinlocks on x86 use separate READs and WRITEs in spin_lock(), and we need a full barrier to order the WRITEs of the lock acquisition with READs in critical sections to let spin_unlock_wait() work. > Right now, we start to hardcode something into the architectures - for so= me > callers. > Other callers use solutions such as smp_mb__after_unlock_lock(), i.e. arch > dependent workarounds in arch independent code. >=20 Please note that fixes above in spin_unlock_wait() and smp_mb__after_unlock_lock() are for two different problems, smp_mb__after_unlock_lock() is to upgrade a lock+unlock pair into a full barrier, which is useful to RCpc archs, e.g. PowerPC. > And: We unnecessarily add overhead. > Both ipc/sem and netfilter do loops over many spinlocks: > > for (i =3D 0; i < CONNTRACK_LOCKS; i++) { > > spin_unlock_wait(&nf_conntrack_locks[i]); > > } > One memory barrier would be sufficient, but due to embedding we end up wi= th > CONNTRACK_LOCKS barriers. >=20 We can move the smp_mb() out of the spin_unlock_wait(), if we make sure all the users are using the proper barriers, so this overhead is easily fixed. > Should I create a patch? > (i.e. documentation and generic helpers) >=20 There have been some discussions in this thread: http://marc.info/?l=3Dlinux-arm-kernel&m=3D144862480822027 , which may be helpful ;-) Regards, Boqun > -- > Manfred --mP3DRpeJDSE+ciuQ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJXusK4AAoJEEl56MO1B/q4adwH/3Q+1YWHg5pEDeMMbdBIZwuj bSD7kbghyIDvUzSJnkzWpr7jqyh43HYYPbBJ1QrwteoPsNSpBLWO0nffxAOin/JS z+3OyiJpgqZNkkG1HQDiRnNI0398sBJLrxZTUM3Jp5ol8D3DEW2jLeeSQsc0VfQR mu/CMc8gaN6BV6rpL2BMcQEGn78gT6NsykSOoy1YlXibaEBq10F2gm5H8t3z3jFm 60FTdahqeua78xIeWURZrP5LoR/iFIUF7O9FhlZ5qzR98FEE4M4VZrOuF35+8eaL 6ZgzLAEOWdKQ/cIh++XXahsFqyZiUd10O2DNuiNsQL6KmpRHnBI59Lg19xv3e4s= =jErR -----END PGP SIGNATURE----- --mP3DRpeJDSE+ciuQ--