Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751547AbdF1PcC (ORCPT ); Wed, 28 Jun 2017 11:32:02 -0400 Received: from netrider.rowland.org ([192.131.102.5]:51559 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751511AbdF1Pb5 (ORCPT ); Wed, 28 Jun 2017 11:31:57 -0400 Date: Wed, 28 Jun 2017 11:31:55 -0400 (EDT) From: Alan Stern X-X-Sender: stern@netrider.rowland.org To: "Paul E. McKenney" cc: Linus Torvalds , Andrea Parri , Linux Kernel Mailing List , , , Arnd Bergmann , , Thomas Gleixner , Peter Zijlstra , Josh Triplett , Nicolas Pitre , Krister Johansen , Vegard Nossum , , Wu Fengguang , Frederic Weisbecker , Rik van Riel , Steven Rostedt , Ingo Molnar , Luc Maranget , Jade Alglave Subject: Re: [GIT PULL rcu/next] RCU commits for 4.13 In-Reply-To: <20170627233748.GZ3721@linux.vnet.ibm.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3578 Lines: 81 On Tue, 27 Jun 2017, Paul E. McKenney wrote: > On Tue, Jun 27, 2017 at 02:48:18PM -0700, Linus Torvalds wrote: > > On Tue, Jun 27, 2017 at 1:58 PM, Paul E. McKenney > > wrote: > > > > > > So what next? > > > > > > One option would be to weaken the definition of spin_unlock_wait() so > > > that it had acquire semantics but not release semantics. Alternatively, > > > we could keep the full empty-critical-section semantics and add memory > > > barriers to spin_unlock_wait() implementations, perhaps as shown in the > > > patch below. I could go either way, though I do have some preference > > > for the stronger semantics. > > > > > > Thoughts? > > > > I would prefer to just say > > > > - document that spin_unlock_wait() has acquire semantics > > > > - mindlessly add the smp_mb() to all users > > > > - let users then decide if they are ok with just acquire > > > > That's partly because I think we actually have *fewer* users than we > > have implementations of spin_unlock_wait(). So adding a few smp_mb()'s > > in the users is actually likely the smaller change. > > You are right about that! There are only five invocations of > spin_unlock_wait() in the kernel, with a sixth that has since been > converted to spin_lock() immediately followed by spin_unlock(). > > > But it's also because then that allows people who *can* say that > > acquire is sufficient to just use it. People who use > > spin_unlock_wait() tend to have some odd performance reason to do so, > > so I think allowing them to use the more light-weight memory ordering > > if it works for them is a good idea. > > > > But finally, it's partly because I think "acquire" semantics are > > actually the saner ones that we can explain the logic for much more > > clearly. > > > > Basically, acquire semantics means that you are guaranteed to see any > > changes that were done inside a previously locked region. > > > > Doesn't that sound like sensible semantics? > > It is the semantics that most implementations of spin_unlock_wait() > provide. Of the six invocations, two of them very clearly rely > only on the acquire semantics and two others already have the needed > memory barriers in place. I have queued one patch to add smp_mb() > to the remaining spin_unlock_wait() of the surviving five instances, > and another patch to convert the spin_lock/unlock pair to smp_mb() > followed by spin_unlock_wait(). > > So, yes, it is a sensible set of semantics. At this point, agreeing > -any- reasonable semantics would be good, as it would allow us to get > locking added to the prototype Linux-kernel memory model. ;-) > > > Then, the argument for "smp_mb()" (before the spin_unlock_wait()) becomes: > > > > - I did a write that will affect any future lock takes > > > > - the smp_mb() now means that that write will be ordered wrt the > > acquire that guarantees we've seen all old actions taken by a lock. > > > > Does those kinds of semantics make sense to people? The problem is that adding smp_mb() before spin_unlock_wait() does not provide release semantics, as Andrea has pointed out. ISTM that when people want full release & acquire semantics, they should just use "spin_lock(); spin_unlock();". If there are any places where this would add unacceptable overhead, maybe those places need some rethinking. For instance, perhaps we could add a separate primitive that provides only release semantics. (But would using the new primitive together with spin_unlock_wait really be significantly better than lock-unlock?) Alan Stern