linux-kernel - Re: [GIT PULL rcu/next] RCU commits for 4.13

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1706281123240.15338-100000@netrider.rowland.org>
Date:   Wed, 28 Jun 2017 11:31:55 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrea Parri <parri.andrea@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        <priyalee.kushwaha@...el.com>, <drozdziak1@...il.com>,
        Arnd Bergmann <arnd@...db.de>, <ldr709@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Triplett <josh@...htriplett.org>,
        Nicolas Pitre <nico@...aro.org>,
        Krister Johansen <kjlx@...pleofstupid.com>,
        Vegard Nossum <vegard.nossum@...cle.com>, <dcb314@...mail.com>,
        Wu Fengguang <fengguang.wu@...el.com>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Rik van Riel <riel@...hat.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>,
        Luc Maranget <luc.maranget@...ia.fr>,
        Jade Alglave <j.alglave@....ac.uk>
Subject: Re: [GIT PULL rcu/next] RCU commits for 4.13

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
> > <paulmck@...ux.vnet.ibm.com> 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