[PATCH] qspinlock: use signed temporaries for cmpxchg

[PATCH] qspinlock: use signed temporaries for cmpxchg

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
 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(&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;
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(&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;
-- 
2.27.0

Re: [PATCH] qspinlock: use signed temporaries for cmpxchg

[Peter Zijlstra]

On Mon, Oct 26, 2020 at 05:57:51PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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.

Re: [PATCH] qspinlock: use signed temporaries for cmpxchg

[Waiman Long]

On 10/26/20 12:57 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> 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 <arnd@arndb.de>
> ---
>   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(&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;
> 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(&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;

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

[Peter Zijlstra]

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 <arnd@arndb.de>
> > 
> > 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(&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.

Re: [PATCH] qspinlock: use signed temporaries for cmpxchg

[Arnd Bergmann]

On Tue, Oct 27, 2020 at 8:47 AM Peter Zijlstra <peterz@infradead.org> 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

[Peter Zijlstra]

On Tue, Oct 27, 2020 at 09:33:32AM +0100, Arnd Bergmann wrote:
> On Tue, Oct 27, 2020 at 8:47 AM Peter Zijlstra <peterz@infradead.org> 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

[David Laight]

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 <peterz@infradead.org> 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

[Arnd Bergmann]

On Tue, Oct 27, 2020 at 11:32 AM Peter Zijlstra <peterz@infradead.org> 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 <peterz@infradead.org> 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-arnd@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

[Peter Zijlstra]

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!