[PATCH] lockdep: lockdep_set_notrack_class()

[PATCH] lockdep: lockdep_set_notrack_class()

[Kent Overstreet]

Add a new helper to disable lockdep tracking entirely for a given class.

This is needed for bcachefs, which takes too many btree node locks for
lockdep to track. Instead, we have a single lockdep_map for "btree_trans
has any btree nodes locked", which makes more since given that we have
centralized lock management and a cycle detector.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/bcachefs/btree_locking.c   | 2 +-
 include/linux/lockdep.h       | 4 ++++
 include/linux/lockdep_types.h | 1 +
 kernel/locking/lockdep.c      | 9 ++++++++-
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/bcachefs/btree_locking.c b/fs/bcachefs/btree_locking.c
index 79057cde4c41..16834a6b4c3d 100644
--- a/fs/bcachefs/btree_locking.c
+++ b/fs/bcachefs/btree_locking.c
@@ -10,7 +10,7 @@ void bch2_btree_lock_init(struct btree_bkey_cached_common *b,
 			  enum six_lock_init_flags flags)
 {
 	__six_lock_init(&b->lock, "b->c.lock", &bch2_btree_node_lock_key, flags);
-	lockdep_set_novalidate_class(&b->lock);
+	lockdep_set_notrack_class(&b->lock);
 }
 
 #ifdef CONFIG_LOCKDEP
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 08b0d1d9d78b..b76f1bcd2f7f 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -181,6 +181,9 @@ static inline void lockdep_init_map(struct lockdep_map *lock, const char *name,
 #define lockdep_set_novalidate_class(lock) \
 	lockdep_set_class_and_name(lock, &__lockdep_no_validate__, #lock)
 
+#define lockdep_set_notrack_class(lock) \
+	lockdep_set_class_and_name(lock, &__lockdep_no_track__, #lock)
+
 /*
  * Compare locking classes
  */
@@ -338,6 +341,7 @@ static inline void lockdep_set_selftest_task(struct task_struct *task)
 #define lockdep_set_subclass(lock, sub)		do { } while (0)
 
 #define lockdep_set_novalidate_class(lock) do { } while (0)
+#define lockdep_set_notrack_class(lock) do { } while (0)
 
 /*
  * We don't define lockdep_match_class() and lockdep_match_key() for !LOCKDEP
diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
index 70d30d40ea4a..9f361d3ab9d9 100644
--- a/include/linux/lockdep_types.h
+++ b/include/linux/lockdep_types.h
@@ -80,6 +80,7 @@ struct lock_class_key {
 };
 
 extern struct lock_class_key __lockdep_no_validate__;
+extern struct lock_class_key __lockdep_no_track__;
 
 struct lock_trace;
 
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 151bd3de5936..b6bb9fcd992a 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4918,6 +4918,9 @@ EXPORT_SYMBOL_GPL(lockdep_init_map_type);
 struct lock_class_key __lockdep_no_validate__;
 EXPORT_SYMBOL_GPL(__lockdep_no_validate__);
 
+struct lock_class_key __lockdep_no_track__;
+EXPORT_SYMBOL_GPL(__lockdep_no_track__);
+
 #ifdef CONFIG_PROVE_LOCKING
 void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn cmp_fn,
 			     lock_print_fn print_fn)
@@ -5002,6 +5005,9 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	if (unlikely(!debug_locks))
 		return 0;
 
+	if (unlikely(lock->key == &__lockdep_no_track__))
+		return 0;
+
 	if (!prove_locking || lock->key == &__lockdep_no_validate__)
 		check = 0;
 
@@ -5764,7 +5770,8 @@ void lock_release(struct lockdep_map *lock, unsigned long ip)
 
 	trace_lock_release(lock, ip);
 
-	if (unlikely(!lockdep_enabled()))
+	if (unlikely(!lockdep_enabled() ||
+		     lock->key == &__lockdep_no_track__))
 		return;
 
 	raw_local_irq_save(flags);
-- 
2.45.2

Re: [PATCH] lockdep: lockdep_set_notrack_class()

[Waiman Long]

On 6/30/24 01:10, Kent Overstreet wrote:
> Add a new helper to disable lockdep tracking entirely for a given class.
>
> This is needed for bcachefs, which takes too many btree node locks for
> lockdep to track. Instead, we have a single lockdep_map for "btree_trans
> has any btree nodes locked", which makes more since given that we have
> centralized lock management and a cycle detector.

Could you explain a bit more what the current novalidate_class is 
lacking WRT to the bcachefs lock? Is it excessive performance overhead 
or some bogus lockdep warning?

Cheers,
Longman

>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
>   fs/bcachefs/btree_locking.c   | 2 +-
>   include/linux/lockdep.h       | 4 ++++
>   include/linux/lockdep_types.h | 1 +
>   kernel/locking/lockdep.c      | 9 ++++++++-
>   4 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/fs/bcachefs/btree_locking.c b/fs/bcachefs/btree_locking.c
> index 79057cde4c41..16834a6b4c3d 100644
> --- a/fs/bcachefs/btree_locking.c
> +++ b/fs/bcachefs/btree_locking.c
> @@ -10,7 +10,7 @@ void bch2_btree_lock_init(struct btree_bkey_cached_common *b,
>   			  enum six_lock_init_flags flags)
>   {
>   	__six_lock_init(&b->lock, "b->c.lock", &bch2_btree_node_lock_key, flags);
> -	lockdep_set_novalidate_class(&b->lock);
> +	lockdep_set_notrack_class(&b->lock);
>   }
>   
>   #ifdef CONFIG_LOCKDEP
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 08b0d1d9d78b..b76f1bcd2f7f 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -181,6 +181,9 @@ static inline void lockdep_init_map(struct lockdep_map *lock, const char *name,
>   #define lockdep_set_novalidate_class(lock) \
>   	lockdep_set_class_and_name(lock, &__lockdep_no_validate__, #lock)
>   
> +#define lockdep_set_notrack_class(lock) \
> +	lockdep_set_class_and_name(lock, &__lockdep_no_track__, #lock)
> +
>   /*
>    * Compare locking classes
>    */
> @@ -338,6 +341,7 @@ static inline void lockdep_set_selftest_task(struct task_struct *task)
>   #define lockdep_set_subclass(lock, sub)		do { } while (0)
>   
>   #define lockdep_set_novalidate_class(lock) do { } while (0)
> +#define lockdep_set_notrack_class(lock) do { } while (0)
>   
>   /*
>    * We don't define lockdep_match_class() and lockdep_match_key() for !LOCKDEP
> diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
> index 70d30d40ea4a..9f361d3ab9d9 100644
> --- a/include/linux/lockdep_types.h
> +++ b/include/linux/lockdep_types.h
> @@ -80,6 +80,7 @@ struct lock_class_key {
>   };
>   
>   extern struct lock_class_key __lockdep_no_validate__;
> +extern struct lock_class_key __lockdep_no_track__;
>   
>   struct lock_trace;
>   
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 151bd3de5936..b6bb9fcd992a 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4918,6 +4918,9 @@ EXPORT_SYMBOL_GPL(lockdep_init_map_type);
>   struct lock_class_key __lockdep_no_validate__;
>   EXPORT_SYMBOL_GPL(__lockdep_no_validate__);
>   
> +struct lock_class_key __lockdep_no_track__;
> +EXPORT_SYMBOL_GPL(__lockdep_no_track__);
> +
>   #ifdef CONFIG_PROVE_LOCKING
>   void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn cmp_fn,
>   			     lock_print_fn print_fn)
> @@ -5002,6 +5005,9 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
>   	if (unlikely(!debug_locks))
>   		return 0;
>   
> +	if (unlikely(lock->key == &__lockdep_no_track__))
> +		return 0;
> +
>   	if (!prove_locking || lock->key == &__lockdep_no_validate__)
>   		check = 0;
>   
> @@ -5764,7 +5770,8 @@ void lock_release(struct lockdep_map *lock, unsigned long ip)
>   
>   	trace_lock_release(lock, ip);
>   
> -	if (unlikely(!lockdep_enabled()))
> +	if (unlikely(!lockdep_enabled() ||
> +		     lock->key == &__lockdep_no_track__))
>   		return;
>   
>   	raw_local_irq_save(flags);

Re: [PATCH] lockdep: lockdep_set_notrack_class()

[Kent Overstreet]

On Sun, Jun 30, 2024 at 06:08:21PM GMT, Waiman Long wrote:
> On 6/30/24 01:10, Kent Overstreet wrote:
> > Add a new helper to disable lockdep tracking entirely for a given class.
> > 
> > This is needed for bcachefs, which takes too many btree node locks for
> > lockdep to track. Instead, we have a single lockdep_map for "btree_trans
> > has any btree nodes locked", which makes more since given that we have
> > centralized lock management and a cycle detector.
> 
> Could you explain a bit more what the current novalidate_class is lacking
> WRT to the bcachefs lock? Is it excessive performance overhead or some bogus
> lockdep warning?

novalidate just switches off checking of lock ordering, but the fact
that the locks are held is still tracked.

bcachefs takes more btree node locks than lockdep can track, so I'm
switching to a single lockdep map for "this btree_trans has any btree
nodes locked" instead.

Re: [PATCH] lockdep: lockdep_set_notrack_class()

[Waiman Long]

On 6/30/24 20:10, Kent Overstreet wrote:
> On Sun, Jun 30, 2024 at 06:08:21PM GMT, Waiman Long wrote:
>> On 6/30/24 01:10, Kent Overstreet wrote:
>>> Add a new helper to disable lockdep tracking entirely for a given class.
>>>
>>> This is needed for bcachefs, which takes too many btree node locks for
>>> lockdep to track. Instead, we have a single lockdep_map for "btree_trans
>>> has any btree nodes locked", which makes more since given that we have
>>> centralized lock management and a cycle detector.
>> Could you explain a bit more what the current novalidate_class is lacking
>> WRT to the bcachefs lock? Is it excessive performance overhead or some bogus
>> lockdep warning?
> novalidate just switches off checking of lock ordering, but the fact
> that the locks are held is still tracked.
>
> bcachefs takes more btree node locks than lockdep can track, so I'm
> switching to a single lockdep map for "this btree_trans has any btree
> nodes locked" instead.

I asked because you are adding a new special class that is similar to 
no_validate in some way but also a bit different. So we need to document 
what each of these special classes are for to avoid confusion.

Cheers,
Longman