On Mon, May 27, 2019 at 05:02:51PM +0200, Peter Zijlstra wrote:
> 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?
Yep, works this way, and yes thought later that canceling *first_merged
for idx!=first_idx was a bit strange (even if it still worked).
>
> --- 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()
