RFC [PATCH 0/1] Fix lockdep false positive

RFC [PATCH 0/1] Fix lockdep false positive

[Derek Basehore]

I'm not sure if I'm breaking any detection with this patch since I
haven't looked at that lockdep code before. I do know that the unlock
order for locks with a nest lock should not matter, though.
Specifically, you should be able to unlock the nest lock followed by
all the locks nested underneath it.

Derek Basehore (1):
  locking/lockdep: Fix nest lock warning on unlock

 kernel/locking/lockdep.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

-- 
2.20.0.rc2.403.gdbc3b29805-goog

RFC [PATCH 1/1] locking/lockdep: Fix nest lock warning on unlock

[Derek Basehore]

The function __lock_acquire checks that the nest lock is held passed
in as an argument. The issue with this is that __lock_acquire is used
for internal bookkeeping on lock_release. This produces a false
positive lockdep warning on unlock. Since you explicitly don't need to
hold the nest lock on unlock, this is an issue.

This fixes the problem by only checking the nest lock on the actual
lock acquire step.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 kernel/locking/lockdep.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 1efada2dd9dd..2e7297ee6596 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3155,15 +3155,15 @@ EXPORT_SYMBOL_GPL(lockdep_init_map);
 struct lock_class_key __lockdep_no_validate__;
 EXPORT_SYMBOL_GPL(__lockdep_no_validate__);
 
-static int
-print_lock_nested_lock_not_held(struct task_struct *curr,
-				struct held_lock *hlock,
+static void
+print_lock_nested_lock_not_held(struct lockdep_map *lock,
+				struct lockdep_map *nest_lock,
 				unsigned long ip)
 {
 	if (!debug_locks_off())
-		return 0;
+		return;
 	if (debug_locks_silent)
-		return 0;
+		return;
 
 	pr_warn("\n");
 	pr_warn("==================================\n");
@@ -3171,22 +3171,21 @@ print_lock_nested_lock_not_held(struct task_struct *curr,
 	print_kernel_ident();
 	pr_warn("----------------------------------\n");
 
-	pr_warn("%s/%d is trying to lock:\n", curr->comm, task_pid_nr(curr));
-	print_lock(hlock);
+	pr_warn("%s/%d is trying to lock:\n", current->comm,
+			task_pid_nr(current));
+	pr_warn("%s\n", lock->name);
 
 	pr_warn("\nbut this task is not holding:\n");
-	pr_warn("%s\n", hlock->nest_lock->name);
+	pr_warn("%s\n", nest_lock->name);
 
 	pr_warn("\nstack backtrace:\n");
 	dump_stack();
 
 	pr_warn("\nother info that might help us debug this:\n");
-	lockdep_print_held_locks(curr);
+	lockdep_print_held_locks(current);
 
 	pr_warn("\nstack backtrace:\n");
 	dump_stack();
-
-	return 0;
 }
 
 static int __lock_is_held(const struct lockdep_map *lock, int read);
@@ -3335,9 +3334,6 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	}
 	chain_key = iterate_chain_key(chain_key, class_idx);
 
-	if (nest_lock && !__lock_is_held(nest_lock, -1))
-		return print_lock_nested_lock_not_held(curr, hlock, ip);
-
 	if (!validate_chain(curr, lock, hlock, chain_head, chain_key))
 		return 0;
 
@@ -3843,6 +3839,9 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
 	__lock_acquire(lock, subclass, trylock, read, check,
 		       irqs_disabled_flags(flags), nest_lock, ip, 0, 0);
+	if (nest_lock && !__lock_is_held(nest_lock, -1))
+		print_lock_nested_lock_not_held(lock, nest_lock, ip);
+
 	current->lockdep_recursion = 0;
 	raw_local_irq_restore(flags);
 }
-- 
2.20.0.rc2.403.gdbc3b29805-goog

Re: RFC [PATCH 1/1] locking/lockdep: Fix nest lock warning on unlock

[Peter Zijlstra]

On Tue, Dec 11, 2018 at 06:25:06PM -0800, Derek Basehore wrote:
> The function __lock_acquire checks that the nest lock is held passed
> in as an argument. The issue with this is that __lock_acquire is used
> for internal bookkeeping on lock_release. This produces a false
> positive lockdep warning on unlock. Since you explicitly don't need to
> hold the nest lock on unlock, this is an issue.

Who is triggering this?

Re: RFC [PATCH 1/1] locking/lockdep: Fix nest lock warning on unlock

[dbasehore .]

On Wed, Dec 12, 2018 at 1:09 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Dec 11, 2018 at 06:25:06PM -0800, Derek Basehore wrote:
> > The function __lock_acquire checks that the nest lock is held passed
> > in as an argument. The issue with this is that __lock_acquire is used
> > for internal bookkeeping on lock_release. This produces a false
> > positive lockdep warning on unlock. Since you explicitly don't need to
> > hold the nest lock on unlock, this is an issue.
>
> Who is triggering this?

I'm writing code right now that triggers this. The goal is to reduce
lock contention in the Common Clk Framework by getting rid of its
global lock. One of the approaches I'm considering is adding a nest
lock with n "subtree" locks. After locking all of the needed subtree
locks, it's important to unlock the nest lock to reduce contention.