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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Oct 3, 2012 at 9:45 AM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> 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?).

Hm, I have to admit that idea hasn't crossed my mind, but it's indeed
a hole in our current reservation lockdep annotations - since we're
blocking for the unreserve, other threads could potential block
waiting on us to release a lock we're holding already, resulting in a
deadlock.

Since no other locking primitive that I know of has this
wait_for_unlocked interface, I don't know how we could map this in
lockdep. One idea is to grab the lock and release it again immediately
(only in the annotations, not the real lock ofc). But I need to check
the lockdep code to see whether that doesn't trip it up.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch