Date:   Mon, 20 Feb 2023 16:09:33 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Kent Overstreet <kent.overstreet@linux.dev>
Cc:     linux-kernel@vger.kernel.org, mingo@redhat.com,
        stern@rowland.harvard.edu
Subject: Re: [PATCH 1/2] lockdep: lock_set_lock_cmp_fn()
Message-ID: <Y/ONLYqVrWPFyboF@hirez.programming.kicks-ass.net>
References: <20230218032117.2372071-1-kent.overstreet@linux.dev>
 <20230218032117.2372071-2-kent.overstreet@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230218032117.2372071-2-kent.overstreet@linux.dev>
Precedence: bulk

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