From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933885AbcHJSVb (ORCPT ); Wed, 10 Aug 2016 14:21:31 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35610 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934004AbcHJSV2 (ORCPT ); Wed, 10 Aug 2016 14:21:28 -0400 Subject: Re: spin_lock implicit/explicit memory barrier To: Benjamin Herrenschmidt , Michael Ellerman References: <1470787537.3015.83.camel@kernel.crashing.org> Cc: Andrew Morton , Davidlohr Bueso , Linux Kernel Mailing List , Susanne Spraul <1vier1@web.de>, "Paul E. McKenney" , Peter Zijlstra From: Manfred Spraul Message-ID: <4bd34301-0c63-66ae-71b1-6fd68c9fecdd@colorfullife.com> Date: Wed, 10 Aug 2016 20:21:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <1470787537.3015.83.camel@kernel.crashing.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, [adding Peter, correcting Davidlohr's mail address] 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, >> >> regarding commit 51d7d5205d33 ("powerpc: Add smp_mb() to >> arch_spin_is_locked()"): >> >> For the ipc/sem code, I would like to replace the spin_is_locked() with >> a smp_load_acquire(), see: >> >> http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n367 >> >> http://www.ozlabs.org/~akpm/mmots/broken-out/ipc-semc-fix-complex_count-vs-simple-op-race.patch >> >> To my understanding, I must now add a smp_mb(), otherwise it would be >> broken on PowerPC: >> >> The approach that the memory barrier is added into spin_is_locked() >> doesn't work because the code doesn't use spin_is_locked(). >> >> Correct? > Right, otherwise you aren't properly ordered. The current powerpc locks 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. > > Adding Paul McKenney. Just to be safe, let's write down all barrier pairs: entry and exit, simple and complex, and switching simple to complex and vice versa. (@Davidlohr: Could you crosscheck, did I overlook a pair?) 1) spin_lock/spin_unlock pair. 2) ||smp_load_acquire(&sma->complex_mode) and |||smp_store_release(sma->complex_mode, true) pair. ||http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n374 http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n321 The store_release guarantees that all data written by the complex_op syscall is - after the load_acquire - visible by the simple_op syscall. 3) smp_mb() [after spin_lock()] and |||smp_store_mb(sma->complex_mode, true) pair. |http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n287 http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n372 This are actually two pairs: - Writing the lock variable must observed by the task that does spin_unlock_wait() - complex_mode must be observed by the task that does the smp_load_acquire() 4) spin_unlock_wait() and spin_unlock() pair http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291 http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n409 The data from the simple op must be observed by the following complex op. Right now, there is still an smp_rmb() in line 300: The control barrier from the loop inside spin_unlock_wait() is upgraded to an acquire barrier by an additional smp_rmb(). Is this smp_rmb() required? If I understand commit 2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more") right, with this commit qspinlock handle this case without the smp_rmb(). What I don't know if powerpc is using qspinlock already, or if powerpc works without the smp_rmb(). -- Manfred|