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 >
