Re: [RFC] Are you good with Lockdep?

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 11 Nov 2020 11:54:41 +0100
Ingo Molnar <mingo@kernel.org> wrote:

> * Byungchul Park <byungchul.park@lge.com> wrote:
> 
> > PROBLEM 1) First of all, Lockdep gets disabled on the first detection.  
> 
> Lockdep disabling itself after the first warning was an intentional 
> and deliberate design decision. (See more details below.)
> 

[..]

> Making the code simpler is always welcome, but I disagree with 
> enabling multiple warnings, for the technical reasons outlined above.

I 100% agree with Ingo. I wish we could stop *all* warnings after the first
one. The number of times people sent me bug reports with warnings without
showing me previous warnings that caused me to go on wild goose chases is
too many to count. The first warning is the *only* thing I look at.
Anything after that is likely to be caused by the integrity of the system
being compromised by the first bug.

And this is especially true with lockdep, because lockdep only detects the
deadlock, it doesn't tell you which lock was the incorrect locking.

For example. If we have a locking chain of:

 A -> B -> D

 A -> C -> D

Which on a correct system looks like this:

 lock(A)
 lock(B)
 unlock(B)
 unlock(A)

 lock(B)
 lock(D)
 unlock(D)
 unlock(B)

 lock(A)
 lock(C)
 unlock(C)
 unlock(A)

 lock(C)
 lock(D)
 unlock(D)
 unlock(C)

which creates the above chains in that order.

But, lets say we have a bug and the system boots up doing:

 lock(D)
 lock(A)
 unlock(A)
 unlock(D)

which creates the incorrect chain.

 D -> A


Now you do the correct locking:

 lock(A)
 lock(B)

Creates A -> B

 lock(A)
 lock(C)

Creates A -> C

 lock(B)
 lock(D)

Creates B -> D and lockdep detects:

 D -> A -> B -> D

and gives us the lockdep splat!!!

But we don't disable lockdep. We let it continue...

 lock(C)
 lock(D)

Which creates C -> D

Now it explodes with D -> A -> C -> D

Which it already reported. And it can be much more complex when dealing
with interrupt contexts and longer chains. That is, perhaps a different
chain had a missing irq disable, now you might get 5 or 6 more lockdep
splats because of that one bug.

The point I'm making is that the lockdep splats after the first one may
just be another version of the same bug and not a new one. Worse, if you
only look at the later lockdep splats, it may be much more difficult to
find the original bug than if you just had the first one. Believe me, I've
been down that road too many times!

And it can be very difficult to know if new lockdep splats are not the same
bug, and this will waste a lot of developers time!

This is why the decision to disable lockdep after the first splat was made.
There were times I wanted to check locking somewhere, but is was using
linux-next which had a lockdep splat that I didn't care about. So I
made it not disable lockdep. And then I hit this exact scenario, that the
one incorrect chain was causing reports all over the place. To solve it, I
had to patch the incorrect chain to do raw locking to have lockdep ignore
it ;-) Then I was able to test the code I was interested in.

> 
> > PROBLEM 2) Lockdep forces us to emulate lock acquisition for non-lock.  
> 
> >    I have the patch set. Let me share it with you in a few days.  
> 
> Not sure I understand the "problem 2)" outlined here, but I'm looking 
> forward to your patchset!
> 

I think I understand it. For things like completions and other "wait for
events" we have lockdep annotation, but it is rather awkward to implement.
Having something that says "lockdep_wait_event()" and
"lockdep_exec_event()" wrappers would be useful.

-- Steve