From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752966AbcHJVAI (ORCPT ); Wed, 10 Aug 2016 17:00:08 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:50180 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752426AbcHJVAG (ORCPT ); Wed, 10 Aug 2016 17:00:06 -0400 X-IBM-Helo: d03dlp01.boulder.ibm.com X-IBM-MailFrom: paulmck@linux.vnet.ibm.com Date: Wed, 10 Aug 2016 14:00:03 -0700 From: "Paul E. McKenney" To: Davidlohr Bueso Cc: Manfred Spraul , Benjamin Herrenschmidt , Michael Ellerman , Andrew Morton , Linux Kernel Mailing List , Susanne Spraul <1vier1@web.de>, Peter Zijlstra Subject: Re: spin_lock implicit/explicit memory barrier Reply-To: paulmck@linux.vnet.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: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160810191757.GA4952@linux-80c1.suse> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16081021-0016-0000-0000-00000464263F X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00005574; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000180; SDB=6.00742856; UDB=6.00349726; IPR=6.00515413; BA=6.00004655; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00012303; XFM=3.00000011; UTC=2016-08-10 21:00:02 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16081021-0017-0000-0000-000031E6EEC2 Message-Id: <20160810210003.GM3482@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-08-10_17:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1604210000 definitions=main-1608100219 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 10, 2016 at 12:17:57PM -0700, Davidlohr Bueso wrote: > On Wed, 10 Aug 2016, Manfred Spraul wrote: > > >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. > > 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 out of sem_lock(); > similar scenario to the spin_is_locked commit mentioned above, which is crossing of > locks. > > Now that spin_unlock_wait() always implies at least an load-acquire barrier (for both > ticket and qspinlocks, which is still x86 only), we wait on the full critical region. > > So this patch takes this locking scheme: > > CPU0 CPU1 > spin_lock(l) spin_lock(L) > spin_unlock_wait(L) if (spin_is_locked(l)) > foo() foo() > > ... and converts it now to: > > CPU0 CPU1 > complex_mode = 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() > > We should not be doing an smp_mb() right after a spin_lock(), makes no sense. The > spinlock machinery should guarantee us the barriers in the unorthodox locking cases, > such as this. In this case, from what I can see, we do need a store-load fence. That said, yes, it really should be smp_mb__after_unlock_lock() rather than smp_mb(). So if this code pattern is both desired and legitimate, the smp_mb__after_unlock_lock() definitions probably need to move out of kernel/rcu/tree.h to barrier.h or some such. Now, I agree that if everyone was acquiring and releasing the lock in standard fashion, there would be no need for memory barriers other than those in the locking primitives. But that is not the case here: A task is looking at some lock-protected state without actually holding the lock. Thanx, Paul