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 > > <[email protected]> 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
