From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrea Parri <parri.andrea@gmail.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
priyalee.kushwaha@intel.com, 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>,
Alan Stern <stern@rowland.harvard.edu>,
Luc Maranget <luc.maranget@inria.fr>,
Jade Alglave <j.alglave@ucl.ac.uk>
Subject: Re: [GIT PULL rcu/next] RCU commits for 4.13
Date: Tue, 27 Jun 2017 16:37:48 -0700 [thread overview]
Message-ID: <20170627233748.GZ3721@linux.vnet.ibm.com> (raw)
In-Reply-To: <CA+55aFzbsT_XC6UJaaXrG9okGOZRvRKgj3hQTAEqdAae4E-X0w@mail.gmail.com>
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@linux.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?
In case the answer is "yes", the (untested) patch below (combining
three commits) shows the changes that I believe would be required.
A preview is also available as individual commits on branch
spin_unlock_wait.2017.06.27a on -rcu here:
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
As usual, thoughts? ;-)
Thanx, Paul
------------------------------------------------------------------------
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index ef68232b5222..cc01b77a079a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -704,8 +704,10 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
/* initialize eh_tries */
ap->eh_tries = ATA_EH_MAX_TRIES;
- } else
+ } else {
+ smp_mb(); /* Add release semantics for spin_unlock_wait(). */
spin_unlock_wait(ap->lock);
+ }
}
EXPORT_SYMBOL(ata_scsi_cmd_error_handler);
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index d9510e8522d4..0c3f54e2a1d1 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -373,21 +373,21 @@ static __always_inline int spin_trylock_irq(spinlock_t *lock)
* spin_unlock_wait - Interpose between successive critical sections
* @lock: the spinlock whose critical sections are to be interposed.
*
- * Semantically this is equivalent to a spin_lock() immediately
- * followed by a spin_unlock(). However, most architectures have
- * more efficient implementations in which the spin_unlock_wait()
- * cannot block concurrent lock acquisition, and in some cases
- * where spin_unlock_wait() does not write to the lock variable.
- * Nevertheless, spin_unlock_wait() can have high overhead, so if
- * you feel the need to use it, please check to see if there is
- * a better way to get your job done.
+ * Semantically this is equivalent to a spin_lock() immediately followed
+ * by a mythical spin_unlock() that has no ordering semantics. However,
+ * most architectures have more efficient implementations in which the
+ * spin_unlock_wait() cannot block concurrent lock acquisition, and in some
+ * cases where spin_unlock_wait() does not write to the lock variable.
+ * Nevertheless, spin_unlock_wait() can have high overhead, so if you
+ * feel the need to use it, please check to see if there is a better way
+ * to get your job done.
*
- * The ordering guarantees provided by spin_unlock_wait() are:
- *
- * 1. All accesses preceding the spin_unlock_wait() happen before
- * any accesses in later critical sections for this same lock.
- * 2. All accesses following the spin_unlock_wait() happen after
- * any accesses in earlier critical sections for this same lock.
+ * The spin_unlock_wait() function guarantees that all accesses following
+ * the spin_unlock_wait() happen after any accesses in earlier critical
+ * sections for this same lock. Please note that it does -not- guarantee
+ * that accesses preceding the spin_unlock_wait() happen before any accesses
+ * in later critical sections for this same lock. If you need this latter
+ * ordering, precede the spin_unlock_wait() with an smp_mb() or similar.
*/
static __always_inline void spin_unlock_wait(spinlock_t *lock)
{
diff --git a/ipc/sem.c b/ipc/sem.c
index 947dc2348271..ef42e55e9dd0 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -307,8 +307,8 @@ static void complexmode_enter(struct sem_array *sma)
for (i = 0; i < sma->sem_nsems; i++) {
sem = sma->sem_base + i;
- spin_lock(&sem->lock);
- spin_unlock(&sem->lock);
+ smp_mb(); /* Add release semantics for spin_unlock_wait(). */
+ spin_unlock_wait(&sem->lock);
}
}
next prev parent reply other threads:[~2017-06-27 23:38 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-12 21:37 [GIT PULL rcu/next] RCU commits for 4.13 Paul E. McKenney
2017-06-13 6:41 ` Ingo Molnar
2017-06-14 2:54 ` Andrea Parri
2017-06-14 4:33 ` Paul E. McKenney
2017-06-14 14:33 ` Andrea Parri
2017-06-14 20:23 ` Paul E. McKenney
2017-06-19 16:24 ` Andrea Parri
2017-06-27 20:58 ` Paul E. McKenney
2017-06-27 21:48 ` Linus Torvalds
2017-06-27 23:37 ` Paul E. McKenney [this message]
2017-06-28 15:31 ` Alan Stern
2017-06-28 17:03 ` Paul E. McKenney
2017-06-28 20:16 ` Alan Stern
2017-06-28 23:54 ` Paul E. McKenney
2017-06-29 0:05 ` Linus Torvalds
2017-06-29 0:45 ` Paul E. McKenney
2017-06-29 3:17 ` Boqun Feng
2017-06-29 18:47 ` Paul E. McKenney
2017-06-29 11:36 ` Will Deacon
2017-06-29 11:38 ` Will Deacon
2017-06-29 15:59 ` Alan Stern
2017-06-29 18:11 ` Paul E. McKenney
2017-06-30 2:51 ` Boqun Feng
2017-06-30 4:02 ` Paul E. McKenney
2017-06-30 5:16 ` Boqun Feng
2017-06-30 17:31 ` Paul E. McKenney
2017-06-29 18:46 ` Paul E. McKenney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170627233748.GZ3721@linux.vnet.ibm.com \
--to=paulmck@linux.vnet.ibm.com \
--cc=arnd@arndb.de \
--cc=dcb314@hotmail.com \
--cc=drozdziak1@gmail.com \
--cc=fengguang.wu@intel.com \
--cc=fweisbec@gmail.com \
--cc=j.alglave@ucl.ac.uk \
--cc=josh@joshtriplett.org \
--cc=kjlx@templeofstupid.com \
--cc=ldr709@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luc.maranget@inria.fr \
--cc=mingo@kernel.org \
--cc=nico@linaro.org \
--cc=parri.andrea@gmail.com \
--cc=peterz@infradead.org \
--cc=priyalee.kushwaha@intel.com \
--cc=riel@redhat.com \
--cc=rostedt@goodmis.org \
--cc=stern@rowland.harvard.edu \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=vegard.nossum@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).