Re: [PATCH] qspinlock: use signed temporaries for cmpxchg
On Tue, Oct 27, 2020 at 05:22:48PM +0100, Arnd Bergmann wrote: > 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!
Re: [PATCH] qspinlock: use signed temporaries for cmpxchg
On Tue, Oct 27, 2020 at 11:32 AM Peter Zijlstra wrote: > > On Tue, Oct 27, 2020 at 09:33:32AM +0100, Arnd Bergmann wrote: > > On Tue, Oct 27, 2020 at 8:47 AM Peter Zijlstra wrote: > > > On Mon, Oct 26, 2020 at 02:03:06PM -0400, Waiman Long wrote: > > > > 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. > > 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/20201026213040.3889546-6-a...@kernel.org/ where changing the types led to also making it 'const'. > 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. > 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
RE: [PATCH] qspinlock: use signed temporaries for cmpxchg
From: Peter Zijlstra > Sent: 27 October 2020 10:33 > > On Tue, Oct 27, 2020 at 09:33:32AM +0100, Arnd Bergmann wrote: > > On Tue, Oct 27, 2020 at 8:47 AM Peter Zijlstra wrote: > > > On Mon, Oct 26, 2020 at 02:03:06PM -0400, Waiman Long wrote: > > > > 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. > > 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? 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)
Re: [PATCH] qspinlock: use signed temporaries for cmpxchg
On Tue, Oct 27, 2020 at 09:33:32AM +0100, Arnd Bergmann wrote: > On Tue, Oct 27, 2020 at 8:47 AM Peter Zijlstra wrote: > > On Mon, Oct 26, 2020 at 02:03:06PM -0400, Waiman Long wrote: > > > 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. 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?
Re: [PATCH] qspinlock: use signed temporaries for cmpxchg
On Tue, Oct 27, 2020 at 8:47 AM Peter Zijlstra wrote: > On Mon, Oct 26, 2020 at 02:03:06PM -0400, Waiman Long wrote: > > 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
Re: [PATCH] qspinlock: use signed temporaries for cmpxchg
On Mon, Oct 26, 2020 at 02:03:06PM -0400, Waiman Long wrote: > On 10/26/20 12:57 PM, Arnd Bergmann wrote: > > From: Arnd Bergmann > > > > 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. Code consistency cares. Fundamentally we're treating it as a u32 here, using int just because of a confused compiler warning will confuse. > > @@ -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(>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.
Re: [PATCH] qspinlock: use signed temporaries for cmpxchg
On 10/26/20 12:57 PM, Arnd Bergmann wrote: From: Arnd Bergmann 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 --- include/asm-generic/qrwlock.h | 8 include/asm-generic/qspinlock.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h index 3aefde23dcea..84ce841ce735 100644 --- a/include/asm-generic/qrwlock.h +++ b/include/asm-generic/qrwlock.h @@ -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(>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(>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, >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(>cnts, , _QW_LOCKED))) return; diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h index 4fe7fd0fe834..d74b13825501 100644 --- a/include/asm-generic/qspinlock.h +++ b/include/asm-generic/qspinlock.h @@ -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(>val); + int val = atomic_read(>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(>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
Re: [PATCH] qspinlock: use signed temporaries for cmpxchg
On Mon, Oct 26, 2020 at 05:57:51PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > 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. > Yuck, no. This is actively wrong. All that code very much wants u32.
[PATCH] qspinlock: use signed temporaries for cmpxchg
From: Arnd Bergmann 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 --- include/asm-generic/qrwlock.h | 8 include/asm-generic/qspinlock.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h index 3aefde23dcea..84ce841ce735 100644 --- a/include/asm-generic/qrwlock.h +++ b/include/asm-generic/qrwlock.h @@ -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(>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(>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, >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(>cnts, , _QW_LOCKED))) return; diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h index 4fe7fd0fe834..d74b13825501 100644 --- a/include/asm-generic/qspinlock.h +++ b/include/asm-generic/qspinlock.h @@ -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(>val); + int val = atomic_read(>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(>val, , _Q_LOCKED_VAL))) return; -- 2.27.0