diff mbox

[tip/core/rcu,02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

Message ID 1492018825-25634-2-git-send-email-paulmck@linux.vnet.ibm.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Paul E. McKenney April 12, 2017, 5:39 p.m. UTC 
The definition of smp_mb__after_unlock_lock() is currently smp_mb()
for CONFIG_PPC and a no-op otherwise.  It would be better to instead
provide an architecture-selectable Kconfig option, and select the
strength of smp_mb__after_unlock_lock() based on that option.  This
commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
and bases the definition of smp_mb__after_unlock_lock() on this new
CONFIG_ARCH_WEAK_RELACQ Kconfig option.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Boqun Feng <boqun.feng@linux.vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: <linuxppc-dev@lists.ozlabs.org>
---
 arch/Kconfig             | 3 +++
 arch/powerpc/Kconfig     | 1 +
 include/linux/rcupdate.h | 6 +++---
 3 files changed, 7 insertions(+), 3 deletions(-)

Comments

Peter Zijlstra April 13, 2017, 9:21 a.m. UTC | #1 
On Wed, Apr 12, 2017 at 10:39:47AM -0700, Paul E. McKenney wrote:

> CONFIG_ARCH_WEAK_RELACQ Kconfig option.

Naming in the changelog and patch don't match.

> +config ARCH_WEAK_RELEASE_ACQUIRE
> +	bool
> +
Peter Zijlstra April 13, 2017, 9:24 a.m. UTC | #2 
On Wed, Apr 12, 2017 at 10:39:47AM -0700, Paul E. McKenney wrote:
> The definition of smp_mb__after_unlock_lock() is currently smp_mb()
> for CONFIG_PPC and a no-op otherwise.  It would be better to instead
> provide an architecture-selectable Kconfig option, and select the
> strength of smp_mb__after_unlock_lock() based on that option. 


Why is this better? Do we want to have more of this? I thought we still
wanted to convince PPC to go RCsc and eradicate all this RCpc 'fun'. But
instead now you're making it look like its OK to grow more of this pain.
Paul E. McKenney April 13, 2017, 4:17 p.m. UTC | #3 
On Thu, Apr 13, 2017 at 11:21:53AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2017 at 10:39:47AM -0700, Paul E. McKenney wrote:
> 
> > CONFIG_ARCH_WEAK_RELACQ Kconfig option.
> 
> Naming in the changelog and patch don't match.

Good eyes!  Fixed.

							Thanx, Paul

> > +config ARCH_WEAK_RELEASE_ACQUIRE
> > +	bool
> > +
> 
>
Paul E. McKenney April 13, 2017, 4:26 p.m. UTC | #4 
On Thu, Apr 13, 2017 at 11:24:18AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2017 at 10:39:47AM -0700, Paul E. McKenney wrote:
> > The definition of smp_mb__after_unlock_lock() is currently smp_mb()
> > for CONFIG_PPC and a no-op otherwise.  It would be better to instead
> > provide an architecture-selectable Kconfig option, and select the
> > strength of smp_mb__after_unlock_lock() based on that option. 
> 
> Why is this better? Do we want to have more of this? I thought we still
> wanted to convince PPC to go RCsc and eradicate all this RCpc 'fun'. But
> instead now you're making it look like its OK to grow more of this pain.

ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.

To see this, imagine some strange alternate universe in which the Power
hardware guys actually did decide to switch PPC to doing RCsc as you
suggest.  There would still be a lot of Power hardware out there that
still does RCpc.  Therefore, powerpc builds that needed to run on old
Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
built to run only on the shiny new (but mythical) alternate-universe
Power hardware would avoid selecting this Kconfig option.

But the real reason I queued this patch is that Ingo asked me for it:
https://lkml.org/lkml/2017/1/14/88

							Thanx, Paul
Peter Zijlstra April 13, 2017, 4:37 p.m. UTC | #5 
On Thu, Apr 13, 2017 at 09:26:51AM -0700, Paul E. McKenney wrote:

> ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.
> 
> To see this, imagine some strange alternate universe in which the Power
> hardware guys actually did decide to switch PPC to doing RCsc as you
> suggest.  There would still be a lot of Power hardware out there that
> still does RCpc.  Therefore, powerpc builds that needed to run on old
> Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
> built to run only on the shiny new (but mythical) alternate-universe
> Power hardware would avoid selecting this Kconfig option.

Ah, but Power software guys could do it today by replacing an LWSYNC
with a SYNC in say arch_spin_unlock().

And yes, I know this isn't a popular suggestion, but it would do the
trick.

Its just that since there's one (PPC) we can sort of pressure them with
the pain of being the only ones to hit all the bugs. But the moment more
appear (and I'm afraid it'll be MIPS, with the excuse that PPC already
does this) it will be ever so much harder to get rid of it.

Then again, maybe I should just give up and accept the Linux kernel has
RCpc locks..
Paul E. McKenney April 13, 2017, 5:03 p.m. UTC | #6 
On Thu, Apr 13, 2017 at 06:37:57PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 13, 2017 at 09:26:51AM -0700, Paul E. McKenney wrote:
> 
> > ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.
> > 
> > To see this, imagine some strange alternate universe in which the Power
> > hardware guys actually did decide to switch PPC to doing RCsc as you
> > suggest.  There would still be a lot of Power hardware out there that
> > still does RCpc.  Therefore, powerpc builds that needed to run on old
> > Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
> > built to run only on the shiny new (but mythical) alternate-universe
> > Power hardware would avoid selecting this Kconfig option.
> 
> Ah, but Power software guys could do it today by replacing an LWSYNC
> with a SYNC in say arch_spin_unlock().
> 
> And yes, I know this isn't a popular suggestion, but it would do the
> trick.

Indeed, there is a fine line between motivating people to move to new
hardware on the one hand and terminally annoying existing users on
the other.  ;-)

> Its just that since there's one (PPC) we can sort of pressure them with
> the pain of being the only ones to hit all the bugs. But the moment more
> appear (and I'm afraid it'll be MIPS, with the excuse that PPC already
> does this) it will be ever so much harder to get rid of it.
> 
> Then again, maybe I should just give up and accept the Linux kernel has
> RCpc locks..

As usual, I must defer to the powerpc maintainers on this one.

							Thanx, Paul
Michael Ellerman April 19, 2017, 1:38 p.m. UTC | #7 
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:

> On Thu, Apr 13, 2017 at 06:37:57PM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 13, 2017 at 09:26:51AM -0700, Paul E. McKenney wrote:
>> 
>> > ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.
>> > 
>> > To see this, imagine some strange alternate universe in which the Power
>> > hardware guys actually did decide to switch PPC to doing RCsc as you
>> > suggest.  There would still be a lot of Power hardware out there that
>> > still does RCpc.  Therefore, powerpc builds that needed to run on old
>> > Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
>> > built to run only on the shiny new (but mythical) alternate-universe
>> > Power hardware would avoid selecting this Kconfig option.
>> 
>> Ah, but Power software guys could do it today by replacing an LWSYNC
>> with a SYNC in say arch_spin_unlock().
>> 
>> And yes, I know this isn't a popular suggestion, but it would do the
>> trick.
>
> Indeed, there is a fine line between motivating people to move to new
> hardware on the one hand and terminally annoying existing users on
> the other.  ;-)
>
>> Its just that since there's one (PPC) we can sort of pressure them with
>> the pain of being the only ones to hit all the bugs. But the moment more
>> appear (and I'm afraid it'll be MIPS, with the excuse that PPC already
>> does this) it will be ever so much harder to get rid of it.
>> 
>> Then again, maybe I should just give up and accept the Linux kernel has
>> RCpc locks..
>
> As usual, I must defer to the powerpc maintainers on this one.

I reworked my locking tests a bit, to run longer, disable ASLR and a few
other things, and ran them again. They just bang repeatedly on an
uncontended lock, so nothing fancy at all.

Switching the release barrier to sync (from lwsync) slows it down by
about 18%.

So I think that pretty much rules it out, at least on current CPUs.

I'll try and get some more time to make sure I didn't do something
stupid in the test, and maybe do a version that includes some
contention.

cheers
Paul E. McKenney April 19, 2017, 3:09 p.m. UTC | #8 
On Wed, Apr 19, 2017 at 11:38:22PM +1000, Michael Ellerman wrote:
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> 
> > On Thu, Apr 13, 2017 at 06:37:57PM +0200, Peter Zijlstra wrote:
> >> On Thu, Apr 13, 2017 at 09:26:51AM -0700, Paul E. McKenney wrote:
> >> 
> >> > ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.
> >> > 
> >> > To see this, imagine some strange alternate universe in which the Power
> >> > hardware guys actually did decide to switch PPC to doing RCsc as you
> >> > suggest.  There would still be a lot of Power hardware out there that
> >> > still does RCpc.  Therefore, powerpc builds that needed to run on old
> >> > Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
> >> > built to run only on the shiny new (but mythical) alternate-universe
> >> > Power hardware would avoid selecting this Kconfig option.
> >> 
> >> Ah, but Power software guys could do it today by replacing an LWSYNC
> >> with a SYNC in say arch_spin_unlock().
> >> 
> >> And yes, I know this isn't a popular suggestion, but it would do the
> >> trick.
> >
> > Indeed, there is a fine line between motivating people to move to new
> > hardware on the one hand and terminally annoying existing users on
> > the other.  ;-)
> >
> >> Its just that since there's one (PPC) we can sort of pressure them with
> >> the pain of being the only ones to hit all the bugs. But the moment more
> >> appear (and I'm afraid it'll be MIPS, with the excuse that PPC already
> >> does this) it will be ever so much harder to get rid of it.
> >> 
> >> Then again, maybe I should just give up and accept the Linux kernel has
> >> RCpc locks..
> >
> > As usual, I must defer to the powerpc maintainers on this one.
> 
> I reworked my locking tests a bit, to run longer, disable ASLR and a few
> other things, and ran them again. They just bang repeatedly on an
> uncontended lock, so nothing fancy at all.
> 
> Switching the release barrier to sync (from lwsync) slows it down by
> about 18%.

Ouch!!!

> So I think that pretty much rules it out, at least on current CPUs.
> 
> I'll try and get some more time to make sure I didn't do something
> stupid in the test, and maybe do a version that includes some
> contention.

Looking forward to seeing what you come up with...

							Thanx, Paul
diff mbox

Patch

diff --git a/arch/Kconfig b/arch/Kconfig
index cd211a14a88f..adefaf344239 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -320,6 +320,9 @@  config HAVE_CMPXCHG_LOCAL
 config HAVE_CMPXCHG_DOUBLE
 	bool
 
+config ARCH_WEAK_RELEASE_ACQUIRE
+	bool
+
 config ARCH_WANT_IPC_PARSE_VERSION
 	bool
 
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 97a8bc8a095c..7a5c9b764cd2 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -99,6 +99,7 @@  config PPC
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF		if PPC64
 	select ARCH_WANT_IPC_PARSE_VERSION
+	select ARCH_WEAK_RELEASE_ACQUIRE
 	select BINFMT_ELF
 	select BUILDTIME_EXTABLE_SORT
 	select CLONE_BACKWARDS
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index de88b33c0974..e6146d0074f8 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1127,11 +1127,11 @@  do { \
  * if the UNLOCK and LOCK are executed by the same CPU or if the
  * UNLOCK and LOCK operate on the same lock variable.
  */
-#ifdef CONFIG_PPC
+#ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE
 #define smp_mb__after_unlock_lock()	smp_mb()  /* Full ordering for lock. */
-#else /* #ifdef CONFIG_PPC */
+#else /* #ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE */
 #define smp_mb__after_unlock_lock()	do { } while (0)
-#endif /* #else #ifdef CONFIG_PPC */
+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE */
 
 
 #endif /* __LINUX_RCUPDATE_H */