Date: Wed, 28 Jun 2017 17:45:56 -0700
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>,
        Andrea Parri <parri.andrea@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        priyalee.kushwaha@intel.com,
        =?utf-8?Q?Stanis=C5=82aw?= Drozd <drozdziak1@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>, ldr709@gmail.com,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Josh Triplett <josh@joshtriplett.org>, Nicolas Pitre <nico@linaro.org>,
        Krister Johansen <kjlx@templeofstupid.com>,
        Vegard Nossum <vegard.nossum@oracle.com>, dcb314@hotmail.com,
        Wu Fengguang <fengguang.wu@intel.com>,
        Frederic Weisbecker <fweisbec@gmail.com>,
        Rik van Riel <riel@redhat.com>, Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@kernel.org>, Luc Maranget <luc.maranget@inria.fr>,
        Jade Alglave <j.alglave@ucl.ac.uk>
Subject: Re: [GIT PULL rcu/next] RCU commits for 4.13
Reply-To: paulmck@linux.vnet.ibm.com
References: <20170628170321.GQ3721@linux.vnet.ibm.com>
 <Pine.LNX.4.44L0.1706281547270.27696-100000@netrider.rowland.org>
 <20170628235412.GB3721@linux.vnet.ibm.com>
 <CA+55aFwLq5oPvY5HwpKq9LkCuQ5No7O5g=+ij1T68ONu-gOm_Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+55aFwLq5oPvY5HwpKq9LkCuQ5No7O5g=+ij1T68ONu-gOm_Q@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Message-Id: <20170629004556.GD3721@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3571
Lines: 77

On Wed, Jun 28, 2017 at 05:05:46PM -0700, Linus Torvalds wrote:
> On Wed, Jun 28, 2017 at 4:54 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > Linus, are you dead-set against defining spin_unlock_wait() to be
> > spin_lock + spin_unlock?  For example, is the current x86 implementation
> > of spin_unlock_wait() really a non-negotiable hard requirement?  Or
> > would you be willing to live with the spin_lock + spin_unlock semantics?
> 
> So I think the "same as spin_lock + spin_unlock" semantics are kind of insane.
> 
> One of the issues is that the same as "spin_lock + spin_unlock" is
> basically now architecture-dependent. Is it really the
> architecture-dependent ordering you want to define this as?
> 
> So I just think it's a *bad* definition. If somebody wants something
> that is exactly equivalent to spin_lock+spin_unlock, then dammit, just
> do *THAT*. It's completely pointless to me to define
> spin_unlock_wait() in those terms.
> 
> And if it's not equivalent to the *architecture* behavior of
> spin_lock+spin_unlock, then I think it should be descibed in terms
> that aren't about the architecture implementation (so you shouldn't
> describe it as "spin_lock+spin_unlock", you should describe it in
> terms of memory barrier semantics.
> 
> And if we really have to use the spin_lock+spinunlock semantics for
> this, then what is the advantage of spin_unlock_wait at all, if it
> doesn't fundamentally avoid some locking overhead of just taking the
> spinlock in the first place?
> 
> And if we can't use a cheaper model, maybe we should just get rid of
> it entirely?
> 
> Finally: if the memory barrier semantics are exactly the same, and
> it's purely about avoiding some nasty contention case, I think the
> concept is broken - contention is almost never an actual issue, and if
> it is, the problem is much deeper than spin_unlock_wait().

All good points!

I must confess that your sentence about getting rid of spin_unlock_wait()
entirely does resonate with me, especially given the repeated bouts of
"but what -exactly- is it -supposed- to do?" over the past 18 months
or so.  ;-)

Just for completeness, here is a list of the definitions that have been
put forward, just in case it inspires someone to come up with something
better:

1.	spin_unlock_wait() provides only acquire semantics.  Code
	placed after the spin_unlock_wait() will see the effects of
	all previous critical sections, but there is no guarantees for
	subsequent critical sections.  The x86 implementation provides
	this.  I -think- that the ARM and PowerPC implementations could
	get rid of a memory-barrier instruction and still provide this.

2.	As #1 above, but a "smp_mb();spin_unlock_wait();" provides the
	additional guarantee that code placed before this construct is
	seen by all subsequent critical sections.  The x86 implementation
	provides this, as do ARM and PowerPC, but it is not clear that all
	architectures do.  As Alan noted, this is an extremely unnatural
	definition for the current memory model.

3.	[ Just for completeness, yes, this is off the table! ]  The
	spin_unlock_wait() has the same semantics as a spin_lock()
	followed immediately by a spin_unlock().

4.	spin_unlock_wait() is analogous to synchronize_rcu(), where
	spin_unlock_wait()'s "read-side critical sections" are the lock's
	normal critical sections.  This was the first definition I heard
	that made any sense to me, but it turns out to be equivalent
	to #3.	Thus, also off the table.

Does anyone know of any other possible definitions?

							Thanx, Paul