From: Paul E. McKenney <hidden> Date: 2017-01-15 03:45:03
On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
quoted
* Paul E. McKenney [off-list ref] wrote:
[ . . . ]
quoted
quoted
+ */+#ifdef CONFIG_PPC+#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */+#else /* #ifdef CONFIG_PPC */+#define smp_mb__after_unlock_lock() do { } while (0)+#endif /* #else #ifdef CONFIG_PPC */
Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
#ifdefs into generic headers is generally frowned upon.
The canonical approach would be either to define a helper Kconfig variable that
can be set by PPC (but other architectures don't need to set it), or to expose a
suitable macro (function) for architectures to define in their barrier.h arch
header file.
Very well, I will add a separate commit for this. 4.11 OK?
Does the patch below seem reasonable?
Thanx, Paul
------------------------------------------------------------------------
commit 271c0601237c41a279f975563e13837bace0df03
Author: Paul E. McKenney [off-list ref]
Date: Sat Jan 14 13:32:50 2017 -0800
rcu: Make arch select smp_mb__after_unlock_lock() strength
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 [off-list ref]
Signed-off-by: Paul E. McKenney [off-list ref]
Cc: Peter Zijlstra [off-list ref]
Cc: Will Deacon [off-list ref]
Cc: Boqun Feng [off-list ref]
Cc: Benjamin Herrenschmidt [off-list ref]
Cc: Paul Mackerras [off-list ref]
Cc: Michael Ellerman [off-list ref]
Cc: [off-list ref]
@@ -1175,11 +1175,11 @@ do { \*iftheUNLOCKandLOCKareexecutedbythesameCPUorifthe*UNLOCKandLOCKoperateonthesamelockvariable.*/-#ifdef CONFIG_PPC+#ifdef CONFIG_ARCH_WEAK_RELACQ#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */-#else /* #ifdef CONFIG_PPC */+#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */#define smp_mb__after_unlock_lock() do { } while (0)-#endif /* #else #ifdef CONFIG_PPC */+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
So at the risk of sounding totally pedantic, why not structure it like the
existing smp_mb__before/after*() primitives in barrier.h?
That allows asm-generic/barrier.h to pick up the definition - for example in the
case of smp_acquire__after_ctrl_dep() we do:
#ifndef smp_acquire__after_ctrl_dep
#define smp_acquire__after_ctrl_dep() smp_rmb()
#endif
Which allows Tile to relax it:
arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep() barrier()
I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even
though tree-RCU is the only user of this barrier type.
Thanks,
Ingo
@@ -1175,11 +1175,11 @@ do { \*iftheUNLOCKandLOCKareexecutedbythesameCPUorifthe*UNLOCKandLOCKoperateonthesamelockvariable.*/-#ifdef CONFIG_PPC+#ifdef CONFIG_ARCH_WEAK_RELACQ#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */-#else /* #ifdef CONFIG_PPC */+#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */#define smp_mb__after_unlock_lock() do { } while (0)-#endif /* #else #ifdef CONFIG_PPC */+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
So at the risk of sounding totally pedantic, why not structure it like the
existing smp_mb__before/after*() primitives in barrier.h?
That allows asm-generic/barrier.h to pick up the definition - for example in the
case of smp_acquire__after_ctrl_dep() we do:
#ifndef smp_acquire__after_ctrl_dep
#define smp_acquire__after_ctrl_dep() smp_rmb()
#endif
Which allows Tile to relax it:
arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep() barrier()
I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even
though tree-RCU is the only user of this barrier type.
I wouldn't have any problem with that, however, some time back it was
moved into RCU because (you guessed it!) RCU is the only user. ;-)
Thanx, Paul
@@ -1175,11 +1175,11 @@ do { \*iftheUNLOCKandLOCKareexecutedbythesameCPUorifthe*UNLOCKandLOCKoperateonthesamelockvariable.*/-#ifdef CONFIG_PPC+#ifdef CONFIG_ARCH_WEAK_RELACQ#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */-#else /* #ifdef CONFIG_PPC */+#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */#define smp_mb__after_unlock_lock() do { } while (0)-#endif /* #else #ifdef CONFIG_PPC */+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
So at the risk of sounding totally pedantic, why not structure it like the
existing smp_mb__before/after*() primitives in barrier.h?
That allows asm-generic/barrier.h to pick up the definition - for example in the
case of smp_acquire__after_ctrl_dep() we do:
#ifndef smp_acquire__after_ctrl_dep
#define smp_acquire__after_ctrl_dep() smp_rmb()
#endif
Which allows Tile to relax it:
arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep() barrier()
I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even
though tree-RCU is the only user of this barrier type.
I wouldn't have any problem with that, however, some time back it was
moved into RCU because (you guessed it!) RCU is the only user. ;-)
Indeed ...
[sounds of rummaging around in the Git tree]
I found this commit of yours from ancient history (more than a year ago!):
commit 12d560f4ea87030667438a169912380be00cea4b
Author: Paul E. McKenney [off-list ref]
Date: Tue Jul 14 18:35:23 2015 -0700
rcu,locking: Privatize smp_mb__after_unlock_lock()
RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
likely the only thing that ever will use it, so this commit makes this
macro private to RCU.
Signed-off-by: Paul E. McKenney [off-list ref]
Cc: Will Deacon [off-list ref]
Cc: Peter Zijlstra [off-list ref]
Cc: Benjamin Herrenschmidt [off-list ref]
Cc: "linux-arch@vger.kernel.org" [off-list ref]
So I concur and I'm fine with your patch - or with the status quo code as well.
Thanks,
Ingo
@@ -1175,11 +1175,11 @@ do { \*iftheUNLOCKandLOCKareexecutedbythesameCPUorifthe*UNLOCKandLOCKoperateonthesamelockvariable.*/-#ifdef CONFIG_PPC+#ifdef CONFIG_ARCH_WEAK_RELACQ#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */-#else /* #ifdef CONFIG_PPC */+#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */#define smp_mb__after_unlock_lock() do { } while (0)-#endif /* #else #ifdef CONFIG_PPC */+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
So at the risk of sounding totally pedantic, why not structure it like the
existing smp_mb__before/after*() primitives in barrier.h?
That allows asm-generic/barrier.h to pick up the definition - for example in the
case of smp_acquire__after_ctrl_dep() we do:
#ifndef smp_acquire__after_ctrl_dep
#define smp_acquire__after_ctrl_dep() smp_rmb()
#endif
Which allows Tile to relax it:
arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep() barrier()
I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even
though tree-RCU is the only user of this barrier type.
I wouldn't have any problem with that, however, some time back it was
moved into RCU because (you guessed it!) RCU is the only user. ;-)
Indeed ...
[sounds of rummaging around in the Git tree]
I found this commit of yours from ancient history (more than a year ago!):
commit 12d560f4ea87030667438a169912380be00cea4b
Author: Paul E. McKenney [off-list ref]
Date: Tue Jul 14 18:35:23 2015 -0700
rcu,locking: Privatize smp_mb__after_unlock_lock()
RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
likely the only thing that ever will use it, so this commit makes this
macro private to RCU.
Signed-off-by: Paul E. McKenney [off-list ref]
Cc: Will Deacon [off-list ref]
Cc: Peter Zijlstra [off-list ref]
Cc: Benjamin Herrenschmidt [off-list ref]
Cc: "linux-arch@vger.kernel.org" [off-list ref]
So I concur and I'm fine with your patch - or with the status quo code as well.
I already have the patch queued, so how about I keep it if I get an ack
from the powerpc guys and drop it otherwise?
Thanx, Paul
[sounds of rummaging around in the Git tree]
I found this commit of yours from ancient history (more than a year ago!):
commit 12d560f4ea87030667438a169912380be00cea4b
Author: Paul E. McKenney [off-list ref]
Date: Tue Jul 14 18:35:23 2015 -0700
rcu,locking: Privatize smp_mb__after_unlock_lock()
RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
likely the only thing that ever will use it, so this commit makes this
macro private to RCU.
Signed-off-by: Paul E. McKenney [off-list ref]
Cc: Will Deacon [off-list ref]
Cc: Peter Zijlstra [off-list ref]
Cc: Benjamin Herrenschmidt [off-list ref]
Cc: "linux-arch@vger.kernel.org" [off-list ref]
So I concur and I'm fine with your patch - or with the status quo code as well.
I already have the patch queued, so how about I keep it if I get an ack
from the powerpc guys and drop it otherwise?
Yeah, sounds good! Your patch made me look up 'RelAcq' so it has documentation
value as well ;-)
Thanks,
Ingo
From: Paul E. McKenney <hidden> Date: 2017-01-15 19:45:37
On Sun, Jan 15, 2017 at 10:40:58AM +0100, Ingo Molnar wrote:
* Paul E. McKenney [off-list ref] wrote:
quoted
quoted
[sounds of rummaging around in the Git tree]
I found this commit of yours from ancient history (more than a year ago!):
commit 12d560f4ea87030667438a169912380be00cea4b
Author: Paul E. McKenney [off-list ref]
Date: Tue Jul 14 18:35:23 2015 -0700
rcu,locking: Privatize smp_mb__after_unlock_lock()
RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
likely the only thing that ever will use it, so this commit makes this
macro private to RCU.
Signed-off-by: Paul E. McKenney [off-list ref]
Cc: Will Deacon [off-list ref]
Cc: Peter Zijlstra [off-list ref]
Cc: Benjamin Herrenschmidt [off-list ref]
Cc: "linux-arch@vger.kernel.org" [off-list ref]
So I concur and I'm fine with your patch - or with the status quo code as well.
I already have the patch queued, so how about I keep it if I get an ack
from the powerpc guys and drop it otherwise?
Yeah, sounds good! Your patch made me look up 'RelAcq' so it has documentation
value as well ;-)
;-) ;-) ;-)
Looking forward, my guess would be that if some other code needs
smp_mb__after_unlock_lock() or if some other architecture needs
non-smb_mb() special handling, I should consider making it work the
same as smp_mb__after_atomic() and friends. Does that seem like a
reasonable thought?
Thanx, Paul
On Sun, Jan 15, 2017 at 10:40:58AM +0100, Ingo Molnar wrote:
quoted
* Paul E. McKenney [off-list ref] wrote:
quoted
quoted
[sounds of rummaging around in the Git tree]
I found this commit of yours from ancient history (more than a year ago!):
commit 12d560f4ea87030667438a169912380be00cea4b
Author: Paul E. McKenney [off-list ref]
Date: Tue Jul 14 18:35:23 2015 -0700
rcu,locking: Privatize smp_mb__after_unlock_lock()
RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
likely the only thing that ever will use it, so this commit makes this
macro private to RCU.
Signed-off-by: Paul E. McKenney [off-list ref]
Cc: Will Deacon [off-list ref]
Cc: Peter Zijlstra [off-list ref]
Cc: Benjamin Herrenschmidt [off-list ref]
Cc: "linux-arch@vger.kernel.org" [off-list ref]
So I concur and I'm fine with your patch - or with the status quo code as well.
I already have the patch queued, so how about I keep it if I get an ack
from the powerpc guys and drop it otherwise?
Yeah, sounds good! Your patch made me look up 'RelAcq' so it has documentation
value as well ;-)
;-) ;-) ;-)
Looking forward, my guess would be that if some other code needs
smp_mb__after_unlock_lock() or if some other architecture needs
non-smb_mb() special handling, I should consider making it work the
same as smp_mb__after_atomic() and friends. Does that seem like a
reasonable thought?
Yeah, absolutely - it's just that the pattern triggered the 'this looks a bit too
specialized' response in me, but after seeing the details (again ...) I agree that
this time is different!
Thanks,
Ingo
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-01-23 08:12:09
"Paul E. McKenney" [off-list ref] writes:
On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
quoted
On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
quoted
* Paul E. McKenney [off-list ref] wrote:
[ . . . ]
quoted
quoted
quoted
+ */+#ifdef CONFIG_PPC+#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */+#else /* #ifdef CONFIG_PPC */+#define smp_mb__after_unlock_lock() do { } while (0)+#endif /* #else #ifdef CONFIG_PPC */
Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
#ifdefs into generic headers is generally frowned upon.
The canonical approach would be either to define a helper Kconfig variable that
can be set by PPC (but other architectures don't need to set it), or to expose a
suitable macro (function) for architectures to define in their barrier.h arch
header file.
Very well, I will add a separate commit for this. 4.11 OK?
Does the patch below seem reasonable?
Thanx, Paul
------------------------------------------------------------------------
commit 271c0601237c41a279f975563e13837bace0df03
Author: Paul E. McKenney [off-list ref]
Date: Sat Jan 14 13:32:50 2017 -0800
rcu: Make arch select smp_mb__after_unlock_lock() strength
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 [off-list ref]
Signed-off-by: Paul E. McKenney [off-list ref]
Cc: Peter Zijlstra [off-list ref]
Cc: Will Deacon [off-list ref]
Cc: Boqun Feng [off-list ref]
Cc: Benjamin Herrenschmidt [off-list ref]
Cc: Paul Mackerras [off-list ref]
Cc: Michael Ellerman [off-list ref]
Cc: [off-list ref]
Personally I'd call it ARCH_WEAK_RELEASE_ACQUIRE, which is longer but
clearer I think. But it's not a big deal, so which ever you prefer.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
From: Paul E. McKenney <hidden> Date: 2017-01-24 02:45:38
On Mon, Jan 23, 2017 at 07:12:03PM +1100, Michael Ellerman wrote:
"Paul E. McKenney" [off-list ref] writes:
quoted
On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
quoted
On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
quoted
* Paul E. McKenney [off-list ref] wrote:
[ . . . ]
quoted
quoted
quoted
+ */+#ifdef CONFIG_PPC+#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */+#else /* #ifdef CONFIG_PPC */+#define smp_mb__after_unlock_lock() do { } while (0)+#endif /* #else #ifdef CONFIG_PPC */
Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
#ifdefs into generic headers is generally frowned upon.
The canonical approach would be either to define a helper Kconfig variable that
can be set by PPC (but other architectures don't need to set it), or to expose a
suitable macro (function) for architectures to define in their barrier.h arch
header file.
Very well, I will add a separate commit for this. 4.11 OK?
Does the patch below seem reasonable?
Thanx, Paul
------------------------------------------------------------------------
commit 271c0601237c41a279f975563e13837bace0df03
Author: Paul E. McKenney [off-list ref]
Date: Sat Jan 14 13:32:50 2017 -0800
rcu: Make arch select smp_mb__after_unlock_lock() strength
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 [off-list ref]
Signed-off-by: Paul E. McKenney [off-list ref]
Cc: Peter Zijlstra [off-list ref]
Cc: Will Deacon [off-list ref]
Cc: Boqun Feng [off-list ref]
Cc: Benjamin Herrenschmidt [off-list ref]
Cc: Paul Mackerras [off-list ref]
Cc: Michael Ellerman [off-list ref]
Cc: [off-list ref]
Personally I'd call it ARCH_WEAK_RELEASE_ACQUIRE, which is longer but
clearer I think. But it's not a big deal, so which ever you prefer.