[2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

Commit Message

Peter Zijlstra June 6, 2014, 5:11 p.m. UTC

On Fri, Jun 06, 2014 at 09:09:47AM -0700, James Bottomley wrote:
> On Fri, 2014-06-06 at 08:55 -0700, Davidlohr Bueso wrote:
> > PA-RISC's cmpxchg is not save against normal stores and the code used
> > for optimistic spinning is known broken because of this.
> 
> What about all the other identified architectures?  The problem is that
> unless you can do an atomic Read Modify Write on your architecture, you
> have to implement our exchange primitives with locking, and that makes
> you unsafe against stores  We happen to be the architecture that
> detected this, but I thought we agreed sparc32, metag, tile32, arc and
> possibly hexagon have this problem.
> 
> Rather than naming all the failing architectures, we probably want an 
> 
> ARCH_NO_ATOMIC_RMW

The thing is, all these archs are broken beyond this particular problem,
Mikulas Patocka found a number of other spots.

In any case, sure I can exclude more. Although ideally someone goes do
that __atomic sparse thing to flush out all this.

---
Subject: locking, mutex: Disable optimistic spinning on !RMW archs

For some archs a regular store does not play nice with cmpxchg(), the
optimistic spinning code (and various other places not caught by this)
break this assumption and make things go boom.

Until something better is found, disable optimistic spinning for these
archs.

Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Reported-by: Mikulas Patocka <[e-mail]>
Signed-off-by: Peter Zijlstra <[e-mail]>
---
 kernel/Kconfig.locks | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Chris Metcalf June 6, 2014, 5:19 p.m. UTC | #1

On 6/6/2014 1:11 PM, Peter Zijlstra wrote:
> The thing is, all these archs are broken beyond this particular problem,
> Mikulas Patocka found a number of other spots.
>
> In any case, sure I can exclude more. Although ideally someone goes do
> that __atomic sparse thing to flush out all this.
>
> ---
> Subject: locking, mutex: Disable optimistic spinning on !RMW archs
>
> For some archs a regular store does not play nice with cmpxchg(), the
> optimistic spinning code (and various other places not caught by this)
> break this assumption and make things go boom.
>
> Until something better is found, disable optimistic spinning for these
> archs.
>
> [..]
>   
> +config ARCH_NO_ATOMIC_RMW
> +	def_bool y
> +	depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)

For tile:

Acked-by: Chris Metcalf <[e-mail]>

But you should use "TILEPRO" (added in kernel 3.5) instead of "(TILE && !TILEGX)".

Linus Torvalds June 6, 2014, 5:22 p.m. UTC | #2

On Fri, Jun 6, 2014 at 10:11 AM, Peter Zijlstra <[e-mail]> wrote:
>
> +config ARCH_NO_ATOMIC_RMW
> +       def_bool y
> +       depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)

Ugh. We've had these kinds of things before, and they are broken and
nasty to maintain.

Just make it

    config ARCH_SUPPORTS_ATOMIC_RMW
        bool

which defaults to no. And then make MUTEX_SPIN_ON_OWNER depend on that.

And then we can add "select ARCH_SUPPORTS_ATOMIC_RMW" to the few
architectures we (a) care about and (b) know work. So start with x86,
arm, powerpc and sparc64, and then the rest can just add their own
oneliners if they care.

Remember, most people really won't ever care about this, simply
because it only matters if you have enough CPU's for the whole
spinning thing to make a noticeable difference.  So missing some odd
architecture _really_ doesn't matter.

And we really really *really* shouldn't have these kinds of "random
really odd architecture details" in the generic code, not even if it's
something as specific as kernel/Kconfig.locks.

               Linus

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index d2b32ac27a39..b9c132c48bf1 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -220,6 +220,10 @@  config INLINE_WRITE_UNLOCK_IRQRESTORE
 
 endif
 
+config ARCH_NO_ATOMIC_RMW
+	def_bool y
+	depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)
+
 config MUTEX_SPIN_ON_OWNER
 	def_bool y
-	depends on SMP && !DEBUG_MUTEXES
+	depends on SMP && !DEBUG_MUTEXES && !ARCH_NO_ATOMIC_RMW