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

From: Peter Zijlstra
Date: Mon Feb 20 2023 - 10:11:35 EST


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);
}