From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933497AbcH2Noc (ORCPT ); Mon, 29 Aug 2016 09:44:32 -0400 Received: from merlin.infradead.org ([205.233.59.134]:40320 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932908AbcH2Nob (ORCPT ); Mon, 29 Aug 2016 09:44:31 -0400 Date: Mon, 29 Aug 2016 15:44:24 +0200 From: Peter Zijlstra To: Manfred Spraul Cc: benh@kernel.crashing.org, paulmck@linux.vnet.ibm.com, Ingo Molnar , Boqun Feng , Andrew Morton , LKML , 1vier1@web.de, Davidlohr Bueso Subject: Re: [PATCH 1/4] spinlock: Document memory barrier rules Message-ID: <20160829134424.GS10153@twins.programming.kicks-ass.net> References: <1472385376-8801-1-git-send-email-manfred@colorfullife.com> <1472385376-8801-2-git-send-email-manfred@colorfullife.com> <20160829104815.GI10153@twins.programming.kicks-ass.net> <968e4c62-4486-a6aa-8fdf-67ff9b05a330@colorfullife.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <968e4c62-4486-a6aa-8fdf-67ff9b05a330@colorfullife.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 29, 2016 at 02:54:54PM +0200, Manfred Spraul wrote: > Hi Peter, > > On 08/29/2016 12:48 PM, Peter Zijlstra wrote: > >On Sun, Aug 28, 2016 at 01:56:13PM +0200, Manfred Spraul wrote: > >>Right now, the spinlock machinery tries to guarantee barriers even for > >>unorthodox locking cases, which ends up as a constant stream of updates > >>as the architectures try to support new unorthodox ideas. > >> > >>The patch proposes to reverse that: > >>spin_lock is ACQUIRE, spin_unlock is RELEASE. > >>spin_unlock_wait is also ACQUIRE. > >>Code that needs further guarantees must use appropriate explicit barriers. > >> > >>Architectures that can implement some barriers for free can define the > >>barriers as NOPs. > >> > >>As the initial step, the patch converts ipc/sem.c to the new defines: > >>- no more smp_rmb() after spin_unlock_wait(), that is part of > >> spin_unlock_wait() > >>- smp_mb__after_spin_lock() instead of a direct smp_mb(). > >> > >Why? This does not explain why.. > > Which explanation is missing? > > - removal of the smb_rmb() after spin_unlock_wait? So that should have been a separate patch. This thing doing two things is wrong too. But no, this I get. I did make spin_unlock_wait() an ACQUIRE after all. > - Why smp_mb is required after spin_lock? See Patch 02, I added the race > that exists on real hardware. > > Exactly the same issue exists for sem.c > > - Why introduce a smp_mb__after_spin_lock()? > > The other options would be: > - same as RCU, i.e. add CONFIG_PPC into sem.c and nf_contrack_core.c > - overhead for all archs by added an unconditional smp_mb() See, this too doesn't adequately explain the situation, since all refers to other sources. If you add a barrier, the Changelog had better be clear. And I'm still not entirely sure I get what exactly this barrier should do, nor why it defaults to a full smp_mb. If what I suspect it should do, only PPC and ARM64 need the barrier. And x86 doesn't need it -- _however_ it would need it if you require full smp_mb semantics, which I suspect you don't. Which brings us back to a very poor definition of what this barrier should be doing.