diff mbox

[4/6] arch/sparc: Enable queued rwlocks for SPARC

Message ID 1495154170-854693-5-git-send-email-babu.moger@oracle.com
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

Babu Moger May 19, 2017, 12:36 a.m. UTC 
Enable queued rwlocks for SPARC. Here are the discussions on this feature
when this was introduced.
https://lwn.net/Articles/572765/
https://lwn.net/Articles/582200/

Signed-off-by: Babu Moger <babu.moger@oracle.com>
Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Jane Chu <jane.chu@oracle.com>
Reviewed-by: Shannon Nelson <shannon.nelson@oracle.com>
Reviewed-by: Vijay Kumar <vijay.ac.kumar@oracle.com>
---
 arch/sparc/Kconfig                      |    1 +
 arch/sparc/include/asm/qrwlock.h        |    7 +++++++
 arch/sparc/include/asm/spinlock_64.h    |    9 ++++++---
 arch/sparc/include/asm/spinlock_types.h |    5 ++++-
 4 files changed, 18 insertions(+), 4 deletions(-)
 create mode 100644 arch/sparc/include/asm/qrwlock.h

Comments

David Miller May 19, 2017, 2:31 a.m. UTC | #1 
From: Babu Moger <babu.moger@oracle.com>
Date: Thu, 18 May 2017 18:36:08 -0600

> @@ -82,6 +82,7 @@ config SPARC64
>  	select HAVE_ARCH_AUDITSYSCALL
>  	select ARCH_SUPPORTS_ATOMIC_RMW
>  	select HAVE_NMI
> +	select ARCH_USE_QUEUED_RWLOCKS
>  

If you are selecting this on SPARC64 all the time, then:

> @@ -94,6 +94,7 @@ static inline void arch_spin_lock_flags(arch_spinlock_t *lock, unsigned long fla
>  	: "memory");
>  }
>  
> +#ifndef CONFIG_QUEUED_RWLOCKS
>  /* Multi-reader locks, these are much saner than the 32-bit Sparc ones... */

You can remove this segment of ifdef'd code altogether since it is in
a sparc64 specific header file.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Zijlstra May 19, 2017, 9:03 a.m. UTC | #2 
On Thu, May 18, 2017 at 10:31:13PM -0400, David Miller wrote:
> From: Babu Moger <babu.moger@oracle.com>
> Date: Thu, 18 May 2017 18:36:08 -0600
> 
> > @@ -82,6 +82,7 @@ config SPARC64
> >  	select HAVE_ARCH_AUDITSYSCALL
> >  	select ARCH_SUPPORTS_ATOMIC_RMW
> >  	select HAVE_NMI
> > +	select ARCH_USE_QUEUED_RWLOCKS
> >  
> 
> If you are selecting this on SPARC64 all the time, then:
> 
> > @@ -94,6 +94,7 @@ static inline void arch_spin_lock_flags(arch_spinlock_t *lock, unsigned long fla
> >  	: "memory");
> >  }
> >  
> > +#ifndef CONFIG_QUEUED_RWLOCKS
> >  /* Multi-reader locks, these are much saner than the 32-bit Sparc ones... */
> 
> You can remove this segment of ifdef'd code altogether since it is in
> a sparc64 specific header file.


So IIRC Sparc v8 only has that single byte load-and-set (or swap)
instruction, right? That means you can only make test-and-set spinlocks
and then have to build the world on top of that.

I don't see qrwlock -- which assumes the spinlock implementation is fair
-- making much sense for that.

Also, IIRC Sparc-v8 didn't really have very big SMP systems, those came
with v9. And qspinlock only really makes sense on the bigger systems
(not to mention that building the qspinlock on top of atomic operations
build on test-and-set spinlocks just seems extremely dysfunctional).


In any case, I think what I'm saying is that it makes sense to make this
a Sparcv9 only feature.

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Babu Moger May 19, 2017, 4:36 p.m. UTC | #3 
On 5/18/2017 9:31 PM, David Miller wrote:
> From: Babu Moger <babu.moger@oracle.com>
> Date: Thu, 18 May 2017 18:36:08 -0600
>
>> @@ -82,6 +82,7 @@ config SPARC64
>>   	select HAVE_ARCH_AUDITSYSCALL
>>   	select ARCH_SUPPORTS_ATOMIC_RMW
>>   	select HAVE_NMI
>> +	select ARCH_USE_QUEUED_RWLOCKS
>>   
> If you are selecting this on SPARC64 all the time, then:
>
>> @@ -94,6 +94,7 @@ static inline void arch_spin_lock_flags(arch_spinlock_t *lock, unsigned long fla
>>   	: "memory");
>>   }
>>   
>> +#ifndef CONFIG_QUEUED_RWLOCKS
>>   /* Multi-reader locks, these are much saner than the 32-bit Sparc ones... */
> You can remove this segment of ifdef'd code altogether since it is in
> a sparc64 specific header file.

Sure. will do.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Babu Moger May 19, 2017, 4:43 p.m. UTC | #4 
On 5/19/2017 4:03 AM, Peter Zijlstra wrote:
> On Thu, May 18, 2017 at 10:31:13PM -0400, David Miller wrote:
>> From: Babu Moger <babu.moger@oracle.com>
>> Date: Thu, 18 May 2017 18:36:08 -0600
>>
>>> @@ -82,6 +82,7 @@ config SPARC64
>>>   	select HAVE_ARCH_AUDITSYSCALL
>>>   	select ARCH_SUPPORTS_ATOMIC_RMW
>>>   	select HAVE_NMI
>>> +	select ARCH_USE_QUEUED_RWLOCKS
>>>   
>> If you are selecting this on SPARC64 all the time, then:
>>
>>> @@ -94,6 +94,7 @@ static inline void arch_spin_lock_flags(arch_spinlock_t *lock, unsigned long fla
>>>   	: "memory");
>>>   }
>>>   
>>> +#ifndef CONFIG_QUEUED_RWLOCKS
>>>   /* Multi-reader locks, these are much saner than the 32-bit Sparc ones... */
>> You can remove this segment of ifdef'd code altogether since it is in
>> a sparc64 specific header file.
>
> So IIRC Sparc v8 only has that single byte load-and-set (or swap)
> instruction, right? That means you can only make test-and-set spinlocks
> and then have to build the world on top of that.
>
> I don't see qrwlock -- which assumes the spinlock implementation is fair
> -- making much sense for that.
>
> Also, IIRC Sparc-v8 didn't really have very big SMP systems, those came
> with v9. And qspinlock only really makes sense on the bigger systems
> (not to mention that building the qspinlock on top of atomic operations
> build on test-and-set spinlocks just seems extremely dysfunctional).
>
>
> In any case, I think what I'm saying is that it makes sense to make this
> a Sparcv9 only feature.
>

Agree.  Lets keep this as Sparcv9 only feature.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller May 19, 2017, 7:15 p.m. UTC | #5 
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri, 19 May 2017 11:03:02 +0200

> On Thu, May 18, 2017 at 10:31:13PM -0400, David Miller wrote:
>> From: Babu Moger <babu.moger@oracle.com>
>> Date: Thu, 18 May 2017 18:36:08 -0600
>> 
>> > @@ -82,6 +82,7 @@ config SPARC64
>> >  	select HAVE_ARCH_AUDITSYSCALL
>> >  	select ARCH_SUPPORTS_ATOMIC_RMW
>> >  	select HAVE_NMI
>> > +	select ARCH_USE_QUEUED_RWLOCKS
>> >  
>> 
>> If you are selecting this on SPARC64 all the time, then:
>> 
>> > @@ -94,6 +94,7 @@ static inline void arch_spin_lock_flags(arch_spinlock_t *lock, unsigned long fla
>> >  	: "memory");
>> >  }
>> >  
>> > +#ifndef CONFIG_QUEUED_RWLOCKS
>> >  /* Multi-reader locks, these are much saner than the 32-bit Sparc ones... */
>> 
>> You can remove this segment of ifdef'd code altogether since it is in
>> a sparc64 specific header file.
> 
> 
> So IIRC Sparc v8 only has that single byte load-and-set (or swap)
> instruction, right? That means you can only make test-and-set spinlocks
> and then have to build the world on top of that.
> 
> I don't see qrwlock -- which assumes the spinlock implementation is fair
> -- making much sense for that.
> 
> Also, IIRC Sparc-v8 didn't really have very big SMP systems, those came
> with v9. And qspinlock only really makes sense on the bigger systems
> (not to mention that building the qspinlock on top of atomic operations
> build on test-and-set spinlocks just seems extremely dysfunctional).
> 
> 
> In any case, I think what I'm saying is that it makes sense to make this
> a Sparcv9 only feature.

I agree with you, there is no reason to try and support
queued locks on 32-bit sparc.

However, I don't see what any of this has to do with the feedback
I was giving the patch author :-)
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Zijlstra May 19, 2017, 7:31 p.m. UTC | #6 
On Fri, May 19, 2017 at 03:15:53PM -0400, David Miller wrote:
> However, I don't see what any of this has to do with the feedback
> I was giving the patch author :-)

Uhm,... I think my morning brain read things like you having doubts
about making it sparc64 only. But I could have easily misread things.
Ignore my ramblings :-)
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller May 19, 2017, 7:35 p.m. UTC | #7 
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri, 19 May 2017 21:31:26 +0200

> On Fri, May 19, 2017 at 03:15:53PM -0400, David Miller wrote:
>> However, I don't see what any of this has to do with the feedback
>> I was giving the patch author :-)
> 
> Uhm,... I think my morning brain read things like you having doubts
> about making it sparc64 only. But I could have easily misread things.
> Ignore my ramblings :-)

He was editing a sparc64-specific header, adding "queued locks" ifdefs,
which makes no sense if for SPARC64 the queued locks will always be
enabled.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 8787fc4..caeda1c 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -82,6 +82,7 @@  config SPARC64
 	select HAVE_ARCH_AUDITSYSCALL
 	select ARCH_SUPPORTS_ATOMIC_RMW
 	select HAVE_NMI
+	select ARCH_USE_QUEUED_RWLOCKS
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/sparc/include/asm/qrwlock.h b/arch/sparc/include/asm/qrwlock.h
new file mode 100644
index 0000000..d68a4b1
--- /dev/null
+++ b/arch/sparc/include/asm/qrwlock.h
@@ -0,0 +1,7 @@ 
+#ifndef _ASM_SPARC_QRWLOCK_H
+#define _ASM_SPARC_QRWLOCK_H
+
+#include <asm-generic/qrwlock_types.h>
+#include <asm-generic/qrwlock.h>
+
+#endif /* _ASM_SPARC_QRWLOCK_H */
diff --git a/arch/sparc/include/asm/spinlock_64.h b/arch/sparc/include/asm/spinlock_64.h
index 07c9f2e..562dbd8 100644
--- a/arch/sparc/include/asm/spinlock_64.h
+++ b/arch/sparc/include/asm/spinlock_64.h
@@ -94,6 +94,7 @@  static inline void arch_spin_lock_flags(arch_spinlock_t *lock, unsigned long fla
 	: "memory");
 }
 
+#ifndef CONFIG_QUEUED_RWLOCKS
 /* Multi-reader locks, these are much saner than the 32-bit Sparc ones... */
 
 static inline void arch_read_lock(arch_rwlock_t *lock)
@@ -214,12 +215,14 @@  static inline int arch_write_trylock(arch_rwlock_t *lock)
 	return result;
 }
 
-#define arch_read_lock_flags(p, f) arch_read_lock(p)
-#define arch_write_lock_flags(p, f) arch_write_lock(p)
-
 #define arch_read_can_lock(rw)		(!((rw)->lock & 0x80000000UL))
 #define arch_write_can_lock(rw)	(!(rw)->lock)
 
+#endif /* #ifndef CONFIG_QUEUED_RWLOCKS */
+#include <asm/qrwlock.h>
+#define arch_read_lock_flags(p, f) arch_read_lock(p)
+#define arch_write_lock_flags(p, f) arch_write_lock(p)
+
 #define arch_spin_relax(lock)	cpu_relax()
 #define arch_read_relax(lock)	cpu_relax()
 #define arch_write_relax(lock)	cpu_relax()
diff --git a/arch/sparc/include/asm/spinlock_types.h b/arch/sparc/include/asm/spinlock_types.h
index 9c454fd..e052d28 100644
--- a/arch/sparc/include/asm/spinlock_types.h
+++ b/arch/sparc/include/asm/spinlock_types.h
@@ -11,10 +11,13 @@ 
 
 #define __ARCH_SPIN_LOCK_UNLOCKED	{ 0 }
 
+#ifdef CONFIG_QUEUED_RWLOCKS
+#include <asm-generic/qrwlock_types.h>
+#else
 typedef struct {
 	volatile unsigned int lock;
 } arch_rwlock_t;
 
 #define __ARCH_RW_LOCK_UNLOCKED		{ 0 }
-
+#endif /* CONFIG_QUEUED_RWLOCKS */
 #endif