Re: [PATCH v2 1/2] lockdep: Fix OOO unlock when hlocks need merging

[Peter Zijlstra]

On Fri, May 24, 2019 at 11:15:08PM +0300, Imre Deak wrote:
> 
> 	ww_mutex_lock(&ww_lock_a, &ww_ctx);
> 
> 	mutex_lock(&lock_c);
> 
> 	ww_mutex_lock(&ww_lock_b, &ww_ctx);
> 
> 	mutex_unlock(&lock_c);		(*)

> triggers the following WARN in __lock_release() when doing the unlock at *:
> 
> 	DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth - 1);
> 
> The problem is that the WARN check doesn't take into account the merging
> of ww_lock_a and ww_lock_b which results in decreasing curr->lockdep_depth
> by 2 not only 1.

Cute...

> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index c40fba54e324..967352d32af1 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -3714,7 +3714,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
>  				hlock->references = 2;
>  			}
>  
> -			return 1;
> +			return 2;
>  		}
>  	}
>  
> @@ -3920,22 +3920,33 @@ static struct held_lock *find_held_lock(struct task_struct *curr,
>  }
>  
>  static int reacquire_held_locks(struct task_struct *curr, unsigned int depth,
> -			      int idx)
> +				int idx, bool *first_merged)
>  {
>  	struct held_lock *hlock;
> +	int first_idx = idx;
>  
>  	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
>  		return 0;
>  
>  	for (hlock = curr->held_locks + idx; idx < depth; idx++, hlock++) {
> -		if (!__lock_acquire(hlock->instance,
> +		switch (__lock_acquire(hlock->instance,
>  				    hlock_class(hlock)->subclass,
>  				    hlock->trylock,
>  				    hlock->read, hlock->check,
>  				    hlock->hardirqs_off,
>  				    hlock->nest_lock, hlock->acquire_ip,
> -				    hlock->references, hlock->pin_count))
> +				    hlock->references, hlock->pin_count)) {
> +		case 0:
>  			return 1;
> +		case 1:
> +			break;
> +		case 2:
> +			*first_merged = idx == first_idx;
> +			break;
> +		default:
> +			WARN_ON(1);
> +			return 0;
> +		}
>  	}
>  	return 0;
>  }

Does it work for you if I change it like so?

--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3712,7 +3712,7 @@ static int __lock_acquire(struct lockdep
 				hlock->references = 2;
 			}
 
-			return 1;
+			return 2;
 		}
 	}
 
@@ -3918,22 +3918,33 @@ static struct held_lock *find_held_lock(
 }
 
 static int reacquire_held_locks(struct task_struct *curr, unsigned int depth,
-			      int idx)
+				int idx, unsigned int *merged)
 {
 	struct held_lock *hlock;
+	int first_idx = idx;
 
 	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
 		return 0;
 
 	for (hlock = curr->held_locks + idx; idx < depth; idx++, hlock++) {
-		if (!__lock_acquire(hlock->instance,
+		switch (__lock_acquire(hlock->instance,
 				    hlock_class(hlock)->subclass,
 				    hlock->trylock,
 				    hlock->read, hlock->check,
 				    hlock->hardirqs_off,
 				    hlock->nest_lock, hlock->acquire_ip,
-				    hlock->references, hlock->pin_count))
+				    hlock->references, hlock->pin_count)) {
+		case 0:
 			return 1;
+		case 1:
+			break;
+		case 2:
+			*merged += (idx == first_idx);
+			break;
+		default:
+			WARN_ON(1);
+			return 0;
+		}
 	}
 	return 0;
 }
@@ -3944,9 +3955,9 @@ __lock_set_class(struct lockdep_map *loc
 		 unsigned long ip)
 {
 	struct task_struct *curr = current;
+	unsigned int depth, merged = 0
 	struct held_lock *hlock;
 	struct lock_class *class;
-	unsigned int depth;
 	int i;
 
 	if (unlikely(!debug_locks))
@@ -3971,14 +3982,14 @@ __lock_set_class(struct lockdep_map *loc
 	curr->lockdep_depth = i;
 	curr->curr_chain_key = hlock->prev_chain_key;
 
-	if (reacquire_held_locks(curr, depth, i))
+	if (reacquire_held_locks(curr, depth, i, &merged))
 		return 0;
 
 	/*
 	 * I took it apart and put it back together again, except now I have
 	 * these 'spare' parts.. where shall I put them.
 	 */
-	if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth))
+	if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth - merged))
 		return 0;
 	return 1;
 }
@@ -3986,8 +3997,8 @@ __lock_set_class(struct lockdep_map *loc
 static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip)
 {
 	struct task_struct *curr = current;
+	unsigned int depth, merged = 0;
 	struct held_lock *hlock;
-	unsigned int depth;
 	int i;
 
 	if (unlikely(!debug_locks))
@@ -4012,7 +4023,7 @@ static int __lock_downgrade(struct lockd
 	hlock->read = 1;
 	hlock->acquire_ip = ip;
 
-	if (reacquire_held_locks(curr, depth, i))
+	if (reacquire_held_locks(curr, depth, i, &merged))
 		return 0;
 
 	/*
@@ -4021,6 +4032,11 @@ static int __lock_downgrade(struct lockd
 	 */
 	if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth))
 		return 0;
+
+	/* Merging can't happen with unchanged classes.. */
+	if (DEBUG_LOCKS_WARN_ON(merged))
+		return 0;
+
 	return 1;
 }
 
@@ -4035,8 +4051,8 @@ static int
 __lock_release(struct lockdep_map *lock, int nested, unsigned long ip)
 {
 	struct task_struct *curr = current;
+	unsigned int depth, merged = 1;
 	struct held_lock *hlock;
-	unsigned int depth;
 	int i;
 
 	if (unlikely(!debug_locks))
@@ -4091,14 +4107,15 @@ __lock_release(struct lockdep_map *lock,
 	if (i == depth-1)
 		return 1;
 
-	if (reacquire_held_locks(curr, depth, i + 1))
+	if (reacquire_held_locks(curr, depth, i + 1, &merged))
 		return 0;
 
 	/*
 	 * We had N bottles of beer on the wall, we drank one, but now
 	 * there's not N-1 bottles of beer left on the wall...
+	 * Pouring two of the bottles together is acceptable.
 	 */
-	DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth-1);
+	DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth - merged);
 
 	/*
 	 * Since reacquire_held_locks() would have called check_chain_key()