On 10/03/2012 09:57 AM, Maarten Lankhorst wrote:
Hey,
Op 03-10-12 09:45, Thomas Hellstrom schreef:
On 10/02/2012 10:03 AM, Daniel Vetter wrote:
On Tue, Oct 02, 2012 at 08:46:32AM +0200, Thomas Hellstrom wrote:
On 10/01/2012 11:47 AM, Maarten Lankhorst wrote:
I was doing a evil hack where I 'released' lru_lock to lockdep before doing the annotation for a blocking acquire, and left trylock annotations as they were. This made lockdep do the right thing.
I've never looked into how lockdep works. Is this something that can be done permanently or just for testing purposes? Although not related to this, is it possible to do something similar to the trylock reversal in the fault() code where mmap_sem() and reserve() change order using a reserve trylock?
lockdep just requires a bunch of annotations, is a compile-time configure option CONFIG_PROVE_LOCKING and if disabled, has zero overhead. And it's rather awesome in detected deadlocks and handling crazy locking schemes correctly:
- correctly handles trylocks
- correctly handles nested locking (i.e. grabbing a global lock, then grabbing subordinate locks in an unordered sequence since the global lock ensures that no deadlocks can happen).
- any kinds of inversions with special contexts like hardirq, softirq
- same for page-reclaim, i.e. it will yell if you could (potentially) deadlock because your shrinker grabs a lock that you hold while calling kmalloc.
- there are special annotates for various subsystems, e.g. to check for del_timer_sync vs. locks held by that timer. Or the console_lock annotations I've just recently submitted.
- all that with a really flexible set of annotation primitives that afaics should work for almost any insane locking scheme. The fact that Maarten could come up with proper reservation annotations without any changes to lockdep testifies this (he only had to fix a tiny thing to make it a bit more strict in a corner case).
In short I think it's made of awesome. The only downside is that it lacks documentation, you have to read the code to understand it :(
The reason I've suggested to Maarten to abolish the trylock_reservation within the lru_lock is that in that way lockdep only ever sees the trylock, and hence is less strict about complainig about deadlocks. But semantically it's an unconditional reserve. Maarten had some horrible hacks that leaked the lockdep annotations out of the new reservation code, which allowed ttm to be properly annotated. But those also reduced the usefulness for any other users of the reservation code, and so Maarten looked into whether he could remove that trylock dance in ttm.
Imo having excellent lockdep support for cross-device reservations is a requirment, and ending up with less strict annotations for either ttm based drivers or other drivers is not good. And imo the ugly layering that Maarten had in his first proof-of-concept also indicates that something is amiss in the design.
So if I understand you correctly, the reservation changes in TTM are motivated by the fact that otherwise, in the generic reservation code, lockdep can only be annotated for a trylock and not a waiting lock, when it *is* in fact a waiting lock.
I'm completely unfamiliar with setting up lockdep annotations, but the only place a deadlock might occur is if the trylock fails and we do a wait_for_unreserve(). Isn't it possible to annotate the call to wait_for_unreserve() just like an interruptible waiting lock (that is always interrupted, but at least any deadlock will be catched?).
That would not find all bugs, lockdep is meant to find even theoretical bugs, so annotating it as a waiting lock makes more sense. Otherwise lockdep will only barf when the initial trylock fails.
Really, starting a waiting reserve with a call to wait_for_unreserve() if CONFIG_LOCKDEP is set shouldn't be that bad :)? That would catch also the the theoretical errors. In fact, it should suffice with annotating for such a call?
/Thomas
~Maarten