Skip to contentSkip to conversation

locking/arch: Move qrwlock.h include after qspinlock.h

PATCH In review Waiman Long 5 participants 9 messages Feb 10, 2021 – Feb 11, 2021

Subsystems (4)
lore
Share & tools
lore

Waiman Long

author reviewer
#0 lore raw

PATCH 1/1 locking/arch: Move qrwlock.h include after qspinlock.h

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.

---
 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(-)
arch/arm64/include/asm/spinlock.h +1 -1
index 9083d6992603..0525c0b089ed 100644
@@ -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()
arch/mips/include/asm/spinlock.h +1 -1
index 8a88eb265516..6ce2117e49f6 100644
@@ -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 */
arch/xtensa/include/asm/spinlock.h +1 -1
index 584b0de6f2ca..41c449ece2d8 100644
@@ -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()
 
Signature · 7 lines
2.18.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Tested-by on v1 1/1
#1 lore raw
Quoted 10 lines · 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.

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.

Guenter

Quoted 54 lines
---
 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(-)
arch/arm64/include/asm/spinlock.h +1 -1
index 9083d6992603..0525c0b089ed 100644
@@ -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()
arch/mips/include/asm/spinlock.h +1 -1
index 8a88eb265516..6ce2117e49f6 100644
@@ -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 */
arch/xtensa/include/asm/spinlock.h +1 -1
index 584b0de6f2ca..41c449ece2d8 100644
@@ -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()
 
author reviewer
#2 lore raw
Quoted 6 lines · Guenter Roeck
Quoted 5 lines · 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.

Quoted 7 lines
Quoted 5 lines

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.

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.

Quoted 5 lines

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

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

Cheers,
Longman

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Reviewed-by
#3 lore raw
Quoted 24 lines · Waiman Long
Quoted 6 lines · Guenter Roeck
Quoted 5 lines · 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.

Quoted 7 lines
Quoted 5 lines

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.

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.

Quoted 4 lines

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

Quoted 7 lines
Quoted 1 line

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.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#4 lore raw
In reply to Waiman Long
Quoted 9 lines · 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.

Quoted 6 lines
---
 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.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]
#5 lore raw
Quoted 6 lines · Thomas Bogendoerfer
Quoted 4 lines
  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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

maintainer Acked-by
#6 lore raw
Quoted 11 lines · Paolo Bonzini
Quoted 6 lines · Thomas Bogendoerfer
Quoted 4 lines
  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

Signature · 7 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#7 lore raw
In reply to Waiman Long
Quoted 10 lines · 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.

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

Quoted 45 lines
---
  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(-)
arch/arm64/include/asm/spinlock.h +1 -1
index 9083d6992603..0525c0b089ed 100644
@@ -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()
arch/mips/include/asm/spinlock.h +1 -1
index 8a88eb265516..6ce2117e49f6 100644
@@ -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 */
arch/xtensa/include/asm/spinlock.h +0 -0
index 584b0de6f2ca..41c449ece2d8 100644

arch/xtensa/include/asm/spinlock.h line 19

Quoted 9 lines
arch/xtensa/include/asm/spinlock.h +1 -1
@@ -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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

author reviewer
#8 lore raw
Quoted 6 lines · Paolo Bonzini
Quoted 5 lines · 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.

Quoted 7 lines
Quoted 5 lines

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.

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.

Quoted 5 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel