From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755126AbcETOCv (ORCPT ); Fri, 20 May 2016 10:02:51 -0400 Received: from mail-io0-f195.google.com ([209.85.223.195]:34493 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752101AbcETOCt (ORCPT ); Fri, 20 May 2016 10:02:49 -0400 Date: Fri, 20 May 2016 22:05:33 +0800 From: Boqun Feng To: Peter Zijlstra Cc: Davidlohr Bueso , manfred@colorfullife.com, Waiman.Long@hpe.com, mingo@kernel.org, torvalds@linux-foundation.org, ggherdovich@suse.com, mgorman@techsingularity.net, linux-kernel@vger.kernel.org, Paul McKenney , Will Deacon Subject: Re: sem_lock() vs qspinlocks Message-ID: <20160520140533.GA20726@insomnia> References: <20160520053926.GC31084@linux-uzut.site> <20160520115819.GF3193@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="X1bOJ3K7DJ5YkBrT" Content-Disposition: inline In-Reply-To: <20160520115819.GF3193@twins.programming.kicks-ass.net> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --X1bOJ3K7DJ5YkBrT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Peter, On Fri, May 20, 2016 at 01:58:19PM +0200, Peter Zijlstra wrote: > On Thu, May 19, 2016 at 10:39:26PM -0700, Davidlohr Bueso wrote: > > As such, the following restores the behavior of the ticket locks and 'f= ixes' > > (or hides?) the bug in sems. Naturally incorrect approach: > >=20 > > @@ -290,7 +290,8 @@ static void sem_wait_array(struct sem_array *sma) > >=20 > > for (i =3D 0; i < sma->sem_nsems; i++) { > > sem =3D sma->sem_base + i; > > - spin_unlock_wait(&sem->lock); > > + while (atomic_read(&sem->lock)) > > + cpu_relax(); > > } > > ipc_smp_acquire__after_spin_is_unlocked(); > > } >=20 > The actual bug is clear_pending_set_locked() not having acquire > semantics. And the above 'fixes' things because it will observe the old > pending bit or the locked bit, so it doesn't matter if the store > flipping them is delayed. >=20 > The comment in queued_spin_lock_slowpath() above the smp_cond_acquire() > states that that acquire is sufficient, but this is incorrect in the > face of spin_is_locked()/spin_unlock_wait() usage only looking at the > lock byte. >=20 > The problem is that the clear_pending_set_locked() is an unordered > store, therefore this store can be delayed until no later than > spin_unlock() (which orders against it due to the address dependency). >=20 > This opens numerous races; for example: >=20 > ipc_lock_object(&sma->sem_perm); > sem_wait_array(sma); >=20 > false -> spin_is_locked(&sma->sem_perm.lock) >=20 > is entirely possible, because sem_wait_array() consists of pure reads, > so the store can pass all that, even on x86. >=20 > The below 'hack' seems to solve the problem. >=20 > _However_ this also means the atomic_cmpxchg_relaxed() in the locked: > branch is equally wrong -- although not visible on x86. And note that > atomic_cmpxchg_acquire() would not in fact be sufficient either, since > the acquire is on the LOAD not the STORE of the LL/SC. >=20 > I need a break of sorts, because after twisting my head around the sem > code and then the qspinlock code I'm wrecked. I'll try and make a proper > patch if people can indeed confirm my thinking here. >=20 I think your analysis is right, however, the problem only exists if we have the following use pattern, right? 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 spin_lock(A); spin_lock(B); spin_unlock_wait(B); spin_unlock_wait(A); do_something(); do_something(); , which ends up CPU 0 and 1 both running do_something(). And actually this can be simply fixed by add smp_mb() between spin_lock() and spin_unlock_wait() on both CPU, or add an smp_mb() in spin_unlock_wait() as PPC does in 51d7d5205d338 "powerpc: Add smp_mb() to arch_spin_is_locked(= )". So if relaxed/acquire atomics and clear_pending_set_locked() work fine in other situations, a proper fix would be fixing the spin_is_locked()/spin_unlock_wait() or their users? Regards, Boqun > --- > kernel/locking/qspinlock.c | 1 + > 1 file changed, 1 insertion(+) >=20 > diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c > index ce2f75e32ae1..348e172e774f 100644 > --- a/kernel/locking/qspinlock.c > +++ b/kernel/locking/qspinlock.c > @@ -366,6 +366,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock= , u32 val) > * *,1,0 -> *,0,1 > */ > clear_pending_set_locked(lock); > + smp_mb(); > return; > =20 > /* --X1bOJ3K7DJ5YkBrT Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJXPxmpAAoJEEl56MO1B/q4OBsH/1tWoOHbyCXBjuZHgCILdJkx IjlKDjA9fdp2oC9+ibsK5ub/g6K7aBopuVsBap99E0QLw/CIJmlB+9n3RIlQu1Mj 1U6onw3XmU+pEtKZTWz0DPEAL9wpLUjywPXX7vHcI7ttkgmxzsnkK0aoLA2H/7gr Ner2FhFp3AP60wCGDmip82n0aBxGnbn/GvjdeCG7BNPezf9MHbmP1YRX/zuUOOcO x56q8Q9oHFqba9MqKVcUWCUx3v9zkJ6tTKGUHWTtLIdoOTU1yKeWNWyyZT/HqVOP GZNK1TOZ7TFNaLbJocfwuGZIUZ5gA6f6jxO/bvcJVqe4zSqktY6PtI2skl/v0tY= =PnvH -----END PGP SIGNATURE----- --X1bOJ3K7DJ5YkBrT--