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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
