spin_lock_*(): Always evaluate second argument

PATCH RESEND Superseded Bart Van Assche 5 participants 11 messages Jul 22, 2014 – Aug 13, 2014 Locking primitives

Share & tools

Bart Van Assche

author

2014-07-22 07:43 UTC #0 lore raw

PATCH RESEND 1/1 spin_lock_*(): Always evaluate second argument

Evaluating a macro argument only if certain configuration options have been selected is confusing and error-prone. Hence always evaluate the second argument of spin_lock_nested() and spin_lock_nest_lock().

An intentional side effect of this patch is that it avoids that the following warning is reported for netif_addr_lock_nested() when building with CONFIG_DEBUG_LOCK_ALLOC=n and with W=1:

include/linux/netdevice.h: In function 'netif_addr_lock_nested':
include/linux/netdevice.h:2865:6: warning: variable 'subclass' set but not used [-Wunused-but-set-variable]

  int subclass = SINGLE_DEPTH_NESTING;
      ^

Signed-off-by Bart Van Assche <[email protected]>

Acked-by David Rientjes <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc David S. Miller <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/spinlock.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

include/linux/spinlock.h +4 -2

index 3f2867f..32b16cc 100644
@@ -197,8 +197,10 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
-# define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
+# define raw_spin_lock_nested(lock, subclass)		\
+	((void)(subclass), _raw_spin_lock(lock))
+# define raw_spin_lock_nest_lock(lock, nest_lock)	\
+	((void)(nest_lock), _raw_spin_lock(lock))
 #endif
 
 #if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)

Signature · 1 line

1.8.4.5

Andrew Morton

2014-07-22 20:37 UTC #1 lore raw

Quoted 15 lines · On Tue, 22 Jul 2014 09:17:45 +0200 Bart Van Assche <[email protected]> wrote:

Evaluating a macro argument only if certain configuration options have been selected is confusing and error-prone. Hence always evaluate the second argument of spin_lock_nested() and spin_lock_nest_lock().
An intentional side effect of this patch is that it avoids that the following warning is reported for netif_addr_lock_nested() when building with CONFIG_DEBUG_LOCK_ALLOC=n and with W=1:
...
include/linux/spinlock.h +0 -0
@@ -197,8 +197,10 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\

include/linux/spinlock.h static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) · line 205

Quoted 10 lines

include/linux/spinlock.h +4 -2

 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
-# define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
+# define raw_spin_lock_nested(lock, subclass)		\
+	((void)(subclass), _raw_spin_lock(lock))
+# define raw_spin_lock_nest_lock(lock, nest_lock)	\
+	((void)(nest_lock), _raw_spin_lock(lock))
 #endif

Did you try converting these to static inline functions? That should squish the warning and makes the code nicer instead of nastier...

David Rientjes

2014-07-22 23:16 UTC #2 lore raw

Quoted 20 lines · Andrew Morton

Quoted 20 lines
Evaluating a macro argument only if certain configuration options have been selected is confusing and error-prone. Hence always evaluate the second argument of spin_lock_nested() and spin_lock_nest_lock().
An intentional side effect of this patch is that it avoids that the following warning is reported for netif_addr_lock_nested() when building with CONFIG_DEBUG_LOCK_ALLOC=n and with W=1:
...
include/linux/spinlock.h +1 -2
@@ -197,8 +197,10 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
-# define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
+# define raw_spin_lock_nested(lock, subclass)		\

Quoted 9 lines

include/linux/spinlock.h static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) · line 205
Quoted 5 lines
include/linux/spinlock.h +3 -0
+	((void)(subclass), _raw_spin_lock(lock))
+# define raw_spin_lock_nest_lock(lock, nest_lock)	\
+	((void)(nest_lock), _raw_spin_lock(lock))
 #endif
 
Did you try converting these to static inline functions? That should squish the warning and makes the code nicer instead of nastier...

Not sure how that would be done since _raw_spin_lock isn't declared in this scope.

Taking a second look, however, I think the patch doesn't need to modify raw_spin_lock_nest_lock() for the problem being reported and evaluating the parameter of type struct lockdep_map * probably is meaningless.

Bart, is it possible to just get away with the raw_spin_lock_nested() change?

Bart Van Assche

author

2014-08-06 17:27 UTC #3 lore raw

Quoted 33 lines · David Rientjes

Quoted 20 lines · Andrew Morton
Quoted 20 lines
Evaluating a macro argument only if certain configuration options have been selected is confusing and error-prone. Hence always evaluate the second argument of spin_lock_nested() and spin_lock_nest_lock().
An intentional side effect of this patch is that it avoids that the following warning is reported for netif_addr_lock_nested() when building with CONFIG_DEBUG_LOCK_ALLOC=n and with W=1:
...
include/linux/spinlock.h +1 -2
@@ -197,8 +197,10 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
-# define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
+# define raw_spin_lock_nested(lock, subclass)		\
Quoted 8 lines
include/linux/spinlock.h static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) · line 205
Quoted 5 lines
include/linux/spinlock.h +3 -0
+	((void)(subclass), _raw_spin_lock(lock))
+# define raw_spin_lock_nest_lock(lock, nest_lock)	\
+	((void)(nest_lock), _raw_spin_lock(lock))
 #endif
 
Did you try converting these to static inline functions? That should squish the warning and makes the code nicer instead of nastier...
Not sure how that would be done since _raw_spin_lock isn't declared in this scope.

Quoted 6 lines

Taking a second look, however, I think the patch doesn't need to modify raw_spin_lock_nest_lock() for the problem being reported and evaluating the parameter of type struct lockdep_map * probably is meaningless.
Bart, is it possible to just get away with the raw_spin_lock_nested() change?

Probably ... I will post an updated version of this patch.

Bart.

Bart Van Assche

author

2014-08-06 17:54 UTC #4 lore raw

PATCH v2 1/1 spin_lock_nested(): Always evaluate second argument

Evaluating a macro argument only if certain configuration options have been selected is confusing and error-prone. Hence always evaluate the second argument of spin_lock_nested().

An intentional side effect of this patch is that it avoids that the following warning is reported for netif_addr_lock_nested() when building with CONFIG_DEBUG_LOCK_ALLOC=n and with W=1:

include/linux/netdevice.h: In function 'netif_addr_lock_nested':
include/linux/netdevice.h:2865:6: warning: variable 'subclass' set but not used [-Wunused-but-set-variable]

  int subclass = SINGLE_DEPTH_NESTING;
      ^

Signed-off-by Bart Van Assche <[email protected]>

Acked-by David Rientjes <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc David S. Miller <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/spinlock.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

include/linux/spinlock.h +2 -1

index 3f2867f..485d5dc 100644
@@ -197,7 +197,8 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
+# define raw_spin_lock_nested(lock, subclass)		\
+	((void)(subclass), _raw_spin_lock(lock))
 # define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
 #endif

Signature · 1 line

1.8.4.5

David Rientjes

2014-08-06 21:56 UTC #5 lore raw

Re: [PATCH v2] spin_lock_nested(): Always evaluate second argument

Quoted 5 lines · Bart Van Assche

include/linux/spinlock.h +0 -0
index 3f2867f..485d5dc 100644

include/linux/spinlock.h static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) · line 204

Quoted 9 lines

include/linux/spinlock.h +2 -1

@@ -197,7 +197,8 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
+# define raw_spin_lock_nested(lock, subclass)		\
+	((void)(subclass), _raw_spin_lock(lock))
 # define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
 #endif

I think it would be nice to comment why we're evaluating the subclass when CONFIG_DEBUG_LOCK_ALLOC=n so that nobody comes along and unknowingly changes it back.

Thanks for posting the updated version and keeping at it!

Bart Van Assche

author

2014-08-08 10:32 UTC #6 lore raw

Re: [PATCH v2] spin_lock_nested(): Always evaluate second argument

Quoted 20 lines · David Rientjes

Quoted 5 lines · Bart Van Assche
include/linux/spinlock.h +0 -0
index 3f2867f..485d5dc 100644
include/linux/spinlock.h static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) · line 204
Quoted 9 lines
include/linux/spinlock.h +2 -1
@@ -197,7 +197,8 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
+# define raw_spin_lock_nested(lock, subclass)		\
+	((void)(subclass), _raw_spin_lock(lock))
 # define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
 #endif
 
I think it would be nice to comment why we're evaluating the subclass when CONFIG_DEBUG_LOCK_ALLOC=n so that nobody comes along and unknowingly changes it back.

Quoted 1 line

Thanks for posting the updated version and keeping at it!

This morning I received an e-mail from Wu Fengguang reporting that this patch breaks the build for CONFIG_SMP=n. I will post a third version.

Bart.

Bart Van Assche

author

2014-08-08 10:35 UTC #7 lore raw

PATCH v3 1/1 spin_lock_nested(): Always evaluate second argument

Evaluating a macro argument only if certain configuration options have been selected is confusing and error-prone. Hence always evaluate the second argument of spin_lock_nested().

An intentional side effect of this patch is that it avoids that the following warning is reported for netif_addr_lock_nested() when building with CONFIG_DEBUG_LOCK_ALLOC=n and with W=1:

include/linux/netdevice.h: In function 'netif_addr_lock_nested':
include/linux/netdevice.h:2865:6: warning: variable 'subclass' set but not used [-Wunused-but-set-variable]

  int subclass = SINGLE_DEPTH_NESTING;
      ^

Signed-off-by Bart Van Assche <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc David Rientjes <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc David S. Miller <[email protected]>

Cc Andrew Morton <[email protected]>

---
 include/linux/spinlock.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

include/linux/spinlock.h +7 -1

index 3f2867f..262ba4e 100644
@@ -197,7 +197,13 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
+/*
+ * Always evaluate the 'subclass' argument to avoid that the compiler
+ * warns about set-but-not-used variables when building with
+ * CONFIG_DEBUG_LOCK_ALLOC=n and with W=1.
+ */
+# define raw_spin_lock_nested(lock, subclass)		\
+	_raw_spin_lock(((void)(subclass), (lock)))
 # define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
 #endif

Signature · 1 line

1.8.4.5

David Rientjes

2014-08-08 21:52 UTC #8 lore raw

Re: [PATCH v3] spin_lock_nested(): Always evaluate second argument

Quoted 5 lines · Bart Van Assche

include/linux/spinlock.h +0 -0
index 3f2867f..262ba4e 100644

include/linux/spinlock.h static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) · line 204

Quoted 9 lines

include/linux/spinlock.h +5 -1

@@ -197,7 +197,13 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
+/*
+ * Always evaluate the 'subclass' argument to avoid that the compiler
+ * warns about set-but-not-used variables when building with
+ * CONFIG_DEBUG_LOCK_ALLOC=n and with W=1.
+ */

I was hoping there was going to be a more important reason for this change than to avoid compiler warnings, such as an example where someone is doing spin_lock_nested(lock, subclass) and the expression for "subclass" requires evaluation in all configs.

Quoted 5 lines

include/linux/spinlock.h +2 -0

+# define raw_spin_lock_nested(lock, subclass)		\
+	_raw_spin_lock(((void)(subclass), (lock)))
 # define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
 #endif

Peter Zijlstra

maintainer

2014-08-09 06:17 UTC #9 lore raw

Re: [PATCH v3] spin_lock_nested(): Always evaluate second argument

Quoted 13 lines · David Rientjes

Quoted 11 lines · Bart Van Assche

include/linux/spinlock.h +2 -1

index 3f2867f..262ba4e 100644
@@ -197,7 +197,13 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
+/*
+ * Always evaluate the 'subclass' argument to avoid that the compiler

Quoted 8 lines

include/linux/spinlock.h static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) · line 204
Quoted 3 lines
include/linux/spinlock.h +3 -0
+ * warns about set-but-not-used variables when building with
+ * CONFIG_DEBUG_LOCK_ALLOC=n and with W=1.
+ */
I was hoping there was going to be a more important reason for this change than to avoid compiler warnings, such as an example where someone is doing spin_lock_nested(lock, subclass) and the expression for "subclass" requires evaluation in all configs.

That would stink, having that argument have side effects. I'd call that a plain old bug that needs fixing.

tip-bot for Bart Van Assche

merge bot

2014-08-13 10:57 UTC #10 lore raw

[tip:locking/core] locking/spinlocks: Always evaluate the second argument of spin_lock_nested()

In reply to Bart Van Assche

locking/spinlocks: Always evaluate the second argument of spin_lock_nested()

Commit-ID:  4999201a59ef555f9105d2bb2459ed895627f7aa
Gitweb:     http://git.kernel.org/tip/4999201a59ef555f9105d2bb2459ed895627f7aa
Author:     Bart Van Assche <[email protected]>
AuthorDate: Fri, 8 Aug 2014 12:35:36 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Aug 2014 10:32:38 +0200

locking/spinlocks: Always evaluate the second argument of spin_lock_nested()

Evaluating a macro argument only if certain configuration options have been selected is confusing and error-prone. Hence always evaluate the second argument of spin_lock_nested().

An intentional side effect of this patch is that it avoids that the following warning is reported for netif_addr_lock_nested() when building with CONFIG_DEBUG_LOCK_ALLOC=n and with W=1:

include/linux/netdevice.h: In function 'netif_addr_lock_nested':
include/linux/netdevice.h:2865:6: warning: variable 'subclass' set but not used [-Wunused-but-set-variable]

    int subclass = SINGLE_DEPTH_NESTING;
        ^

Signed-off-by Bart Van Assche <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Cc David Rientjes <[email protected]>

Cc David S. Miller <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Paul E. McKenney <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 include/linux/spinlock.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

include/linux/spinlock.h +7 -1

index 3f2867f..262ba4e 100644
@@ -197,7 +197,13 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock)
 		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
 	 } while (0)
 #else
-# define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
+/*
+ * Always evaluate the 'subclass' argument to avoid that the compiler
+ * warns about set-but-not-used variables when building with
+ * CONFIG_DEBUG_LOCK_ALLOC=n and with W=1.
+ */
+# define raw_spin_lock_nested(lock, subclass)		\
+	_raw_spin_lock(((void)(subclass), (lock)))
 # define raw_spin_lock_nest_lock(lock, nest_lock)	_raw_spin_lock(lock)
 #endif