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 <[email protected]>
Cc: Peter Zijlstra <[email protected]> (maintainer:LOCKING PRIMITIVES)
Cc: Ingo Molnar <[email protected]> (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
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);
}
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()...)
}
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 <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
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
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 <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> ---
> 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!
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