All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 14:45 ` Waiman Long
  0 siblings, 0 replies; 21+ messages in thread
From: Waiman Long @ 2021-02-10 14:45 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Will Deacon, Catalin Marinas,
	Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-kernel, linux-arm-kernel, linux-mips, linux-xtensa,
	Naresh Kamboju, Guenter Roeck, Ben Gardon, Waiman Long

The queued rwlock code has a dependency on the current spinlock
implementation (likely to be qspinlock), but not vice versa. Including
qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
functionality.

If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
include should always be after qspinlock.h. Update the current set of
asm/spinlock.h files to enforce that.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 arch/arm64/include/asm/spinlock.h  | 2 +-
 arch/mips/include/asm/spinlock.h   | 2 +-
 arch/xtensa/include/asm/spinlock.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index 9083d6992603..0525c0b089ed 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -5,8 +5,8 @@
 #ifndef __ASM_SPINLOCK_H
 #define __ASM_SPINLOCK_H
 
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 /* See include/linux/spinlock.h */
 #define smp_mb__after_spinlock()	smp_mb()
diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
index 8a88eb265516..6ce2117e49f6 100644
--- a/arch/mips/include/asm/spinlock.h
+++ b/arch/mips/include/asm/spinlock.h
@@ -10,7 +10,6 @@
 #define _ASM_SPINLOCK_H
 
 #include <asm/processor.h>
-#include <asm/qrwlock.h>
 
 #include <asm-generic/qspinlock_types.h>
 
@@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
 }
 
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #endif /* _ASM_SPINLOCK_H */
diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
index 584b0de6f2ca..41c449ece2d8 100644
--- a/arch/xtensa/include/asm/spinlock.h
+++ b/arch/xtensa/include/asm/spinlock.h
@@ -12,8 +12,8 @@
 #define _XTENSA_SPINLOCK_H
 
 #include <asm/barrier.h>
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #define smp_mb__after_spinlock()	smp_mb()
 
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 14:45 ` Waiman Long
  0 siblings, 0 replies; 21+ messages in thread
From: Waiman Long @ 2021-02-10 14:45 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Will Deacon, Catalin Marinas,
	Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-xtensa, Naresh Kamboju, linux-kernel, linux-mips,
	linux-arm-kernel, Ben Gardon, Waiman Long, Guenter Roeck

The queued rwlock code has a dependency on the current spinlock
implementation (likely to be qspinlock), but not vice versa. Including
qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
functionality.

If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
include should always be after qspinlock.h. Update the current set of
asm/spinlock.h files to enforce that.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 arch/arm64/include/asm/spinlock.h  | 2 +-
 arch/mips/include/asm/spinlock.h   | 2 +-
 arch/xtensa/include/asm/spinlock.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index 9083d6992603..0525c0b089ed 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -5,8 +5,8 @@
 #ifndef __ASM_SPINLOCK_H
 #define __ASM_SPINLOCK_H
 
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 /* See include/linux/spinlock.h */
 #define smp_mb__after_spinlock()	smp_mb()
diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
index 8a88eb265516..6ce2117e49f6 100644
--- a/arch/mips/include/asm/spinlock.h
+++ b/arch/mips/include/asm/spinlock.h
@@ -10,7 +10,6 @@
 #define _ASM_SPINLOCK_H
 
 #include <asm/processor.h>
-#include <asm/qrwlock.h>
 
 #include <asm-generic/qspinlock_types.h>
 
@@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
 }
 
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #endif /* _ASM_SPINLOCK_H */
diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
index 584b0de6f2ca..41c449ece2d8 100644
--- a/arch/xtensa/include/asm/spinlock.h
+++ b/arch/xtensa/include/asm/spinlock.h
@@ -12,8 +12,8 @@
 #define _XTENSA_SPINLOCK_H
 
 #include <asm/barrier.h>
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #define smp_mb__after_spinlock()	smp_mb()
 
-- 
2.18.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
  2021-02-10 14:45 ` Waiman Long
@ 2021-02-10 15:05   ` Guenter Roeck
  -1 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2021-02-10 15:05 UTC (permalink / raw)
  To: Waiman Long, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-kernel, linux-arm-kernel, linux-mips, linux-xtensa,
	Naresh Kamboju, Ben Gardon

On 2/10/21 6:45 AM, Waiman Long wrote:
> The queued rwlock code has a dependency on the current spinlock
> implementation (likely to be qspinlock), but not vice versa. Including
> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
> functionality.
> 
> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
> include should always be after qspinlock.h. Update the current set of
> asm/spinlock.h files to enforce that.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

There should be a Fixes: tag here. If the SHA of the offending commit is not
stable, there should be a better reference than "The queued rwlock code".

This patch fixes the build problem I had observed on mips. I also tested
xtensa:defconfig and arm64:defconfig with no problems observed.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>  arch/arm64/include/asm/spinlock.h  | 2 +-
>  arch/mips/include/asm/spinlock.h   | 2 +-
>  arch/xtensa/include/asm/spinlock.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
> index 9083d6992603..0525c0b089ed 100644
> --- a/arch/arm64/include/asm/spinlock.h
> +++ b/arch/arm64/include/asm/spinlock.h
> @@ -5,8 +5,8 @@
>  #ifndef __ASM_SPINLOCK_H
>  #define __ASM_SPINLOCK_H
>  
> -#include <asm/qrwlock.h>
>  #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>  
>  /* See include/linux/spinlock.h */
>  #define smp_mb__after_spinlock()	smp_mb()
> diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
> index 8a88eb265516..6ce2117e49f6 100644
> --- a/arch/mips/include/asm/spinlock.h
> +++ b/arch/mips/include/asm/spinlock.h
> @@ -10,7 +10,6 @@
>  #define _ASM_SPINLOCK_H
>  
>  #include <asm/processor.h>
> -#include <asm/qrwlock.h>
>  
>  #include <asm-generic/qspinlock_types.h>
>  
> @@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
>  }
>  
>  #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>  
>  #endif /* _ASM_SPINLOCK_H */
> diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
> index 584b0de6f2ca..41c449ece2d8 100644
> --- a/arch/xtensa/include/asm/spinlock.h
> +++ b/arch/xtensa/include/asm/spinlock.h
> @@ -12,8 +12,8 @@
>  #define _XTENSA_SPINLOCK_H
>  
>  #include <asm/barrier.h>
> -#include <asm/qrwlock.h>
>  #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>  
>  #define smp_mb__after_spinlock()	smp_mb()
>  
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 15:05   ` Guenter Roeck
  0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2021-02-10 15:05 UTC (permalink / raw)
  To: Waiman Long, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-xtensa, Naresh Kamboju, linux-kernel, linux-mips,
	Ben Gardon, linux-arm-kernel

On 2/10/21 6:45 AM, Waiman Long wrote:
> The queued rwlock code has a dependency on the current spinlock
> implementation (likely to be qspinlock), but not vice versa. Including
> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
> functionality.
> 
> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
> include should always be after qspinlock.h. Update the current set of
> asm/spinlock.h files to enforce that.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

There should be a Fixes: tag here. If the SHA of the offending commit is not
stable, there should be a better reference than "The queued rwlock code".

This patch fixes the build problem I had observed on mips. I also tested
xtensa:defconfig and arm64:defconfig with no problems observed.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>  arch/arm64/include/asm/spinlock.h  | 2 +-
>  arch/mips/include/asm/spinlock.h   | 2 +-
>  arch/xtensa/include/asm/spinlock.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
> index 9083d6992603..0525c0b089ed 100644
> --- a/arch/arm64/include/asm/spinlock.h
> +++ b/arch/arm64/include/asm/spinlock.h
> @@ -5,8 +5,8 @@
>  #ifndef __ASM_SPINLOCK_H
>  #define __ASM_SPINLOCK_H
>  
> -#include <asm/qrwlock.h>
>  #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>  
>  /* See include/linux/spinlock.h */
>  #define smp_mb__after_spinlock()	smp_mb()
> diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
> index 8a88eb265516..6ce2117e49f6 100644
> --- a/arch/mips/include/asm/spinlock.h
> +++ b/arch/mips/include/asm/spinlock.h
> @@ -10,7 +10,6 @@
>  #define _ASM_SPINLOCK_H
>  
>  #include <asm/processor.h>
> -#include <asm/qrwlock.h>
>  
>  #include <asm-generic/qspinlock_types.h>
>  
> @@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
>  }
>  
>  #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>  
>  #endif /* _ASM_SPINLOCK_H */
> diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
> index 584b0de6f2ca..41c449ece2d8 100644
> --- a/arch/xtensa/include/asm/spinlock.h
> +++ b/arch/xtensa/include/asm/spinlock.h
> @@ -12,8 +12,8 @@
>  #define _XTENSA_SPINLOCK_H
>  
>  #include <asm/barrier.h>
> -#include <asm/qrwlock.h>
>  #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>  
>  #define smp_mb__after_spinlock()	smp_mb()
>  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
  2021-02-10 15:05   ` Guenter Roeck
@ 2021-02-10 15:53     ` Waiman Long
  -1 siblings, 0 replies; 21+ messages in thread
From: Waiman Long @ 2021-02-10 15:53 UTC (permalink / raw)
  To: Guenter Roeck, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-kernel, linux-arm-kernel, linux-mips, linux-xtensa,
	Naresh Kamboju, Ben Gardon

On 2/10/21 10:05 AM, Guenter Roeck wrote:
> On 2/10/21 6:45 AM, Waiman Long wrote:
>> The queued rwlock code has a dependency on the current spinlock
>> implementation (likely to be qspinlock), but not vice versa. Including
>> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
>> functionality.
>>
>> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
>> include should always be after qspinlock.h. Update the current set of
>> asm/spinlock.h files to enforce that.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> There should be a Fixes: tag here. If the SHA of the offending commit is not
> stable, there should be a better reference than "The queued rwlock code".
I originally have a Fixes tag when I was modifying the mips' 
asm/spinlock.h file. After I realize that there are more files to 
modify, I take that out. Anyway, the problem was exposed by Ben's 
qrwlock patch. So existing stable releases should still be fine without 
this patch.
>
> This patch fixes the build problem I had observed on mips. I also tested
> xtensa:defconfig and arm64:defconfig with no problems observed.
>
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks for the testing as I don't have a build environment to verify that.

Cheers,
Longman


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 15:53     ` Waiman Long
  0 siblings, 0 replies; 21+ messages in thread
From: Waiman Long @ 2021-02-10 15:53 UTC (permalink / raw)
  To: Guenter Roeck, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-xtensa, Naresh Kamboju, linux-kernel, linux-mips,
	Ben Gardon, linux-arm-kernel

On 2/10/21 10:05 AM, Guenter Roeck wrote:
> On 2/10/21 6:45 AM, Waiman Long wrote:
>> The queued rwlock code has a dependency on the current spinlock
>> implementation (likely to be qspinlock), but not vice versa. Including
>> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
>> functionality.
>>
>> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
>> include should always be after qspinlock.h. Update the current set of
>> asm/spinlock.h files to enforce that.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> There should be a Fixes: tag here. If the SHA of the offending commit is not
> stable, there should be a better reference than "The queued rwlock code".
I originally have a Fixes tag when I was modifying the mips' 
asm/spinlock.h file. After I realize that there are more files to 
modify, I take that out. Anyway, the problem was exposed by Ben's 
qrwlock patch. So existing stable releases should still be fine without 
this patch.
>
> This patch fixes the build problem I had observed on mips. I also tested
> xtensa:defconfig and arm64:defconfig with no problems observed.
>
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks for the testing as I don't have a build environment to verify that.

Cheers,
Longman


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
  2021-02-10 14:45 ` Waiman Long
@ 2021-02-10 16:19   ` Thomas Bogendoerfer
  -1 siblings, 0 replies; 21+ messages in thread
From: Thomas Bogendoerfer @ 2021-02-10 16:19 UTC (permalink / raw)
  To: Waiman Long
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Catalin Marinas,
	Chris Zankel, Max Filippov, linux-kernel, linux-arm-kernel,
	linux-mips, linux-xtensa, Naresh Kamboju, Guenter Roeck,
	Ben Gardon

On Wed, Feb 10, 2021 at 09:45:56AM -0500, Waiman Long wrote:
> The queued rwlock code has a dependency on the current spinlock
> implementation (likely to be qspinlock), but not vice versa. Including
> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
> functionality.
> 
> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
> include should always be after qspinlock.h. Update the current set of
> asm/spinlock.h files to enforce that.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  arch/arm64/include/asm/spinlock.h  | 2 +-
>  arch/mips/include/asm/spinlock.h   | 2 +-
>  arch/xtensa/include/asm/spinlock.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

which tree should this go through ? I can take it via mips-next,
if everybody agrees.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 16:19   ` Thomas Bogendoerfer
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Bogendoerfer @ 2021-02-10 16:19 UTC (permalink / raw)
  To: Waiman Long
  Cc: Chris Zankel, linux-xtensa, Peter Zijlstra, Catalin Marinas,
	Naresh Kamboju, linux-kernel, linux-mips, Max Filippov,
	Ingo Molnar, Guenter Roeck, Ben Gardon, Will Deacon,
	linux-arm-kernel

On Wed, Feb 10, 2021 at 09:45:56AM -0500, Waiman Long wrote:
> The queued rwlock code has a dependency on the current spinlock
> implementation (likely to be qspinlock), but not vice versa. Including
> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
> functionality.
> 
> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
> include should always be after qspinlock.h. Update the current set of
> asm/spinlock.h files to enforce that.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  arch/arm64/include/asm/spinlock.h  | 2 +-
>  arch/mips/include/asm/spinlock.h   | 2 +-
>  arch/xtensa/include/asm/spinlock.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

which tree should this go through ? I can take it via mips-next,
if everybody agrees.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
  2021-02-10 15:53     ` Waiman Long
@ 2021-02-10 17:33       ` Ben Gardon
  -1 siblings, 0 replies; 21+ messages in thread
From: Ben Gardon @ 2021-02-10 17:33 UTC (permalink / raw)
  To: Waiman Long
  Cc: Guenter Roeck, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov,
	LKML, linux-arm-kernel, linux-mips, linux-xtensa, Naresh Kamboju

On Wed, Feb 10, 2021 at 7:54 AM Waiman Long <longman@redhat.com> wrote:
>
> On 2/10/21 10:05 AM, Guenter Roeck wrote:
> > On 2/10/21 6:45 AM, Waiman Long wrote:
> >> The queued rwlock code has a dependency on the current spinlock
> >> implementation (likely to be qspinlock), but not vice versa. Including
> >> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
> >> functionality.
> >>
> >> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
> >> include should always be after qspinlock.h. Update the current set of
> >> asm/spinlock.h files to enforce that.
> >>
> >> Signed-off-by: Waiman Long <longman@redhat.com>
> > There should be a Fixes: tag here. If the SHA of the offending commit is not
> > stable, there should be a better reference than "The queued rwlock code".
> I originally have a Fixes tag when I was modifying the mips'
> asm/spinlock.h file. After I realize that there are more files to
> modify, I take that out. Anyway, the problem was exposed by Ben's
> qrwlock patch. So existing stable releases should still be fine without
> this patch.
> >
> > This patch fixes the build problem I had observed on mips. I also tested
> > xtensa:defconfig and arm64:defconfig with no problems observed.
> >
> > Tested-by: Guenter Roeck <linux@roeck-us.net>
>
> Thanks for the testing as I don't have a build environment to verify that.
>
> Cheers,
> Longman
>

Thanks Longman and Guenter for developing and testing this fix! I
don't have the environment to test this either, but the patch looks
good to me.
Reviewed-by: Ben Gardon <bgardon@google.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 17:33       ` Ben Gardon
  0 siblings, 0 replies; 21+ messages in thread
From: Ben Gardon @ 2021-02-10 17:33 UTC (permalink / raw)
  To: Waiman Long
  Cc: Chris Zankel, Thomas Bogendoerfer, linux-xtensa, Peter Zijlstra,
	Catalin Marinas, Naresh Kamboju, LKML, linux-mips, Max Filippov,
	Ingo Molnar, linux-arm-kernel, Will Deacon, Guenter Roeck

On Wed, Feb 10, 2021 at 7:54 AM Waiman Long <longman@redhat.com> wrote:
>
> On 2/10/21 10:05 AM, Guenter Roeck wrote:
> > On 2/10/21 6:45 AM, Waiman Long wrote:
> >> The queued rwlock code has a dependency on the current spinlock
> >> implementation (likely to be qspinlock), but not vice versa. Including
> >> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
> >> functionality.
> >>
> >> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
> >> include should always be after qspinlock.h. Update the current set of
> >> asm/spinlock.h files to enforce that.
> >>
> >> Signed-off-by: Waiman Long <longman@redhat.com>
> > There should be a Fixes: tag here. If the SHA of the offending commit is not
> > stable, there should be a better reference than "The queued rwlock code".
> I originally have a Fixes tag when I was modifying the mips'
> asm/spinlock.h file. After I realize that there are more files to
> modify, I take that out. Anyway, the problem was exposed by Ben's
> qrwlock patch. So existing stable releases should still be fine without
> this patch.
> >
> > This patch fixes the build problem I had observed on mips. I also tested
> > xtensa:defconfig and arm64:defconfig with no problems observed.
> >
> > Tested-by: Guenter Roeck <linux@roeck-us.net>
>
> Thanks for the testing as I don't have a build environment to verify that.
>
> Cheers,
> Longman
>

Thanks Longman and Guenter for developing and testing this fix! I
don't have the environment to test this either, but the patch looks
good to me.
Reviewed-by: Ben Gardon <bgardon@google.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
  2021-02-10 14:45 ` Waiman Long
@ 2021-02-10 18:28   ` Paolo Bonzini
  -1 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2021-02-10 18:28 UTC (permalink / raw)
  To: Waiman Long, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-kernel, linux-arm-kernel, linux-mips, linux-xtensa,
	Naresh Kamboju, Guenter Roeck, Ben Gardon

On 10/02/21 15:45, Waiman Long wrote:
> The queued rwlock code has a dependency on the current spinlock
> implementation (likely to be qspinlock), but not vice versa. Including
> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
> functionality.
> 
> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
> include should always be after qspinlock.h. Update the current set of
> asm/spinlock.h files to enforce that.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

arch/sparc/include/asm/spinlock_64.h is missing.  Also, the include in 
kernel/locking/qrwlock.c is not necessary (it may be there for aesthetic 
reasons, but it complicates thing in this case).

I'll send a v2 that is based on the kvm/next tree.

Paolo

> ---
>   arch/arm64/include/asm/spinlock.h  | 2 +-
>   arch/mips/include/asm/spinlock.h   | 2 +-
>   arch/xtensa/include/asm/spinlock.h | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
> index 9083d6992603..0525c0b089ed 100644
> --- a/arch/arm64/include/asm/spinlock.h
> +++ b/arch/arm64/include/asm/spinlock.h
> @@ -5,8 +5,8 @@
>   #ifndef __ASM_SPINLOCK_H
>   #define __ASM_SPINLOCK_H
>   
> -#include <asm/qrwlock.h>
>   #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>   
>   /* See include/linux/spinlock.h */
>   #define smp_mb__after_spinlock()	smp_mb()
> diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
> index 8a88eb265516..6ce2117e49f6 100644
> --- a/arch/mips/include/asm/spinlock.h
> +++ b/arch/mips/include/asm/spinlock.h
> @@ -10,7 +10,6 @@
>   #define _ASM_SPINLOCK_H
>   
>   #include <asm/processor.h>
> -#include <asm/qrwlock.h>
>   
>   #include <asm-generic/qspinlock_types.h>
>   
> @@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
>   }
>   
>   #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>   
>   #endif /* _ASM_SPINLOCK_H */
> diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
> index 584b0de6f2ca..41c449ece2d8 100644
> --- a/arch/xtensa/include/asm/spinlock.h
> +++ b/arch/xtensa/include/asm/spinlock.h
> @@ -12,8 +12,8 @@
>   #define _XTENSA_SPINLOCK_H
>   
>   #include <asm/barrier.h>
> -#include <asm/qrwlock.h>
>   #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>   
>   #define smp_mb__after_spinlock()	smp_mb()
>   
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 18:28   ` Paolo Bonzini
  0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2021-02-10 18:28 UTC (permalink / raw)
  To: Waiman Long, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-xtensa, Naresh Kamboju, linux-kernel, linux-mips,
	linux-arm-kernel, Ben Gardon, Guenter Roeck

On 10/02/21 15:45, Waiman Long wrote:
> The queued rwlock code has a dependency on the current spinlock
> implementation (likely to be qspinlock), but not vice versa. Including
> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
> functionality.
> 
> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
> include should always be after qspinlock.h. Update the current set of
> asm/spinlock.h files to enforce that.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

arch/sparc/include/asm/spinlock_64.h is missing.  Also, the include in 
kernel/locking/qrwlock.c is not necessary (it may be there for aesthetic 
reasons, but it complicates thing in this case).

I'll send a v2 that is based on the kvm/next tree.

Paolo

> ---
>   arch/arm64/include/asm/spinlock.h  | 2 +-
>   arch/mips/include/asm/spinlock.h   | 2 +-
>   arch/xtensa/include/asm/spinlock.h | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
> index 9083d6992603..0525c0b089ed 100644
> --- a/arch/arm64/include/asm/spinlock.h
> +++ b/arch/arm64/include/asm/spinlock.h
> @@ -5,8 +5,8 @@
>   #ifndef __ASM_SPINLOCK_H
>   #define __ASM_SPINLOCK_H
>   
> -#include <asm/qrwlock.h>
>   #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>   
>   /* See include/linux/spinlock.h */
>   #define smp_mb__after_spinlock()	smp_mb()
> diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
> index 8a88eb265516..6ce2117e49f6 100644
> --- a/arch/mips/include/asm/spinlock.h
> +++ b/arch/mips/include/asm/spinlock.h
> @@ -10,7 +10,6 @@
>   #define _ASM_SPINLOCK_H
>   
>   #include <asm/processor.h>
> -#include <asm/qrwlock.h>
>   
>   #include <asm-generic/qspinlock_types.h>
>   
> @@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
>   }
>   
>   #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>   
>   #endif /* _ASM_SPINLOCK_H */
> diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
> index 584b0de6f2ca..41c449ece2d8 100644
> --- a/arch/xtensa/include/asm/spinlock.h
> +++ b/arch/xtensa/include/asm/spinlock.h
> @@ -12,8 +12,8 @@
>   #define _XTENSA_SPINLOCK_H
>   
>   #include <asm/barrier.h>
> -#include <asm/qrwlock.h>
>   #include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
>   
>   #define smp_mb__after_spinlock()	smp_mb()
>   
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
  2021-02-10 18:28   ` Paolo Bonzini
@ 2021-02-10 18:50     ` Waiman Long
  -1 siblings, 0 replies; 21+ messages in thread
From: Waiman Long @ 2021-02-10 18:50 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-kernel, linux-arm-kernel, linux-mips, linux-xtensa,
	Naresh Kamboju, Guenter Roeck, Ben Gardon

On 2/10/21 1:28 PM, Paolo Bonzini wrote:
> On 10/02/21 15:45, Waiman Long wrote:
>> The queued rwlock code has a dependency on the current spinlock
>> implementation (likely to be qspinlock), but not vice versa. Including
>> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
>> functionality.
>>
>> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
>> include should always be after qspinlock.h. Update the current set of
>> asm/spinlock.h files to enforce that.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>
> arch/sparc/include/asm/spinlock_64.h is missing.  Also, the include in 
> kernel/locking/qrwlock.c is not necessary (it may be there for 
> aesthetic reasons, but it complicates thing in this case).

Sorry for missing arch/sparc/include/asm/spinlock_64.h. I was just 
focusing on asm/spinlock.h and not aware that there are other variants 
there.

It is true that the asm/qrwlock.h include in qrwlock.c is not really 
necessary. I can't recall why it was there.

>
> I'll send a v2 that is based on the kvm/next tree.
>
> Paolo
>
Thanks for taking care of that.

Cheers,
Longman


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 18:50     ` Waiman Long
  0 siblings, 0 replies; 21+ messages in thread
From: Waiman Long @ 2021-02-10 18:50 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Thomas Bogendoerfer, Chris Zankel, Max Filippov
  Cc: linux-xtensa, Naresh Kamboju, linux-kernel, linux-mips,
	linux-arm-kernel, Ben Gardon, Guenter Roeck

On 2/10/21 1:28 PM, Paolo Bonzini wrote:
> On 10/02/21 15:45, Waiman Long wrote:
>> The queued rwlock code has a dependency on the current spinlock
>> implementation (likely to be qspinlock), but not vice versa. Including
>> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock
>> functionality.
>>
>> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h
>> include should always be after qspinlock.h. Update the current set of
>> asm/spinlock.h files to enforce that.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>
> arch/sparc/include/asm/spinlock_64.h is missing.  Also, the include in 
> kernel/locking/qrwlock.c is not necessary (it may be there for 
> aesthetic reasons, but it complicates thing in this case).

Sorry for missing arch/sparc/include/asm/spinlock_64.h. I was just 
focusing on asm/spinlock.h and not aware that there are other variants 
there.

It is true that the asm/qrwlock.h include in qrwlock.c is not really 
necessary. I can't recall why it was there.

>
> I'll send a v2 that is based on the kvm/next tree.
>
> Paolo
>
Thanks for taking care of that.

Cheers,
Longman


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
  2021-02-10 16:19   ` Thomas Bogendoerfer
@ 2021-02-11 12:59     ` Paolo Bonzini
  -1 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2021-02-11 12:59 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Waiman Long
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Catalin Marinas,
	Chris Zankel, Max Filippov, linux-kernel, linux-arm-kernel,
	linux-mips, linux-xtensa, Naresh Kamboju, Guenter Roeck,
	Ben Gardon

On 10/02/21 17:19, Thomas Bogendoerfer wrote:
>>   arch/arm64/include/asm/spinlock.h  | 2 +-
>>   arch/mips/include/asm/spinlock.h   | 2 +-
>>   arch/xtensa/include/asm/spinlock.h | 2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
> which tree should this go through ? I can take it via mips-next,
> if everybody agrees.

The breakage is in the KVM tree, and the existing patch has acked-by 
from the locking primitives folks.  So I'll queue it there in order to 
limit the range that breaks bisection.

Paolo


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-11 12:59     ` Paolo Bonzini
  0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2021-02-11 12:59 UTC (permalink / raw)
  To: Thomas Bogendoerfer, Waiman Long
  Cc: Chris Zankel, linux-xtensa, Peter Zijlstra, Catalin Marinas,
	Naresh Kamboju, linux-kernel, linux-mips, Max Filippov,
	Ingo Molnar, Guenter Roeck, Ben Gardon, Will Deacon,
	linux-arm-kernel

On 10/02/21 17:19, Thomas Bogendoerfer wrote:
>>   arch/arm64/include/asm/spinlock.h  | 2 +-
>>   arch/mips/include/asm/spinlock.h   | 2 +-
>>   arch/xtensa/include/asm/spinlock.h | 2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
> which tree should this go through ? I can take it via mips-next,
> if everybody agrees.

The breakage is in the KVM tree, and the existing patch has acked-by 
from the locking primitives folks.  So I'll queue it there in order to 
limit the range that breaks bisection.

Paolo


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
  2021-02-11 12:59     ` Paolo Bonzini
@ 2021-02-11 14:41       ` Thomas Bogendoerfer
  -1 siblings, 0 replies; 21+ messages in thread
From: Thomas Bogendoerfer @ 2021-02-11 14:41 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Waiman Long, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Catalin Marinas, Chris Zankel, Max Filippov, linux-kernel,
	linux-arm-kernel, linux-mips, linux-xtensa, Naresh Kamboju,
	Guenter Roeck, Ben Gardon

On Thu, Feb 11, 2021 at 01:59:35PM +0100, Paolo Bonzini wrote:
> On 10/02/21 17:19, Thomas Bogendoerfer wrote:
> > >   arch/arm64/include/asm/spinlock.h  | 2 +-
> > >   arch/mips/include/asm/spinlock.h   | 2 +-
> > >   arch/xtensa/include/asm/spinlock.h | 2 +-
> > >   3 files changed, 3 insertions(+), 3 deletions(-)
> > which tree should this go through ? I can take it via mips-next,
> > if everybody agrees.
> 
> The breakage is in the KVM tree, and the existing patch has acked-by from
> the locking primitives folks.  So I'll queue it there in order to limit the
> range that breaks bisection.

if it's not too late you can add by 

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-11 14:41       ` Thomas Bogendoerfer
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Bogendoerfer @ 2021-02-11 14:41 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Chris Zankel, linux-xtensa, Peter Zijlstra, Catalin Marinas,
	Naresh Kamboju, linux-kernel, linux-mips, Max Filippov,
	Ingo Molnar, Guenter Roeck, Ben Gardon, Waiman Long, Will Deacon,
	linux-arm-kernel

On Thu, Feb 11, 2021 at 01:59:35PM +0100, Paolo Bonzini wrote:
> On 10/02/21 17:19, Thomas Bogendoerfer wrote:
> > >   arch/arm64/include/asm/spinlock.h  | 2 +-
> > >   arch/mips/include/asm/spinlock.h   | 2 +-
> > >   arch/xtensa/include/asm/spinlock.h | 2 +-
> > >   3 files changed, 3 insertions(+), 3 deletions(-)
> > which tree should this go through ? I can take it via mips-next,
> > if everybody agrees.
> 
> The breakage is in the KVM tree, and the existing patch has acked-by from
> the locking primitives folks.  So I'll queue it there in order to limit the
> range that breaks bisection.

if it's not too late you can add by 

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 18:33 ` Paolo Bonzini
  0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2021-02-10 18:33 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Guenter Roeck, Waiman Long, Ben Gardon, Peter Zijlstra,
	Ingo Molnar, Will Deacon, Catalin Marinas, Thomas Bogendoerfer,
	David S. Miller, Chris Zankel, Max Filippov, Arnd Bergmann,
	Guo Ren, Davidlohr Bueso,
	moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	open list:MIPS, open list:SPARC + UltraSPARC (sparc/sparc64),
	open list:TENSILICA XTENSA PORT (xtensa),
	open list:GENERIC INCLUDE/ASM HEADER FILES,
	open list:C-SKY ARCHITECTURE

include/asm-generic/qrwlock.h was trying to get arch_spin_is_locked via
asm-generic/qspinlock.h.  However, this does not work because architectures
might be using queued rwlocks but not queued spinlocks (csky), or because they
might be defining their own queued_* macros before including asm/qspinlock.h.

To fix this, ensure that asm/spinlock.h always includes qrwlock.h after
defining arch_spin_is_locked (either directly for csky, or via
asm/qspinlock.h for other architectures).  The only inclusion elsewhere
is in kernel/locking/qrwlock.c.  That one is really unnecessary because
the file is only compiled in SMP configurations (config QUEUED_RWLOCKS
depends on SMP) and in that case linux/spinlock.h already includes
asm/qrwlock.h if needed, via asm/spinlock.h.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Cc: Waiman Long <longman@redhat.com>
Fixes: 26128cb6c7e6 ("locking/rwlocks: Add contention detection for rwlocks")
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Ben Gardon <bgardon@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2: Fix sparc too.  Add a comment in qrwlock.h itself.
	Remove unnecessary inclusion in kernel/locking/qrwlock.c

 arch/arm64/include/asm/spinlock.h    | 2 +-
 arch/mips/include/asm/spinlock.h     | 2 +-
 arch/sparc/include/asm/spinlock_64.h | 2 +-
 arch/xtensa/include/asm/spinlock.h   | 2 +-
 include/asm-generic/qrwlock.h        | 3 ++-
 kernel/locking/qrwlock.c             | 1 -
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index 9083d6992603..0525c0b089ed 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -5,8 +5,8 @@
 #ifndef __ASM_SPINLOCK_H
 #define __ASM_SPINLOCK_H
 
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 /* See include/linux/spinlock.h */
 #define smp_mb__after_spinlock()	smp_mb()
diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
index 8a88eb265516..6ce2117e49f6 100644
--- a/arch/mips/include/asm/spinlock.h
+++ b/arch/mips/include/asm/spinlock.h
@@ -10,7 +10,6 @@
 #define _ASM_SPINLOCK_H
 
 #include <asm/processor.h>
-#include <asm/qrwlock.h>
 
 #include <asm-generic/qspinlock_types.h>
 
@@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
 }
 
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #endif /* _ASM_SPINLOCK_H */
diff --git a/arch/sparc/include/asm/spinlock_64.h b/arch/sparc/include/asm/spinlock_64.h
index 7fc82a233f49..3a9a0b0c7465 100644
--- a/arch/sparc/include/asm/spinlock_64.h
+++ b/arch/sparc/include/asm/spinlock_64.h
@@ -11,8 +11,8 @@
 
 #include <asm/processor.h>
 #include <asm/barrier.h>
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #endif /* !(__ASSEMBLY__) */
 
diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
index 584b0de6f2ca..41c449ece2d8 100644
--- a/arch/xtensa/include/asm/spinlock.h
+++ b/arch/xtensa/include/asm/spinlock.h
@@ -12,8 +12,8 @@
 #define _XTENSA_SPINLOCK_H
 
 #include <asm/barrier.h>
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #define smp_mb__after_spinlock()	smp_mb()
 
diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 0020d3b820a7..7ae0ece07b4e 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -14,7 +14,8 @@
 #include <asm/processor.h>
 
 #include <asm-generic/qrwlock_types.h>
-#include <asm-generic/qspinlock.h>
+
+/* Must be included from asm/spinlock.h after defining arch_spin_is_locked.  */
 
 /*
  * Writer states & reader shift and bias.
diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
index fe9ca92faa2a..4786dd271b45 100644
--- a/kernel/locking/qrwlock.c
+++ b/kernel/locking/qrwlock.c
@@ -12,7 +12,6 @@
 #include <linux/percpu.h>
 #include <linux/hardirq.h>
 #include <linux/spinlock.h>
-#include <asm/qrwlock.h>
 
 /**
  * queued_read_lock_slowpath - acquire read lock of a queue rwlock
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 18:33 ` Paolo Bonzini
  0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2021-02-10 18:33 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Guenter Roeck, Waiman Long, Ben Gardon, Peter Zijlstra,
	Ingo Molnar, Will Deacon, Catalin Marinas, Thomas Bogendoerfer,
	David S. Miller, Chris Zankel, Max Filippov, Arnd Bergmann,
	Guo Ren, Davidlohr Bueso,
	moderated list:ARM64 PORT AARCH64 ARCHITECTURE, open list:MIPS,
	open list:SPARC + UltraSPARC sparc/sparc64,
	open list:TENSILICA XTENSA PORT xtensa,
	open list:GENERIC INCLUDE/ASM HEADER FILES,
	open list:C-SKY ARCHITECTURE

include/asm-generic/qrwlock.h was trying to get arch_spin_is_locked via
asm-generic/qspinlock.h.  However, this does not work because architectures
might be using queued rwlocks but not queued spinlocks (csky), or because they
might be defining their own queued_* macros before including asm/qspinlock.h.

To fix this, ensure that asm/spinlock.h always includes qrwlock.h after
defining arch_spin_is_locked (either directly for csky, or via
asm/qspinlock.h for other architectures).  The only inclusion elsewhere
is in kernel/locking/qrwlock.c.  That one is really unnecessary because
the file is only compiled in SMP configurations (config QUEUED_RWLOCKS
depends on SMP) and in that case linux/spinlock.h already includes
asm/qrwlock.h if needed, via asm/spinlock.h.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Cc: Waiman Long <longman@redhat.com>
Fixes: 26128cb6c7e6 ("locking/rwlocks: Add contention detection for rwlocks")
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Ben Gardon <bgardon@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2: Fix sparc too.  Add a comment in qrwlock.h itself.
	Remove unnecessary inclusion in kernel/locking/qrwlock.c

 arch/arm64/include/asm/spinlock.h    | 2 +-
 arch/mips/include/asm/spinlock.h     | 2 +-
 arch/sparc/include/asm/spinlock_64.h | 2 +-
 arch/xtensa/include/asm/spinlock.h   | 2 +-
 include/asm-generic/qrwlock.h        | 3 ++-
 kernel/locking/qrwlock.c             | 1 -
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index 9083d6992603..0525c0b089ed 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -5,8 +5,8 @@
 #ifndef __ASM_SPINLOCK_H
 #define __ASM_SPINLOCK_H
 
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 /* See include/linux/spinlock.h */
 #define smp_mb__after_spinlock()	smp_mb()
diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
index 8a88eb265516..6ce2117e49f6 100644
--- a/arch/mips/include/asm/spinlock.h
+++ b/arch/mips/include/asm/spinlock.h
@@ -10,7 +10,6 @@
 #define _ASM_SPINLOCK_H
 
 #include <asm/processor.h>
-#include <asm/qrwlock.h>
 
 #include <asm-generic/qspinlock_types.h>
 
@@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
 }
 
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #endif /* _ASM_SPINLOCK_H */
diff --git a/arch/sparc/include/asm/spinlock_64.h b/arch/sparc/include/asm/spinlock_64.h
index 7fc82a233f49..3a9a0b0c7465 100644
--- a/arch/sparc/include/asm/spinlock_64.h
+++ b/arch/sparc/include/asm/spinlock_64.h
@@ -11,8 +11,8 @@
 
 #include <asm/processor.h>
 #include <asm/barrier.h>
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #endif /* !(__ASSEMBLY__) */
 
diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
index 584b0de6f2ca..41c449ece2d8 100644
--- a/arch/xtensa/include/asm/spinlock.h
+++ b/arch/xtensa/include/asm/spinlock.h
@@ -12,8 +12,8 @@
 #define _XTENSA_SPINLOCK_H
 
 #include <asm/barrier.h>
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #define smp_mb__after_spinlock()	smp_mb()
 
diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 0020d3b820a7..7ae0ece07b4e 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -14,7 +14,8 @@
 #include <asm/processor.h>
 
 #include <asm-generic/qrwlock_types.h>
-#include <asm-generic/qspinlock.h>
+
+/* Must be included from asm/spinlock.h after defining arch_spin_is_locked.  */
 
 /*
  * Writer states & reader shift and bias.
diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
index fe9ca92faa2a..4786dd271b45 100644
--- a/kernel/locking/qrwlock.c
+++ b/kernel/locking/qrwlock.c
@@ -12,7 +12,6 @@
 #include <linux/percpu.h>
 #include <linux/hardirq.h>
 #include <linux/spinlock.h>
-#include <asm/qrwlock.h>
 
 /**
  * queued_read_lock_slowpath - acquire read lock of a queue rwlock
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h
@ 2021-02-10 18:33 ` Paolo Bonzini
  0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2021-02-10 18:33 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: open list:GENERIC INCLUDE/ASM HEADER FILES, Chris Zankel,
	Thomas Bogendoerfer, Arnd Bergmann, Davidlohr Bueso,
	Peter Zijlstra, Catalin Marinas,
	open list:TENSILICA XTENSA PORT xtensa, open list:MIPS,
	open list:C-SKY ARCHITECTURE, Max Filippov, Ingo Molnar, Guo Ren,
	moderated list:ARM64 PORT AARCH64 ARCHITECTURE, Ben Gardon,
	open list:SPARC + UltraSPARC sparc/sparc64, Waiman Long,
	Will Deacon, David S. Miller, Guenter Roeck

include/asm-generic/qrwlock.h was trying to get arch_spin_is_locked via
asm-generic/qspinlock.h.  However, this does not work because architectures
might be using queued rwlocks but not queued spinlocks (csky), or because they
might be defining their own queued_* macros before including asm/qspinlock.h.

To fix this, ensure that asm/spinlock.h always includes qrwlock.h after
defining arch_spin_is_locked (either directly for csky, or via
asm/qspinlock.h for other architectures).  The only inclusion elsewhere
is in kernel/locking/qrwlock.c.  That one is really unnecessary because
the file is only compiled in SMP configurations (config QUEUED_RWLOCKS
depends on SMP) and in that case linux/spinlock.h already includes
asm/qrwlock.h if needed, via asm/spinlock.h.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Cc: Waiman Long <longman@redhat.com>
Fixes: 26128cb6c7e6 ("locking/rwlocks: Add contention detection for rwlocks")
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Ben Gardon <bgardon@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2: Fix sparc too.  Add a comment in qrwlock.h itself.
	Remove unnecessary inclusion in kernel/locking/qrwlock.c

 arch/arm64/include/asm/spinlock.h    | 2 +-
 arch/mips/include/asm/spinlock.h     | 2 +-
 arch/sparc/include/asm/spinlock_64.h | 2 +-
 arch/xtensa/include/asm/spinlock.h   | 2 +-
 include/asm-generic/qrwlock.h        | 3 ++-
 kernel/locking/qrwlock.c             | 1 -
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index 9083d6992603..0525c0b089ed 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -5,8 +5,8 @@
 #ifndef __ASM_SPINLOCK_H
 #define __ASM_SPINLOCK_H
 
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 /* See include/linux/spinlock.h */
 #define smp_mb__after_spinlock()	smp_mb()
diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
index 8a88eb265516..6ce2117e49f6 100644
--- a/arch/mips/include/asm/spinlock.h
+++ b/arch/mips/include/asm/spinlock.h
@@ -10,7 +10,6 @@
 #define _ASM_SPINLOCK_H
 
 #include <asm/processor.h>
-#include <asm/qrwlock.h>
 
 #include <asm-generic/qspinlock_types.h>
 
@@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
 }
 
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #endif /* _ASM_SPINLOCK_H */
diff --git a/arch/sparc/include/asm/spinlock_64.h b/arch/sparc/include/asm/spinlock_64.h
index 7fc82a233f49..3a9a0b0c7465 100644
--- a/arch/sparc/include/asm/spinlock_64.h
+++ b/arch/sparc/include/asm/spinlock_64.h
@@ -11,8 +11,8 @@
 
 #include <asm/processor.h>
 #include <asm/barrier.h>
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #endif /* !(__ASSEMBLY__) */
 
diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
index 584b0de6f2ca..41c449ece2d8 100644
--- a/arch/xtensa/include/asm/spinlock.h
+++ b/arch/xtensa/include/asm/spinlock.h
@@ -12,8 +12,8 @@
 #define _XTENSA_SPINLOCK_H
 
 #include <asm/barrier.h>
-#include <asm/qrwlock.h>
 #include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
 
 #define smp_mb__after_spinlock()	smp_mb()
 
diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 0020d3b820a7..7ae0ece07b4e 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -14,7 +14,8 @@
 #include <asm/processor.h>
 
 #include <asm-generic/qrwlock_types.h>
-#include <asm-generic/qspinlock.h>
+
+/* Must be included from asm/spinlock.h after defining arch_spin_is_locked.  */
 
 /*
  * Writer states & reader shift and bias.
diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
index fe9ca92faa2a..4786dd271b45 100644
--- a/kernel/locking/qrwlock.c
+++ b/kernel/locking/qrwlock.c
@@ -12,7 +12,6 @@
 #include <linux/percpu.h>
 #include <linux/hardirq.h>
 #include <linux/spinlock.h>
-#include <asm/qrwlock.h>
 
 /**
  * queued_read_lock_slowpath - acquire read lock of a queue rwlock
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-02-11 15:30 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10 14:45 [PATCH] locking/arch: Move qrwlock.h include after qspinlock.h Waiman Long
2021-02-10 14:45 ` Waiman Long
2021-02-10 15:05 ` Guenter Roeck
2021-02-10 15:05   ` Guenter Roeck
2021-02-10 15:53   ` Waiman Long
2021-02-10 15:53     ` Waiman Long
2021-02-10 17:33     ` Ben Gardon
2021-02-10 17:33       ` Ben Gardon
2021-02-10 16:19 ` Thomas Bogendoerfer
2021-02-10 16:19   ` Thomas Bogendoerfer
2021-02-11 12:59   ` Paolo Bonzini
2021-02-11 12:59     ` Paolo Bonzini
2021-02-11 14:41     ` Thomas Bogendoerfer
2021-02-11 14:41       ` Thomas Bogendoerfer
2021-02-10 18:28 ` Paolo Bonzini
2021-02-10 18:28   ` Paolo Bonzini
2021-02-10 18:50   ` Waiman Long
2021-02-10 18:50     ` Waiman Long
2021-02-10 18:33 Paolo Bonzini
2021-02-10 18:33 ` Paolo Bonzini
2021-02-10 18:33 ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.