On Mon, Mar 20, 2023 at 01:13:05PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 16, 2023 at 08:13:36PM -0700, Boqun Feng wrote:
> > Lock scenario print is always a weak spot of lockdep splats. Improvement
> > can be made if we rework the dependency search and the error printing.
> >
> > However without touching the graph search, we can improve a little for
> > the circular deadlock case, since we have the to-be-added lock
> > dependency, and know whether these two locks are read/write/sync.
> >
> > In order to know whether a held_lock is sync or not, a bit was
> > "stolen" from ->references, which reduce our limit for the same lock
> > class nesting from 2^12 to 2^11, and it should still be good enough.
> >
> > Besides, since we now have bit in held_lock for sync, we don't need the
> > "hardirqoffs being 1" trick, and also we can avoid the __lock_release()
> > if we jump out of __lock_acquire() before the held_lock stored.
> >
> > With these changes, a deadlock case evolved with read lock and sync gets
> > a better print-out from:
> >
> > [...] Possible unsafe locking scenario:
> > [...]
> > [...] CPU0 CPU1
> > [...] ---- ----
> > [...] lock(srcuA);
> > [...] lock(srcuB);
> > [...] lock(srcuA);
> > [...] lock(srcuB);
> >
> > to
> >
> > [...] Possible unsafe locking scenario:
> > [...]
> > [...] CPU0 CPU1
> > [...] ---- ----
> > [...] rlock(srcuA);
> > [...] lock(srcuB);
> > [...] lock(srcuA);
> > [...] sync(srcuB);
> >
> > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> > ---
> > include/linux/lockdep.h | 3 ++-
> > kernel/locking/lockdep.c | 48 ++++++++++++++++++++++++++--------------
> > 2 files changed, 34 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> > index 14d9dbedc6c1..b32256e9e944 100644
> > --- a/include/linux/lockdep.h
> > +++ b/include/linux/lockdep.h
> > @@ -134,7 +134,8 @@ struct held_lock {
> > unsigned int read:2; /* see lock_acquire() comment */
> > unsigned int check:1; /* see lock_acquire() comment */
> > unsigned int hardirqs_off:1;
> > - unsigned int references:12; /* 32 bits */
> > + unsigned int sync:1;
> > + unsigned int references:11; /* 32 bits */
> > unsigned int pin_count;
> > };
> >
>
> Yeah, I suppose we can do that -- another option is to steal some bits
> from pin_count, but whatever (references used to be 11 a long while ago,
> no problem going back to that).
Thanks!
>
> Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
Applied locally.
Regards,
Boqun
