Skip to contentSkip to conversation

qspinlock: use signed temporaries for cmpxchg

PATCH In review Arnd Bergmann 4 participants 9 messages Oct 26, 2020 – Oct 27, 2020 Generic include/asm header files

lore
Share & tools
lore

Arnd Bergmann

author
#0 lore raw

PATCH 1/1 qspinlock: use signed temporaries for cmpxchg

From: Arnd Bergmann <[email protected]>

When building with W=2, the build log is flooded with

include/asm-generic/qrwlock.h:65:56: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qrwlock.h:92:53: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qspinlock.h:68:55: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qspinlock.h:82:52: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign]

The atomics are built on top of signed integers, but the caller doesn't actually care. Just use signed types as well.

Fixes 27df89689e25 ("locking/spinlocks: Remove an instruction from spin and write locks")
Signed-off-by Arnd Bergmann <[email protected]>
---
 include/asm-generic/qrwlock.h   | 8 ++++----
 include/asm-generic/qspinlock.h | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)
include/asm-generic/qrwlock.h +4 -4
index 3aefde23dcea..84ce841ce735 100644
@@ -37,7 +37,7 @@ extern void queued_write_lock_slowpath(struct qrwlock *lock);
  */
 static inline int queued_read_trylock(struct qrwlock *lock)
 {
-	u32 cnts;
+	int cnts;
 
 	cnts = atomic_read(&lock->cnts);
 	if (likely(!(cnts & _QW_WMASK))) {

@@ -56,7 +56,7 @@ static inline int queued_read_trylock(struct qrwlock *lock)
  */
 static inline int queued_write_trylock(struct qrwlock *lock)
 {
-	u32 cnts;
+	int cnts;
 
 	cnts = atomic_read(&lock->cnts);
 	if (unlikely(cnts))

@@ -71,7 +71,7 @@ static inline int queued_write_trylock(struct qrwlock *lock)
  */
 static inline void queued_read_lock(struct qrwlock *lock)
 {
-	u32 cnts;
+	int cnts;
 
 	cnts = atomic_add_return_acquire(_QR_BIAS, &lock->cnts);
 	if (likely(!(cnts & _QW_WMASK)))

@@ -87,7 +87,7 @@ static inline void queued_read_lock(struct qrwlock *lock)
  */
 static inline void queued_write_lock(struct qrwlock *lock)
 {
-	u32 cnts = 0;
+	int cnts = 0;
 	/* Optimize for the unfair lock case where the fair flag is 0. */
 	if (likely(atomic_try_cmpxchg_acquire(&lock->cnts, &cnts, _QW_LOCKED)))
 		return;
include/asm-generic/qspinlock.h +2 -2
index 4fe7fd0fe834..d74b13825501 100644
@@ -60,7 +60,7 @@ static __always_inline int queued_spin_is_contended(struct qspinlock *lock)
  */
 static __always_inline int queued_spin_trylock(struct qspinlock *lock)
 {
-	u32 val = atomic_read(&lock->val);
+	int val = atomic_read(&lock->val);
 
 	if (unlikely(val))
 		return 0;

@@ -77,7 +77,7 @@ extern void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
  */
 static __always_inline void queued_spin_lock(struct qspinlock *lock)
 {
-	u32 val = 0;
+	int val = 0;
 
 	if (likely(atomic_try_cmpxchg_acquire(&lock->val, &val, _Q_LOCKED_VAL)))
 		return;
Signature · 1 line
2.27.0
#1 lore raw
Quoted 4 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

When building with W=2, the build log is flooded with

Quoted 8 lines

include/asm-generic/qrwlock.h:65:56: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qrwlock.h:92:53: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qspinlock.h:68:55: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qspinlock.h:82:52: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign]

The atomics are built on top of signed integers, but the caller doesn't actually care. Just use signed types as well.

Yuck, no. This is actively wrong. All that code very much wants u32.

#2 lore raw
In reply to Arnd Bergmann
Quoted 73 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

When building with W=2, the build log is flooded with

include/asm-generic/qrwlock.h:65:56: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qrwlock.h:92:53: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qspinlock.h:68:55: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qspinlock.h:82:52: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign]

The atomics are built on top of signed integers, but the caller doesn't actually care. Just use signed types as well.

Fixes 27df89689e25 ("locking/spinlocks: Remove an instruction from spin and write locks")
Signed-off-by Arnd Bergmann <[email protected]>
---
  include/asm-generic/qrwlock.h   | 8 ++++----
  include/asm-generic/qspinlock.h | 4 ++--
  2 files changed, 6 insertions(+), 6 deletions(-)
include/asm-generic/qrwlock.h +4 -4
index 3aefde23dcea..84ce841ce735 100644
@@ -37,7 +37,7 @@ extern void queued_write_lock_slowpath(struct qrwlock *lock);
   */
  static inline int queued_read_trylock(struct qrwlock *lock)
  {
-	u32 cnts;
+	int cnts;
 
  	cnts = atomic_read(&lock->cnts);
  	if (likely(!(cnts & _QW_WMASK))) {

@@ -56,7 +56,7 @@ static inline int queued_read_trylock(struct qrwlock *lock)
   */
  static inline int queued_write_trylock(struct qrwlock *lock)
  {
-	u32 cnts;
+	int cnts;
 
  	cnts = atomic_read(&lock->cnts);
  	if (unlikely(cnts))

@@ -71,7 +71,7 @@ static inline int queued_write_trylock(struct qrwlock *lock)
   */
  static inline void queued_read_lock(struct qrwlock *lock)
  {
-	u32 cnts;
+	int cnts;
 
  	cnts = atomic_add_return_acquire(_QR_BIAS, &lock->cnts);
  	if (likely(!(cnts & _QW_WMASK)))

@@ -87,7 +87,7 @@ static inline void queued_read_lock(struct qrwlock *lock)
   */
  static inline void queued_write_lock(struct qrwlock *lock)
  {
-	u32 cnts = 0;
+	int cnts = 0;
  	/* Optimize for the unfair lock case where the fair flag is 0. */
  	if (likely(atomic_try_cmpxchg_acquire(&lock->cnts, &cnts, _QW_LOCKED)))
  		return;
include/asm-generic/qspinlock.h +1 -1
index 4fe7fd0fe834..d74b13825501 100644
@@ -60,7 +60,7 @@ static __always_inline int queued_spin_is_contended(struct qspinlock *lock)
   */
  static __always_inline int queued_spin_trylock(struct qspinlock *lock)
  {
-	u32 val = atomic_read(&lock->val);
+	int val = atomic_read(&lock->val);
 
  	if (unlikely(val))
  		return 0;

include/asm-generic/qspinlock.h extern void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); · line 83

Quoted 8 lines
include/asm-generic/qspinlock.h +1 -1
@@ -77,7 +77,7 @@ extern void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
   */
  static __always_inline void queued_spin_lock(struct qspinlock *lock)
  {
-	u32 val = 0;
+	int val = 0;
 
  	if (likely(atomic_try_cmpxchg_acquire(&lock->val, &val, _Q_LOCKED_VAL)))
  		return;

Yes, it shouldn't really matter if the value is defined as int or u32. However, the only caveat that I see is queued_spin_lock_slowpath() is expecting a u32 argument. Maybe you should cast it back to (u32) when calling it.

Cheers,
Longman

#3 lore raw
Quoted 5 lines · Waiman Long
Quoted 4 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

When building with W=2, the build log is flooded with

Quoted 7 lines
Quoted 7 lines

include/asm-generic/qrwlock.h:65:56: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qrwlock.h:92:53: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qspinlock.h:68:55: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign] include/asm-generic/qspinlock.h:82:52: warning: pointer targets in passing argument 2 of 'atomic_try_cmpxchg_acquire' differ in signedness [-Wpointer-sign]

The atomics are built on top of signed integers, but the caller doesn't actually care. Just use signed types as well.

Code consistency cares. Fundamentally we're treating it as a u32 here, using int just because of a confused compiler warning will confuse.

Quoted 5 lines
Quoted 5 lines
include/asm-generic/qspinlock.h +0 -1
@@ -77,7 +77,7 @@ extern void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
   */
  static __always_inline void queued_spin_lock(struct qspinlock *lock)
  {
-	u32 val = 0;
Quoted 8 lines

include/asm-generic/qspinlock.h extern void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); · line 83

Quoted 3 lines
include/asm-generic/qspinlock.h +1 -0
+	int val = 0;
  	if (likely(atomic_try_cmpxchg_acquire(&lock->val, &val, _Q_LOCKED_VAL)))
  		return;

Yes, it shouldn't really matter if the value is defined as int or u32. However, the only caveat that I see is queued_spin_lock_slowpath() is expecting a u32 argument. Maybe you should cast it back to (u32) when calling it.

No, we're not going to confuse the code. That stuff is hard enough as it is. This warning is garbage and just needs to stay off.

author
#4 lore raw
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines · Waiman Long

On 10/26/20 12:57 PM, Arnd Bergmann wrote:
Yes, it shouldn't really matter if the value is defined as int or u32. However, the only caveat that I see is queued_spin_lock_slowpath() is expecting a u32 argument. Maybe you should cast it back to (u32) when calling it.

No, we're not going to confuse the code. That stuff is hard enough as it is. This warning is garbage and just needs to stay off.

Ok, so the question then becomes: should we drop -Wpointer-sign from W=2 and move it to W=3, or instead disable it locally. I could add __diag_ignore(GCC, 4, "-Wpointer-sign") in the couple of header files that produce this kind of warning if there is a general feeling that it still helps to have this for drivers.

In the current state, there are a handful of header files that cause 90% of all the W=2 warnings, making it impractical to ever build a driver with W=2 and get anything useful out of it. I find some of the warnings in the set useful in finding actual bugs, but much less so if they are drowned out by noise from known false-positives.

Arnd

#5 lore raw
Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · Peter Zijlstra
Quoted 5 lines · Waiman Long

On 10/26/20 12:57 PM, Arnd Bergmann wrote:
Yes, it shouldn't really matter if the value is defined as int or u32. However, the only caveat that I see is queued_spin_lock_slowpath() is expecting a u32 argument. Maybe you should cast it back to (u32) when calling it.

Quoted 8 lines
Quoted 2 lines

No, we're not going to confuse the code. That stuff is hard enough as it is. This warning is garbage and just needs to stay off.

Ok, so the question then becomes: should we drop -Wpointer-sign from W=2 and move it to W=3, or instead disable it locally. I could add __diag_ignore(GCC, 4, "-Wpointer-sign") in the couple of header files that produce this kind of warning if there is a general feeling that it still helps to have this for drivers.

What is an actual geniune bug that this warning helps find?

Note that the kernel relies on -fno-strict-overflow to get rid of the signed UB that is otherwise present in C.

If you add that __diag_ignore() it should go in atomic.h I suppose, because all of atomic hard relies on this, and then the question becomes how much code is left that doesn't include that header and consequently doesn't ignore that warning.

So, is it useful to begin with in finding actual problems? and given we have to annotate away a bucket-load, how much coverage will there remain if we squish the known false-positives?

#6 lore raw

From: Peter Zijlstra

Quoted 25 lines

Sent: 27 October 2020 10:33

Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · Peter Zijlstra
Quoted 5 lines · Waiman Long

On 10/26/20 12:57 PM, Arnd Bergmann wrote:
Yes, it shouldn't really matter if the value is defined as int or u32. However, the only caveat that I see is queued_spin_lock_slowpath() is expecting a u32 argument. Maybe you should cast it back to (u32) when calling it.

Quoted 8 lines
Quoted 2 lines

No, we're not going to confuse the code. That stuff is hard enough as it is. This warning is garbage and just needs to stay off.

Ok, so the question then becomes: should we drop -Wpointer-sign from W=2 and move it to W=3, or instead disable it locally. I could add __diag_ignore(GCC, 4, "-Wpointer-sign") in the couple of header files that produce this kind of warning if there is a general feeling that it still helps to have this for drivers.

What is an actual geniune bug that this warning helps find?

Note that the kernel relies on -fno-strict-overflow to get rid of the signed UB that is otherwise present in C.

Quoted 8 lines

If you add that __diag_ignore() it should go in atomic.h I suppose, because all of atomic hard relies on this, and then the question becomes how much code is left that doesn't include that header and consequently doesn't ignore that warning.

So, is it useful to begin with in finding actual problems? and given we have to annotate away a bucket-load, how much coverage will there remain if we squish the known false-positives?

Especially since adding casts just makes the code harder to read and can easily hide real bugs.

FWIW you might want to try -Wwrite-strings.
That ought to be fixable by sprinkling 'const.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

author
#7 lore raw
In reply to Peter Zijlstra
Quoted 13 lines · Peter Zijlstra
Quoted 11 lines · Arnd Bergmann
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines · Waiman Long

On 10/26/20 12:57 PM, Arnd Bergmann wrote:
Yes, it shouldn't really matter if the value is defined as int or u32. However, the only caveat that I see is queued_spin_lock_slowpath() is expecting a u32 argument. Maybe you should cast it back to (u32) when calling it.

No, we're not going to confuse the code. That stuff is hard enough as it is. This warning is garbage and just needs to stay off.

Quoted 7 lines
Quoted 5 lines

Ok, so the question then becomes: should we drop -Wpointer-sign from W=2 and move it to W=3, or instead disable it locally. I could add __diag_ignore(GCC, 4, "-Wpointer-sign") in the couple of header files that produce this kind of warning if there is a general feeling that it still helps to have this for drivers.

What is an actual geniune bug that this warning helps find?

I've gone through the git history to find something mentioning this warning, but there was no evidence of a real bug that could have been prevented with this warning, and lots of work wasted on shutting up the compiler.

The best case was
https://lore.kernel.org/lkml/[email protected]/
where changing the types led to also making it 'const'.

Quoted 4 lines

If you add that __diag_ignore() it should go in atomic.h I suppose, because all of atomic hard relies on this, and then the question becomes how much code is left that doesn't include that header and consequently doesn't ignore that warning.

I don't think it would work: the __diag_ignore() has to be in the caller, not the function that is called.

Quoted 3 lines

So, is it useful to begin with in finding actual problems? and given we have to annotate away a bucket-load, how much coverage will there remain if we squish the known false-positives?

In an x86 allmodconfig build, I see 113618 -Wpointer-sign warnings, 68318 of those in qspinlock.h and qrwlock.h. With the six patches I sent, the total number goes down to 15201, which of course is still fairly pointless to go through. Almost all of these are in drivers that have less than 10 warnings, and few of them are in headers included by other drivers.

I looked at the top remaining files, but couldn't find any actual bugs there. If there are real bugs, they are certainly hard to find among the false positives.

I have already sent patches to move -Wnested-externs and -Wcast-align from W=2 to W=3, and I guess -Wpointer-sign could be handled the same way to make the W=2 level useful again.

Arnd

----
1764 ../drivers/staging/rtl8723bs/hal/HalHWImg8723B_RF.c

    810 ../drivers/net/wireless/ath/ath11k/debugfs_htt_stats.c
    411 ../include/linux/moduleparam.h
    230 ../drivers/net/ethernet/neterion/vxge/vxge-ethtool.h
    184 ../include/linux/nls.h
    150 ../drivers/scsi/esas2r/esas2r.h
    146 ../include/net/tls.h
    144 ../sound/soc/codecs/wm5100.c
    135 ../drivers/scsi/ufs/ufs-sysfs.c
    130 ../include/sound/hda_regmap.h
    125 ../drivers/scsi/myrs.c
    121 ../include/linux/fscrypt.h
    113 ../drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
    105 ../drivers/net/wireless/ath/ath9k/hw.h
     81 ../drivers/staging/media/allegro-dvt/nal-h264.c
     75 ../drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
     68 ../drivers/scsi/esas2r/esas2r_init.c
     56 ../sound/soc/sof/ipc.c
     56 ../drivers/staging/qlge/qlge_dbg.c
     50 ../sound/usb/mixer.c
     50 ../drivers/net/ethernet/brocade/bna/bnad_ethtool.c
     50 ../drivers/isdn/capi/capiutil.c
     47 ../fs/nfs/internal.h
     46 ../drivers/scsi/esas2r/esas2r_int.c
     45 ../drivers/scsi/qla2xxx/qla_init.c
     44 ../drivers/platform/x86/sony-laptop.c
     44 ../drivers/lightnvm/pblk.h
     43 ../drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c
     43 ../drivers/media/pci/cx25821/cx25821-medusa-video.c
     42 ../sound/pci/au88x0/au88x0_core.c
#8 lore raw
Quoted 4 lines · Arnd Bergmann

I have already sent patches to move -Wnested-externs and -Wcast-align from W=2 to W=3, and I guess -Wpointer-sign could be handled the same way to make the W=2 level useful again.

Works for me ;-), thanks!