Add lockdep_assert_not_held()
cover Add lockdep_assert_not_held()
Some kernel functions must not be called holding a specific lock. Doing so could lead to locking problems. Currently these routines call lock_is_held() to check for lock hold followed by WARN_ON.
Adding a common lockdep interface will help reduce the duplication of this logic in the rest of the kernel.
Add lockdep_assert_not_held() to be used in these functions to detect incorrect calls while holding a lock.
lockdep_assert_not_held() provides the opposite functionality of lockdep_assert_held() which is used to assert calls that require holding a specific lock.
The need for lockdep_assert_not_held() came up in a discussion on ath10k patch. ath10k_drain_tx() and i915_vma_pin_ww() are examples of functions that can use lockdep_assert_not_held().
This patch series adds lockdep_assert_not_held() and uses it in the second patch in ath10k_drain_tx() function.
Shuah Khan (2):
lockdep: add lockdep_assert_not_held()
ath10k: detect conf_mutex held ath10k_drain_tx() calls
drivers/net/wireless/ath/ath10k/mac.c | 2 ++ include/linux/lockdep.h | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletion(-)
Signature · 1 line
2.27.0
PATCH 1/2 lockdep: add lockdep_assert_not_held()
Some kernel functions must not be called holding a specific lock. Doing so could lead to locking problems. Currently these routines call lock_is_held() to check for lock hold followed by WARN_ON.
Adding a common lockdep interface will help reduce the duplication of this logic in the rest of the kernel.
Add lockdep_assert_not_held() to be used in these functions to detect incorrect calls while holding a lock.
lockdep_assert_not_held() provides the opposite functionality of lockdep_assert_held() which is used to assert calls that require holding a specific lock.
The need for lockdep_assert_not_held() came up in a discussion on ath10k patch. ath10k_drain_tx() and i915_vma_pin_ww() are examples of functions that can use lockdep_assert_not_held().
--- include/linux/lockdep.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
include/linux/lockdep.h +6 -1
@@ -294,6 +294,10 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);#define lockdep_depth (tsk) (debug_locks ? (tsk)->lockdep_depth : 0) +#define lockdep_assert_not_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l)); \ + } while (0) +#define lockdep_assert_held (l) do { \ WARN_ON(debug_locks && !lockdep_is_held(l)); \ } while (0)
@@ -383,8 +387,9 @@ extern int lock_is_held(const void *);extern int lockdep_is_held (const void *);#define lockdep_is_held_type (l, r) (1) +#define lockdep_assert_not_held (l) do { (void)(l); } while (0)#define lockdep_assert_held (l) do { (void)(l); } while (0) -#define lockdep_assert_held_write (l) do { (void)(l); } while (0) +#define lockdep_assert_held_write (l) do { (void)(l); } while (0)#define lockdep_assert_held_read (l) do { (void)(l); } while (0)#define lockdep_assert_held_once (l) do { (void)(l); } while (0)
Signature · 7 lines
2.27.0 _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k
Peter Zijlstra
maintainerRe: [PATCH 1/2] lockdep: add lockdep_assert_not_held()
include/linux/lockdep.h extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); · line 300
Quoted 4 lines · Shuah Khan
include/linux/lockdep.h +4
-0+#define lockdep_assert_not_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l)); \ + } while (0) +
This thing isn't as straight forward as you might think, but it'll mostly work.
Notably this thing will misfire when lockdep_off() is employed. It certainyl needs a comment to explain the subtleties.
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Peter Zijlstra
maintainerRe: [PATCH 1/2] lockdep: add lockdep_assert_not_held()
Quoted 10 lines · Peter Zijlstra
Review
Quoted 4 lines · Shuah Khan
(quoted diff) +4
-0+#define lockdep_assert_not_held(l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l)); \ + } while (0) +This thing isn't as straight forward as you might think, but it'll mostly work.
Quoted 2 lines
Notably this thing will misfire when
lockdep_off()is employed. It certainyl needs a comment to explain the subtleties.
I think something like so will work, but please double check.
include/linux/lockdep.h +7 -3
@@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);#define lockdep_depth (tsk) (debug_locks ? (tsk)->lockdep_depth : 0) -#define lockdep_assert_held (l) do { \ - WARN_ON(debug_locks && !lockdep_is_held(l)); \ +#define lockdep_assert_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 0)); \ } while (0) -#define lockdep_assert_held_write (l) do { \ +#define lockdep_assert_not_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 1)); \ + } while (0) + +#define lockdep_assert_held_write (l) do { \ WARN_ON(debug_locks && !lockdep_is_held_type(l, 0)); \ } while (0)
kernel/locking/lockdep.c +1 -1
@@ -5467,7 +5467,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)int ret =0 ;if (unlikely (!lockdep_enabled ())) -return 1 ;/* avoid false negative lockdep_assert_held() */ +return -1 ;/* avoid false negative lockdep_assert_held() */ raw_local_irq_save (flags);check_flags (flags);
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 1/2] lockdep: add lockdep_assert_not_held()
Quoted 2 lines · Peter Zijlstra
I think something like so will work, but please double check.
Yeah, that looks better.
include/linux/lockdep.h extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); · line 299
Quoted 10 lines
+++
b/include/linux/lockdep.hinclude/linux/lockdep.h +2
-2@@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);#define lockdep_depth (tsk) (debug_locks ? (tsk)->lockdep_depth : 0) -#define lockdep_assert_held (l) do { \ - WARN_ON(debug_locks && !lockdep_is_held(l)); \ +#define lockdep_assert_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 0)); \ } while (0)
That doesn't really need to change? It's the same.
Quoted 14 lines
include/linux/lockdep.h +5
-1-#define lockdep_assert_held_write (l) do { \ +#define lockdep_assert_not_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 1)); \ + } while (0) + +#define lockdep_assert_held_write (l) do { \ WARN_ON(debug_locks && !lockdep_is_held_type(l, 0)); \ } while (0)kernel/locking/lockdep.c renamed from kernel/locking/lockdep. +0
-0
kernel/locking/lockdep.c noinstr int lock_is_held_type(const struct lockdep_map *lock, int read) · line 5470
Quoted 5 lines
kernel/locking/lockdep.c renamed from kernel/locking/lockdep. +1
-1@@ -5467,7 +5467,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)int ret =0 ;if (unlikely (!lockdep_enabled ())) -return 1 ;/* avoid false negative lockdep_assert_held() */ +return -1 ;/* avoid false negative lockdep_assert_held() */
Maybe add lockdep_assert_not_held() to the comment, to explain the -1 (vs non-zero)?
johannes
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Peter Zijlstra
maintainerRe: [PATCH 1/2] lockdep: add lockdep_assert_not_held()
Quoted 11 lines · Johannes Berg
Quoted 2 lines · Peter Zijlstra
I think something like so will work, but please double check.
Yeah, that looks better.
Quoted 5 lines
+++
b/include/linux/lockdep.hinclude/linux/lockdep.h +0
-0@@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);#define lockdep_depth (tsk) (debug_locks ? (tsk)->lockdep_depth : 0)
Quoted 7 lines
include/linux/lockdep.h extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); · line 299
Quoted 5 lines
include/linux/lockdep.h +2
-2-#define lockdep_assert_held (l) do { \ - WARN_ON(debug_locks && !lockdep_is_held(l)); \ +#define lockdep_assert_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 0)); \ } while (0)That doesn't really need to change? It's the same.
Correct, but I found it more symmetric vs the not implementation below.
Quoted 16 lines
Quoted 16 lines
include/linux/lockdep.h +5
-1-#define lockdep_assert_held_write (l) do { \ +#define lockdep_assert_not_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 1)); \ + } while (0) + +#define lockdep_assert_held_write (l) do { \ WARN_ON(debug_locks && !lockdep_is_held_type(l, 0)); \ } while (0)kernel/locking/lockdep.c renamed from kernel/locking/lockdep. +0
-0@@ -5467,7 +5467,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)int ret =0 ;
Quoted 6 lines
kernel/locking/lockdep.c noinstr int lock_is_held_type(const struct lockdep_map *lock, int read) · line 5470
Quoted 3 lines
kernel/locking/lockdep.c renamed from kernel/locking/lockdep. +1
-1if (unlikely (!lockdep_enabled ())) -return 1 ;/* avoid false negative lockdep_assert_held() */ +return -1 ;/* avoid false negative lockdep_assert_held() */ Maybe add
lockdep_assert_not_held()to the comment, to explain the -1 (vs non-zero)?
Yeah, or frob a '*' in there.
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 1/2] lockdep: add lockdep_assert_not_held()
Quoted 19 lines · Peter Zijlstra
Quoted 10 lines · Johannes Berg
Quoted 1 line · Peter Zijlstra
I think something like so will work, but please double check.
Yeah, that looks better.
Quoted 5 lines
+++
b/include/linux/lockdep.hinclude/linux/lockdep.h +0
-0@@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);#define lockdep_depth (tsk) (debug_locks ? (tsk)->lockdep_depth : 0)Quoted 7 lines
include/linux/lockdep.h extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); · line 299
Quoted 5 lines
include/linux/lockdep.h +2
-2-#define lockdep_assert_held (l) do { \ - WARN_ON(debug_locks && !lockdep_is_held(l)); \ +#define lockdep_assert_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 0)); \ } while (0)That doesn't really need to change? It's the same.
Quoted 1 line
Correct, but I found it more symmetric vs the not implementation below.
Fair enough. One might argue that you should have an
enum lockdep_lock_state {
LOCK_STATE_NOT_HELD, /* 0 now */
LOCK_STATE_HELD, /* 1 now */
LOCK_STATE_UNKNOWN, /* -1 with your patch but might as well be 2 */
};:)
johannes
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 1/2] lockdep: add lockdep_assert_not_held()
Quoted 24 lines · Johannes Berg
Quoted 19 lines · Peter Zijlstra
Quoted 10 lines · Johannes Berg
Quoted 1 line · Peter Zijlstra
I think something like so will work, but please double check.
Yeah, that looks better.
Quoted 5 lines
+++
b/include/linux/lockdep.hinclude/linux/lockdep.h +0
-0@@ -294,11 +294,15 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);#define lockdep_depth (tsk) (debug_locks ? (tsk)->lockdep_depth : 0)Quoted 7 lines
include/linux/lockdep.h extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); · line 299
Quoted 5 lines
include/linux/lockdep.h +2
-2-#define lockdep_assert_held (l) do { \ - WARN_ON(debug_locks && !lockdep_is_held(l)); \ +#define lockdep_assert_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 0)); \ } while (0)That doesn't really need to change? It's the same.
Quoted 1 line
Correct, but I found it more symmetric vs the not implementation below.
Fair enough. One might argue that you should have an
Quoted 8 lines
enum lockdep_lock_state { LOCK_STATE_NOT_HELD, /* 0 now */ LOCK_STATE_HELD, /* 1 now */ LOCK_STATE_UNKNOWN, /* -1 with your patch but might as well be 2 */ };:)
Thank you both. Picking this back up. Will send v2 incorporating your comments and suggestions.
thanks,
-- Shuah
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
PATCH 2/2 ath10k: detect conf_mutex held ath10k_drain_tx() calls
In reply to Shuah Khanath10k_drain_tx() must not be called with conf_mutex held as workers can use that also. Add call to lockdep_assert_not_held() on conf_mutex to detect if conf_mutex is held by the caller.
The idea for this patch stemmed from coming across the comment block above the ath10k_drain_tx() while reviewing the conf_mutex holds during to debug the conf_mutex lock assert in ath10k_debug_fw_stats_request().
Adding detection to assert on conf_mutex hold will help detect incorrect usages that could lead to locking problems when async worker routines try to call this routine.
--- drivers/net/wireless/ath/ath10k/mac.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/net/wireless/ath/ath10k/mac.c +2 -0
@@ -4728,6 +4728,8 @@ static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,/* Must not be called with conf_mutex held as workers can use that also. */ void ath10k_drain_tx (struct ath10k *ar) { +lockdep_assert_not_held (&ar->conf_mutex); +/* make sure rcu-protected mac80211 tx path itself is drained */ synchronize_net ();
Signature · 7 lines
2.27.0 _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 2/2] ath10k: detect conf_mutex held ath10k_drain_tx() calls
Quoted 8 lines · Shuah Khan
ath10k_drain_tx()must not be called withconf_mutexheld as workers can use that also. Add call tolockdep_assert_not_held()onconf_mutexto detect ifconf_mutexis held by the caller.The idea for this patch stemmed from coming across the comment block above the
ath10k_drain_tx()while reviewing theconf_mutexholds during to debug theconf_mutexlock assert inath10k_debug_fw_stats_request().
Quoted 6 lines
Adding detection to assert on
conf_mutexhold will help detect incorrect usages that could lead to locking problems when async worker routines try to call this routine.
This can go via the same tree as patch 1:
But I can also take this to my ath tree, once patch 1 has reached it. Just let me know what is preferred.
Signature · 8 lines
https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k