[PATCH 0/2] lockdep lock comparison function

[PATCH 0/2] lockdep lock comparison function

[Kent Overstreet]

This patch implements the lock comparison function I've been talking
about, and converts one of bcache's locks to use it.

b->write_lock has different locking rules; I'm not sure there's an easy
way to get rid of lockdep_set_novalidate_class for it - but the code has
changed and my memory is foggy. 

I'd like it if we could convert existing uses of *_lock_nested() to this
approach, since it's more rigorous and IMO, much clearer. That'll
require looking at specific use cases, though - the inode lock in
fs/inode.c is the only one I looked at and it's got a lot of nutty stuff
going on.

Kent Overstreet (2):
  lockdep: lock_set_lock_cmp_fn()
  bcache: Convert to lock_cmp_fn

 drivers/md/bcache/btree.c     | 15 ++++++++++-
 drivers/md/bcache/btree.h     |  4 +--
 include/linux/lockdep.h       |  8 ++++++
 include/linux/lockdep_types.h |  6 +++++
 kernel/locking/lockdep.c      | 51 ++++++++++++++++++++++++++++++++++-
 5 files changed, 80 insertions(+), 4 deletions(-)

-- 
2.39.2

[PATCH 1/2] lockdep: lock_set_lock_cmp_fn()

[Kent Overstreet]

This implements a new interface to lockedp, lock_set_cmp_fn(), for
defining an ordering when taking multiple locks of the same type.

This provides a more rigorous mechanism than the subclass mechanism -
it's hoped that this might be able to replace subclasses.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Peter Zijlstra <peterz@infradead.org> (maintainer:LOCKING PRIMITIVES)
Cc: Ingo Molnar <mingo@redhat.com> (maintainer:LOCKING PRIMITIVES)
---
 include/linux/lockdep.h       |  8 ++++++
 include/linux/lockdep_types.h |  6 +++++
 kernel/locking/lockdep.c      | 51 ++++++++++++++++++++++++++++++++++-
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1f1099dac3..a4673a6f7a 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -662,4 +662,12 @@ lockdep_rcu_suspicious(const char *file, const int line, const char *s)
 }
 #endif
 
+#ifdef CONFIG_PROVE_LOCKING
+void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn fn);
+
+#define lock_set_lock_cmp_fn(lock, fn)		lockdep_set_lock_cmp_fn(&(lock)->dep_map, fn)
+#else
+#define lock_set_lock_cmp_fn(lock, fn)		do {} while (0)
+#endif
+
 #endif /* __LINUX_LOCKDEP_H */
diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
index d22430840b..813467dccb 100644
--- a/include/linux/lockdep_types.h
+++ b/include/linux/lockdep_types.h
@@ -84,6 +84,10 @@ struct lock_trace;
 
 #define LOCKSTAT_POINTS		4
 
+struct lockdep_map;
+typedef int (*lock_cmp_fn)(const struct lockdep_map *a,
+			   const struct lockdep_map *b);
+
 /*
  * The lock-class itself. The order of the structure members matters.
  * reinit_class() zeroes the key member and all subsequent members.
@@ -109,6 +113,8 @@ struct lock_class {
 	struct list_head		locks_after, locks_before;
 
 	const struct lockdep_subclass_key *key;
+	lock_cmp_fn			cmp_fn;
+
 	unsigned int			subclass;
 	unsigned int			dep_gen_id;
 
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e3375bc40d..b9e759743e 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2143,6 +2143,8 @@ check_path(struct held_lock *target, struct lock_list *src_entry,
 	return ret;
 }
 
+static void print_deadlock_bug(struct task_struct *, struct held_lock *, struct held_lock *);
+
 /*
  * Prove that the dependency graph starting at <src> can not
  * lead to <target>. If it can, there is a circle when adding
@@ -2174,7 +2176,10 @@ check_noncircular(struct held_lock *src, struct held_lock *target,
 			*trace = save_trace();
 		}
 
-		print_circular_bug(&src_entry, target_entry, src, target);
+		if (src->class_idx == target->class_idx)
+			print_deadlock_bug(current, src, target);
+		else
+			print_circular_bug(&src_entry, target_entry, src, target);
 	}
 
 	return ret;
@@ -2968,6 +2973,8 @@ static void
 print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
 		   struct held_lock *next)
 {
+	struct lock_class *class = hlock_class(prev);
+
 	if (!debug_locks_off_graph_unlock() || debug_locks_silent)
 		return;
 
@@ -2982,6 +2989,10 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
 	pr_warn("\nbut task is already holding lock:\n");
 	print_lock(prev);
 
+	if (class->cmp_fn)
+		pr_warn("and the lock comparison function returns %i:\n",
+			class->cmp_fn(prev->instance, next->instance));
+
 	pr_warn("\nother info that might help us debug this:\n");
 	print_deadlock_scenario(next, prev);
 	lockdep_print_held_locks(curr);
@@ -3003,6 +3014,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
 static int
 check_deadlock(struct task_struct *curr, struct held_lock *next)
 {
+	struct lock_class *class;
 	struct held_lock *prev;
 	struct held_lock *nest = NULL;
 	int i;
@@ -3023,6 +3035,12 @@ check_deadlock(struct task_struct *curr, struct held_lock *next)
 		if ((next->read == 2) && prev->read)
 			continue;
 
+		class = hlock_class(prev);
+
+		if (class->cmp_fn &&
+		    class->cmp_fn(prev->instance, next->instance) < 0)
+			continue;
+
 		/*
 		 * We're holding the nest_lock, which serializes this lock's
 		 * nesting behaviour.
@@ -3084,6 +3102,14 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
 		return 2;
 	}
 
+	if (prev->class_idx == next->class_idx) {
+		struct lock_class *class = hlock_class(prev);
+
+		if (class->cmp_fn &&
+		    class->cmp_fn(prev->instance, next->instance) < 0)
+			return 2;
+	}
+
 	/*
 	 * Prove that the new <prev> -> <next> dependency would not
 	 * create a circular dependency in the graph. (We do this by
@@ -6597,3 +6623,26 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
 	dump_stack();
 }
 EXPORT_SYMBOL_GPL(lockdep_rcu_suspicious);
+
+#ifdef CONFIG_PROVE_LOCKING
+void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn fn)
+{
+	struct lock_class *class = lock->class_cache[0];
+	unsigned long flags;
+
+	raw_local_irq_save(flags);
+	lockdep_recursion_inc();
+
+	if (!class)
+		class = register_lock_class(lock, 0, 0);
+
+	WARN_ON(class && class->cmp_fn && class->cmp_fn != fn);
+
+	if (class)
+		class->cmp_fn = fn;
+
+	lockdep_recursion_finish();
+	raw_local_irq_restore(flags);
+}
+EXPORT_SYMBOL_GPL(lockdep_set_lock_cmp_fn);
+#endif
-- 
2.39.2

Re: [PATCH 1/2] lockdep: lock_set_lock_cmp_fn()

[Peter Zijlstra]

On Fri, Feb 17, 2023 at 10:21:16PM -0500, Kent Overstreet wrote:

> @@ -2982,6 +2989,10 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
>  	pr_warn("\nbut task is already holding lock:\n");
>  	print_lock(prev);
>  
> +	if (class->cmp_fn)
> +		pr_warn("and the lock comparison function returns %i:\n",
> +			class->cmp_fn(prev->instance, next->instance));
> +

Please, use {} for any actual multi-line.

But is this sufficient data to debug a splat? Given an inversion on this
class, we'll get something like:

 A
 A -a
 A -b

vs

 A
 A c

which is I suppose sufficient to say that A messed up, but not much
more. With subclasses we would've gotten

 A/0
 A/1
 A/2

vs

 A/2
 A/0

which is much simpler to work with. Can we improve on this? Give the
cmp_fn an additinoal optional argument of a string pointer or so to fill
out with actual details to be printed?


>  	pr_warn("\nother info that might help us debug this:\n");
>  	print_deadlock_scenario(next, prev);
>  	lockdep_print_held_locks(curr);

> @@ -6597,3 +6623,26 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
>  	dump_stack();
>  }
>  EXPORT_SYMBOL_GPL(lockdep_rcu_suspicious);
> +
> +#ifdef CONFIG_PROVE_LOCKING

Surely we can find an existing #ifdef block to squirrel this in?

> +void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn fn)
> +{
> +	struct lock_class *class = lock->class_cache[0];
> +	unsigned long flags;
> +
> +	raw_local_irq_save(flags);
> +	lockdep_recursion_inc();
> +
> +	if (!class)
> +		class = register_lock_class(lock, 0, 0);
> +
> +	WARN_ON(class && class->cmp_fn && class->cmp_fn != fn);
> +
> +	if (class)
> +		class->cmp_fn = fn;
> +
> +	lockdep_recursion_finish();
> +	raw_local_irq_restore(flags);
> +}
> +EXPORT_SYMBOL_GPL(lockdep_set_lock_cmp_fn);

However, the bigger question is if this should be part of the
lockdep_init_map_*() family. Perhaps something like below.

With the thinking that the comparator is very much part of the class.

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1f1099dac3f0..ab50a889991f 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -181,8 +181,18 @@ extern void lockdep_unregister_key(struct lock_class_key *key);
  * to lockdep:
  */
 
-extern void lockdep_init_map_type(struct lockdep_map *lock, const char *name,
-	struct lock_class_key *key, int subclass, u8 inner, u8 outer, u8 lock_type);
+extern void
+lockdep_init_map_cmp(struct lockdep_map *lock, const char *name,
+		     struct lock_class_key *key, int subclass, u8 inner, u8 outer,
+		     u8 lock_type, lock_cmp_fn cmp_fn);
+
+static inline void
+lockdep_init_map_type(struct lockdep_map *lock, const char *name,
+		      struct lock_class_key *key, int subclass, u8 inner, u8 outer,
+		      u8 lock_type)
+{
+	lockdep_init_map_cmp(lock, name, key, subclass, inner, outer, lock_type, NULL);
+}
 
 static inline void
 lockdep_init_map_waits(struct lockdep_map *lock, const char *name,
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e3375bc40dad..5a6c9512d5b2 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4800,9 +4800,10 @@ static inline int check_wait_context(struct task_struct *curr,
 /*
  * Initialize a lock instance's lock-class mapping info:
  */
-void lockdep_init_map_type(struct lockdep_map *lock, const char *name,
-			    struct lock_class_key *key, int subclass,
-			    u8 inner, u8 outer, u8 lock_type)
+void lockdep_init_map_cmp(struct lockdep_map *lock, const char *name,
+			  struct lock_class_key *key, int subclass,
+			  u8 inner, u8 outer, u8 lock_type,
+			  lock_cmp_fn cmp_fn)
 {
 	int i;
 
@@ -4847,7 +4848,8 @@ void lockdep_init_map_type(struct lockdep_map *lock, const char *name,
 	if (unlikely(!debug_locks))
 		return;
 
-	if (subclass) {
+	if (subclass || cmp_fn) {
+		struct lock_class *class;
 		unsigned long flags;
 
 		if (DEBUG_LOCKS_WARN_ON(!lockdep_enabled()))
@@ -4855,7 +4857,9 @@ void lockdep_init_map_type(struct lockdep_map *lock, const char *name,
 
 		raw_local_irq_save(flags);
 		lockdep_recursion_inc();
-		register_lock_class(lock, subclass, 1);
+		class = register_lock_class(lock, subclass, 1);
+		if (!WARN_ON(!class || (class->cmp_fn && class->cmp_fn != cmp_fn)))
+			class->cmp_fn = cmp_fn;
 		lockdep_recursion_finish();
 		raw_local_irq_restore(flags);
 	}

Re: [PATCH 1/2] lockdep: lock_set_lock_cmp_fn()

[Kent Overstreet]

On Mon, Feb 20, 2023 at 04:09:33PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 17, 2023 at 10:21:16PM -0500, Kent Overstreet wrote:
> 
> > @@ -2982,6 +2989,10 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
> >  	pr_warn("\nbut task is already holding lock:\n");
> >  	print_lock(prev);
> >  
> > +	if (class->cmp_fn)
> > +		pr_warn("and the lock comparison function returns %i:\n",
> > +			class->cmp_fn(prev->instance, next->instance));
> > +
> 
> Please, use {} for any actual multi-line.
> 
> But is this sufficient data to debug a splat? Given an inversion on this
> class, we'll get something like:
> 
>  A
>  A -a
>  A -b
> 
> vs
> 
>  A
>  A c
> 
> which is I suppose sufficient to say that A messed up, but not much
> more. With subclasses we would've gotten
> 
>  A/0
>  A/1
>  A/2
> 
> vs
> 
>  A/2
>  A/0
> 
> which is much simpler to work with. Can we improve on this? Give the
> cmp_fn an additinoal optional argument of a string pointer or so to fill
> out with actual details to be printed?

Yes. This is where printbufs and %pf() would've been really nice, it'll
be doable but ugly with what we have now for string output. We just need
to add another callback, either .lock_print() or .lock_to_text(), and
that can print the information about the lock that's relevant for lock
ordering.

For bcache, that'd be something like

static void btree_lock_to_text()
{
	struct btree *b = container_of()

	seq_buf_printf("l=%u %llu:%llu", b->level, KEY_INODE(), KEY_OFFSET()...)
}

Re: [PATCH 1/2] lockdep: lock_set_lock_cmp_fn()

[Kent Overstreet]

On Mon, Feb 20, 2023 at 04:09:33PM +0100, Peter Zijlstra wrote:
> which is much simpler to work with. Can we improve on this? Give the
> cmp_fn an additinoal optional argument of a string pointer or so to fill
> out with actual details to be printed?

Here you go, example bcache output:

Patch is not as pretty as I'd like because not every path that prints a
lock has held_lock available - but the ones we care about in this
scenario do.

============================================
WARNING: possible recursive locking detected
6.2.0-rc8-00003-g7d81e591ca6a-dirty #15 Not tainted
--------------------------------------------
kworker/14:3/938 is trying to acquire lock:
ffff8880143218c8 (&b->lock l=0 0:2803368){++++}-{3:3}, at: bch_btree_node_get.part.0+0x81/0x2b0

but task is already holding lock:
ffff8880143de8c8 (&b->lock l=1 1048575:9223372036854775807){++++}-{3:3}, at: __bch_btree_map_nodes+0xea/0x1e0
and the lock comparison function returns 1:

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&b->lock l=1 1048575:9223372036854775807);
  lock(&b->lock l=0 0:2803368);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by kworker/14:3/938:
 #0: ffff888005ea9d38 ((wq_completion)bcache){+.+.}-{0:0}, at: process_one_work+0x1ec/0x530
 #1: ffff8880098c3e70 ((work_completion)(&cl->work)#3){+.+.}-{0:0}, at: process_one_work+0x1ec/0x530
 #2: ffff8880143de8c8 (&b->lock l=1 1048575:9223372036854775807){++++}-{3:3}, at: __bch_btree_map_nodes+0xea/0x1e0

-- >8 --
Subject: [PATCH] lockdep: lock_set_print_fn()

This implements a new interface to lockedp, lock_set_print_fn(), for
printing additional information about a lock.

This is intended to be useful with the previous lock_set_cmp_fn(); when
defininig an ordering for locks of a given type, we'll want to print out
information about that lock that's relevant for the ordering we defined.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
---
 include/linux/lockdep.h       |  3 ++
 include/linux/lockdep_types.h |  2 +
 kernel/locking/lockdep.c      | 81 ++++++++++++++++++++++-------------
 3 files changed, 57 insertions(+), 29 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 98e0338a74..66d8a5a24e 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -664,10 +664,13 @@ lockdep_rcu_suspicious(const char *file, const int line, const char *s)
 
 #ifdef CONFIG_PROVE_LOCKING
 void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn fn);
+void lockdep_set_lock_print_fn(struct lockdep_map *lock, lock_print_fn fn);
 
 #define lock_set_cmp_fn(lock, fn)	lockdep_set_lock_cmp_fn(&(lock)->dep_map, fn)
+#define lock_set_print_fn(lock, fn)	lockdep_set_lock_print_fn(&(lock)->dep_map, fn)
 #else
 #define lock_set_cmp_fn(lock, fn)	do {} while (0)
+#define lock_set_print_fn(lock, fn)	do {} while (0)
 #endif
 
 #endif /* __LINUX_LOCKDEP_H */
diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
index 813467dccb..8bf79c4e48 100644
--- a/include/linux/lockdep_types.h
+++ b/include/linux/lockdep_types.h
@@ -87,6 +87,7 @@ struct lock_trace;
 struct lockdep_map;
 typedef int (*lock_cmp_fn)(const struct lockdep_map *a,
 			   const struct lockdep_map *b);
+typedef void (*lock_print_fn)(const struct lockdep_map *map);
 
 /*
  * The lock-class itself. The order of the structure members matters.
@@ -114,6 +115,7 @@ struct lock_class {
 
 	const struct lockdep_subclass_key *key;
 	lock_cmp_fn			cmp_fn;
+	lock_print_fn			print_fn;
 
 	unsigned int			subclass;
 	unsigned int			dep_gen_id;
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index b9e759743e..edecd0301b 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -708,7 +708,7 @@ void get_usage_chars(struct lock_class *class, char usage[LOCK_USAGE_CHARS])
 	usage[i] = '\0';
 }
 
-static void __print_lock_name(struct lock_class *class)
+static void __print_lock_name(struct held_lock *hlock, struct lock_class *class)
 {
 	char str[KSYM_NAME_LEN];
 	const char *name;
@@ -723,17 +723,19 @@ static void __print_lock_name(struct lock_class *class)
 			printk(KERN_CONT "#%d", class->name_version);
 		if (class->subclass)
 			printk(KERN_CONT "/%d", class->subclass);
+		if (hlock && class->print_fn)
+			class->print_fn(hlock->instance);
 	}
 }
 
-static void print_lock_name(struct lock_class *class)
+static void print_lock_name(struct held_lock *hlock, struct lock_class *class)
 {
 	char usage[LOCK_USAGE_CHARS];
 
 	get_usage_chars(class, usage);
 
 	printk(KERN_CONT " (");
-	__print_lock_name(class);
+	__print_lock_name(hlock, class);
 	printk(KERN_CONT "){%s}-{%d:%d}", usage,
 			class->wait_type_outer ?: class->wait_type_inner,
 			class->wait_type_inner);
@@ -771,7 +773,7 @@ static void print_lock(struct held_lock *hlock)
 	}
 
 	printk(KERN_CONT "%px", hlock->instance);
-	print_lock_name(lock);
+	print_lock_name(hlock, lock);
 	printk(KERN_CONT ", at: %pS\n", (void *)hlock->acquire_ip);
 }
 
@@ -1867,7 +1869,7 @@ print_circular_bug_entry(struct lock_list *target, int depth)
 	if (debug_locks_silent)
 		return;
 	printk("\n-> #%u", depth);
-	print_lock_name(target->class);
+	print_lock_name(NULL, target->class);
 	printk(KERN_CONT ":\n");
 	print_lock_trace(target->trace, 6);
 }
@@ -1896,11 +1898,11 @@ print_circular_lock_scenario(struct held_lock *src,
 	 */
 	if (parent != source) {
 		printk("Chain exists of:\n  ");
-		__print_lock_name(source);
+		__print_lock_name(src, source);
 		printk(KERN_CONT " --> ");
-		__print_lock_name(parent);
+		__print_lock_name(NULL, parent);
 		printk(KERN_CONT " --> ");
-		__print_lock_name(target);
+		__print_lock_name(tgt, target);
 		printk(KERN_CONT "\n\n");
 	}
 
@@ -1908,16 +1910,16 @@ print_circular_lock_scenario(struct held_lock *src,
 	printk("       CPU0                    CPU1\n");
 	printk("       ----                    ----\n");
 	printk("  lock(");
-	__print_lock_name(target);
+	__print_lock_name(tgt, target);
 	printk(KERN_CONT ");\n");
 	printk("                               lock(");
-	__print_lock_name(parent);
+	__print_lock_name(NULL, parent);
 	printk(KERN_CONT ");\n");
 	printk("                               lock(");
-	__print_lock_name(target);
+	__print_lock_name(tgt, target);
 	printk(KERN_CONT ");\n");
 	printk("  lock(");
-	__print_lock_name(source);
+	__print_lock_name(src, source);
 	printk(KERN_CONT ");\n");
 	printk("\n *** DEADLOCK ***\n\n");
 }
@@ -2335,7 +2337,7 @@ static void print_lock_class_header(struct lock_class *class, int depth)
 	int bit;
 
 	printk("%*s->", depth, "");
-	print_lock_name(class);
+	print_lock_name(NULL, class);
 #ifdef CONFIG_DEBUG_LOCKDEP
 	printk(KERN_CONT " ops: %lu", debug_class_ops_read(class));
 #endif
@@ -2517,11 +2519,11 @@ print_irq_lock_scenario(struct lock_list *safe_entry,
 	 */
 	if (middle_class != unsafe_class) {
 		printk("Chain exists of:\n  ");
-		__print_lock_name(safe_class);
+		__print_lock_name(NULL, safe_class);
 		printk(KERN_CONT " --> ");
-		__print_lock_name(middle_class);
+		__print_lock_name(NULL, middle_class);
 		printk(KERN_CONT " --> ");
-		__print_lock_name(unsafe_class);
+		__print_lock_name(NULL, unsafe_class);
 		printk(KERN_CONT "\n\n");
 	}
 
@@ -2529,18 +2531,18 @@ print_irq_lock_scenario(struct lock_list *safe_entry,
 	printk("       CPU0                    CPU1\n");
 	printk("       ----                    ----\n");
 	printk("  lock(");
-	__print_lock_name(unsafe_class);
+	__print_lock_name(NULL, unsafe_class);
 	printk(KERN_CONT ");\n");
 	printk("                               local_irq_disable();\n");
 	printk("                               lock(");
-	__print_lock_name(safe_class);
+	__print_lock_name(NULL, safe_class);
 	printk(KERN_CONT ");\n");
 	printk("                               lock(");
-	__print_lock_name(middle_class);
+	__print_lock_name(NULL, middle_class);
 	printk(KERN_CONT ");\n");
 	printk("  <Interrupt>\n");
 	printk("    lock(");
-	__print_lock_name(safe_class);
+	__print_lock_name(NULL, safe_class);
 	printk(KERN_CONT ");\n");
 	printk("\n *** DEADLOCK ***\n\n");
 }
@@ -2577,20 +2579,20 @@ print_bad_irq_dependency(struct task_struct *curr,
 	pr_warn("\nand this task is already holding:\n");
 	print_lock(prev);
 	pr_warn("which would create a new lock dependency:\n");
-	print_lock_name(hlock_class(prev));
+	print_lock_name(prev, hlock_class(prev));
 	pr_cont(" ->");
-	print_lock_name(hlock_class(next));
+	print_lock_name(next, hlock_class(next));
 	pr_cont("\n");
 
 	pr_warn("\nbut this new dependency connects a %s-irq-safe lock:\n",
 		irqclass);
-	print_lock_name(backwards_entry->class);
+	print_lock_name(NULL, backwards_entry->class);
 	pr_warn("\n... which became %s-irq-safe at:\n", irqclass);
 
 	print_lock_trace(backwards_entry->class->usage_traces[bit1], 1);
 
 	pr_warn("\nto a %s-irq-unsafe lock:\n", irqclass);
-	print_lock_name(forwards_entry->class);
+	print_lock_name(NULL, forwards_entry->class);
 	pr_warn("\n... which became %s-irq-unsafe at:\n", irqclass);
 	pr_warn("...");
 
@@ -2960,10 +2962,10 @@ print_deadlock_scenario(struct held_lock *nxt, struct held_lock *prv)
 	printk("       CPU0\n");
 	printk("       ----\n");
 	printk("  lock(");
-	__print_lock_name(prev);
+	__print_lock_name(prv, prev);
 	printk(KERN_CONT ");\n");
 	printk("  lock(");
-	__print_lock_name(next);
+	__print_lock_name(nxt, next);
 	printk(KERN_CONT ");\n");
 	printk("\n *** DEADLOCK ***\n\n");
 	printk(" May be due to missing lock nesting notation\n\n");
@@ -3943,11 +3945,11 @@ static void print_usage_bug_scenario(struct held_lock *lock)
 	printk("       CPU0\n");
 	printk("       ----\n");
 	printk("  lock(");
-	__print_lock_name(class);
+	__print_lock_name(lock, class);
 	printk(KERN_CONT ");\n");
 	printk("  <Interrupt>\n");
 	printk("    lock(");
-	__print_lock_name(class);
+	__print_lock_name(lock, class);
 	printk(KERN_CONT ");\n");
 	printk("\n *** DEADLOCK ***\n\n");
 }
@@ -4033,7 +4035,7 @@ print_irq_inversion_bug(struct task_struct *curr,
 		pr_warn("but this lock took another, %s-unsafe lock in the past:\n", irqclass);
 	else
 		pr_warn("but this lock was taken by another, %s-safe lock in the past:\n", irqclass);
-	print_lock_name(other->class);
+	print_lock_name(NULL, other->class);
 	pr_warn("\n\nand interrupts could create inverse lock ordering between them.\n\n");
 
 	pr_warn("\nother info that might help us debug this:\n");
@@ -6645,4 +6647,25 @@ void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn fn)
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(lockdep_set_lock_cmp_fn);
+
+void lockdep_set_lock_print_fn(struct lockdep_map *lock, lock_print_fn fn)
+{
+	struct lock_class *class = lock->class_cache[0];
+	unsigned long flags;
+
+	raw_local_irq_save(flags);
+	lockdep_recursion_inc();
+
+	if (!class)
+		class = register_lock_class(lock, 0, 0);
+
+	WARN_ON(class && class->print_fn && class->print_fn != fn);
+
+	if (class)
+		class->print_fn = fn;
+
+	lockdep_recursion_finish();
+	raw_local_irq_restore(flags);
+}
+EXPORT_SYMBOL_GPL(lockdep_set_lock_print_fn);
 #endif
-- 
2.39.2

Re: [PATCH 1/2] lockdep: lock_set_lock_cmp_fn()

[Peter Zijlstra]

On Mon, Feb 20, 2023 at 06:51:59PM -0500, Kent Overstreet wrote:
> On Mon, Feb 20, 2023 at 04:09:33PM +0100, Peter Zijlstra wrote:
> > which is much simpler to work with. Can we improve on this? Give the
> > cmp_fn an additinoal optional argument of a string pointer or so to fill
> > out with actual details to be printed?
> 
> Here you go, example bcache output:
> 
> Patch is not as pretty as I'd like because not every path that prints a
> lock has held_lock available - but the ones we care about in this
> scenario do.

Right, unavoidable that.

> ============================================
> WARNING: possible recursive locking detected
> 6.2.0-rc8-00003-g7d81e591ca6a-dirty #15 Not tainted
> --------------------------------------------
> kworker/14:3/938 is trying to acquire lock:
> ffff8880143218c8 (&b->lock l=0 0:2803368){++++}-{3:3}, at: bch_btree_node_get.part.0+0x81/0x2b0
> 
> but task is already holding lock:
> ffff8880143de8c8 (&b->lock l=1 1048575:9223372036854775807){++++}-{3:3}, at: __bch_btree_map_nodes+0xea/0x1e0
> and the lock comparison function returns 1:
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&b->lock l=1 1048575:9223372036854775807);
>   lock(&b->lock l=0 0:2803368);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation
> 
> 3 locks held by kworker/14:3/938:
>  #0: ffff888005ea9d38 ((wq_completion)bcache){+.+.}-{0:0}, at: process_one_work+0x1ec/0x530
>  #1: ffff8880098c3e70 ((work_completion)(&cl->work)#3){+.+.}-{0:0}, at: process_one_work+0x1ec/0x530
>  #2: ffff8880143de8c8 (&b->lock l=1 1048575:9223372036854775807){++++}-{3:3}, at: __bch_btree_map_nodes+0xea/0x1e0

Much better indeed. Thanks!

> -- >8 --
> Subject: [PATCH] lockdep: lock_set_print_fn()
> 
> This implements a new interface to lockedp, lock_set_print_fn(), for
> printing additional information about a lock.
> 
> This is intended to be useful with the previous lock_set_cmp_fn(); when
> defininig an ordering for locks of a given type, we'll want to print out
> information about that lock that's relevant for the ordering we defined.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> ---
>  include/linux/lockdep.h       |  3 ++
>  include/linux/lockdep_types.h |  2 +
>  kernel/locking/lockdep.c      | 81 ++++++++++++++++++++++-------------
>  3 files changed, 57 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 98e0338a74..66d8a5a24e 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -664,10 +664,13 @@ lockdep_rcu_suspicious(const char *file, const int line, const char *s)
>  
>  #ifdef CONFIG_PROVE_LOCKING
>  void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn fn);
> +void lockdep_set_lock_print_fn(struct lockdep_map *lock, lock_print_fn fn);

I would suggest the same as last time; integrate this in the class
setting zoo of functions. If you insiste, please keep it one function
and force print_fn when cmp_fn. We don't want people to skimp out on
this.

Other than that, I don't think this can fully replace subclasses, since
subclasses would allow lock hierarchies with other classes inter-twined,
while this really relies on pure class nesting.

That is:

	A/0
	B
	A/1

is a valid subclass nesting set, but you can't achieve the same with
this.

That said; it does seem like a very useful additional annotation for the
more complex nesting sets.

Thanks!

Re: [PATCH 1/2] lockdep: lock_set_lock_cmp_fn()

[Kent Overstreet]

On Thu, Feb 23, 2023 at 02:01:58PM +0100, Peter Zijlstra wrote:
> I would suggest the same as last time; integrate this in the class
> setting zoo of functions.

I don't really see a particular zoo, things like pretty scattered to me.
With lockdep_init_map_type?

> If you insiste, please keep it one function
> and force print_fn when cmp_fn. We don't want people to skimp out on
> this.

That's easily done.

> 
> Other than that, I don't think this can fully replace subclasses, since
> subclasses would allow lock hierarchies with other classes inter-twined,
> while this really relies on pure class nesting.
> 
> That is:
> 
> 	A/0
> 	B
> 	A/1
> 
> is a valid subclass nesting set, but you can't achieve the same with
> this.

Why not just assign those As to two different classes?

> That said; it does seem like a very useful additional annotation for the
> more complex nesting sets.
> 
> Thanks!

Cheers

[PATCH 2/2] bcache: Convert to lock_cmp_fn

[Kent Overstreet]

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Coly Li <colyli@suse.de> (maintainer:BCACHE (BLOCK LAYER CACHE))
---
 drivers/md/bcache/btree.c | 15 ++++++++++++++-
 drivers/md/bcache/btree.h |  4 ++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 147c493a98..db2bf2402c 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -559,6 +559,19 @@ static void mca_data_alloc(struct btree *b, struct bkey *k, gfp_t gfp)
 	}
 }
 
+#define cmp_int(l, r)		((l > r) - (l < r))
+
+#ifdef CONFIG_PROVE_LOCKING
+static int btree_lock_cmp_fn(const struct lockdep_map *_a,
+			     const struct lockdep_map *_b)
+{
+	const struct btree *a = container_of(_a, struct btree, lock.dep_map);
+	const struct btree *b = container_of(_b, struct btree, lock.dep_map);
+
+	return -cmp_int(a->level, b->level) ?: bkey_cmp(&a->key, &b->key);
+}
+#endif
+
 static struct btree *mca_bucket_alloc(struct cache_set *c,
 				      struct bkey *k, gfp_t gfp)
 {
@@ -572,7 +585,7 @@ static struct btree *mca_bucket_alloc(struct cache_set *c,
 		return NULL;
 
 	init_rwsem(&b->lock);
-	lockdep_set_novalidate_class(&b->lock);
+	lock_set_lock_cmp_fn(&b->lock, btree_lock_cmp_fn);
 	mutex_init(&b->write_lock);
 	lockdep_set_novalidate_class(&b->write_lock);
 	INIT_LIST_HEAD(&b->list);
diff --git a/drivers/md/bcache/btree.h b/drivers/md/bcache/btree.h
index 1b5fdbc0d8..17b1d201ce 100644
--- a/drivers/md/bcache/btree.h
+++ b/drivers/md/bcache/btree.h
@@ -247,8 +247,8 @@ static inline void bch_btree_op_init(struct btree_op *op, int write_lock_level)
 
 static inline void rw_lock(bool w, struct btree *b, int level)
 {
-	w ? down_write_nested(&b->lock, level + 1)
-	  : down_read_nested(&b->lock, level + 1);
+	w ? down_write(&b->lock)
+	  : down_read(&b->lock);
 	if (w)
 		b->seq++;
 }
-- 
2.39.2