From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759296AbbKSSCA (ORCPT ); Thu, 19 Nov 2015 13:02:00 -0500 Received: from foss.arm.com ([217.140.101.70]:41536 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759212AbbKSSB5 (ORCPT ); Thu, 19 Nov 2015 13:01:57 -0500 Date: Thu, 19 Nov 2015 18:01:52 +0000 From: Will Deacon To: "Paul E. McKenney" Cc: Linus Torvalds , Peter Zijlstra , Boqun Feng , Oleg Nesterov , Ingo Molnar , Linux Kernel Mailing List , Jonathan Corbet , Michal Hocko , David Howells , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras Subject: Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Message-ID: <20151119180151.GF1616@arm.com> References: <20151111193953.GA23515@redhat.com> <20151112070915.GC6314@fixme-laptop.cn.ibm.com> <20151116155658.GW17308@twins.programming.kicks-ass.net> <20151116160445.GK11639@twins.programming.kicks-ass.net> <20151116162452.GD1999@arm.com> <20151117115109.GD28649@arm.com> <20151117210109.GY5184@linux.vnet.ibm.com> <20151118112514.GC1588@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151118112514.GC1588@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 18, 2015 at 11:25:14AM +0000, Will Deacon wrote: > On Tue, Nov 17, 2015 at 01:01:09PM -0800, Paul E. McKenney wrote: > > On Tue, Nov 17, 2015 at 11:51:10AM +0000, Will Deacon wrote: > > > On Mon, Nov 16, 2015 at 01:58:49PM -0800, Linus Torvalds wrote: > > > > On Mon, Nov 16, 2015 at 8:24 AM, Will Deacon wrote: > > > > > > > > > > ... or we upgrade spin_unlock_wait to a LOCK operation, which might be > > > > > slightly cheaper than spin_lock()+spin_unlock(). > > > > > > > > So traditionally the real concern has been the cacheline ping-pong > > > > part of spin_unlock_wait(). I think adding a memory barrier (that > > > > doesn't force any exclusive states, just ordering) to it is fine, but > > > > I don't think we want to necessarily have it have to get the cacheline > > > > into exclusive state. > > > > > > The problem is, I don't think the memory-barrier buys you anything in > > > the context of Boqun's example. In fact, he already had smp_mb() either > > > side of the spin_unlock_wait() and its still broken on arm64 and ppc. > > > > > > Paul is proposing adding a memory barrier after spin_lock() in the racing > > > thread, but I personally think people will forget to add that. > > > > A mechanical check would certainly make me feel better about it, so that > > any lock that was passed to spin_unlock_wait() was required to have all > > acquisitions followed by smp_mb__after_unlock_lock() or some such. > > But I haven't yet given up on finding a better solution. > > Right-o. I'll hack together the arm64 spin_unlock_wait fix, but hold off > merging it for a few weeks in case we get struck by a sudden flash of > inspiration. For completeness, here's what I've currently got. I've failed to measure any performance impact on my 8-core systems, but that's not surprising. Will --->8 >>From da14adc1aef2f12b7a7def4d6b7dde254a91ebf1 Mon Sep 17 00:00:00 2001 From: Will Deacon Date: Thu, 19 Nov 2015 17:48:31 +0000 Subject: [PATCH] arm64: spinlock: serialise spin_unlock_wait against concurrent lockers Boqun Feng reported a rather nasty ordering issue with spin_unlock_wait on architectures implementing spin_lock with LL/SC sequences and acquire semantics: | CPU 1 CPU 2 CPU 3 | ================== ==================== ============== | spin_unlock(&lock); | spin_lock(&lock): | r1 = *lock; // r1 == 0; | o = READ_ONCE(object); // reordered here | object = NULL; | smp_mb(); | spin_unlock_wait(&lock); | *lock = 1; | smp_mb(); | o->dead = true; | if (o) // true | BUG_ON(o->dead); // true!! The crux of the problem is that spin_unlock_wait(&lock) can return on CPU 1 whilst CPU 2 is in the process of taking the lock. This can be resolved by upgrading spin_unlock_wait to a LOCK operation, forcing it to serialise against a concurrent locker and giving it acquire semantics in the process (although it is not at all clear whether this is needed - different callers seem to assume different things about the barrier semantics and architectures are similarly disjoint in their implementations of the macro). This patch implements spin_unlock_wait using an LL/SC sequence with acquire semantics on arm64. For v8.1 systems with the LSE atomics, the exclusive writeback is omitted, since the spin_lock operation is indivisible and no intermediate state can be observed. Signed-off-by: Will Deacon --- arch/arm64/include/asm/spinlock.h | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h index c85e96d174a5..b531791a75ff 100644 --- a/arch/arm64/include/asm/spinlock.h +++ b/arch/arm64/include/asm/spinlock.h @@ -26,9 +26,29 @@ * The memory barriers are implicit with the load-acquire and store-release * instructions. */ +static inline void arch_spin_unlock_wait(arch_spinlock_t *lock) +{ + unsigned int tmp; + arch_spinlock_t lockval; -#define arch_spin_unlock_wait(lock) \ - do { while (arch_spin_is_locked(lock)) cpu_relax(); } while (0) + asm volatile( +" sevl\n" +"1: wfe\n" +"2: ldaxr %w0, %2\n" +" eor %w1, %w0, %w0, ror #16\n" +" cbnz %w1, 1b\n" + ARM64_LSE_ATOMIC_INSN( + /* LL/SC */ +" stxr %w1, %w0, %2\n" + /* Serialise against any concurrent lockers */ +" cbnz %w1, 2b\n", + /* LSE atomics */ +" nop\n" +" nop\n") + : "=&r" (lockval), "=&r" (tmp), "+Q" (*lock) + : + : "memory"); +} #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock) -- 2.1.4