Re: [PATCH 1/5] dma-buf: remove fallback for !CONFIG_DMA_SHARED_BUFFER

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

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.

~Maarten