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

[Thomas Hellstrom <thellstrom@vmware.com>]

On 10/03/2012 02:46 PM, Maarten Lankhorst wrote:
> Op 03-10-12 12:53, Thomas Hellstrom schreef:
>> On 10/03/2012 10:53 AM, Daniel Vetter wrote:
>>> On Wed, Oct 3, 2012 at 10:37 AM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>>>>>> 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.
>>>> I imagine doing the same as mutex_lock_interruptible() does in the
>>>> interrupted path should work...
>>> It simply calls the unlock lockdep annotation function if it breaks
>>> out. So doing a lock/unlock cycle in wait_unreserve should do what we
>>> want.
>>>
>>> And to properly annotate the ttm reserve paths we could just add an
>>> unconditional wait_unreserve call at the beginning like you suggested
>>> (maybe with #ifdef CONFIG_PROVE_LOCKING in case ppl freak out about
>>> the added atomic read in the uncontended case).
>>> -Daniel
>> I think atomic_read()s are cheap, at least on intel as IIRC they don't require bus locking,
>> still I think we should keep it within CONFIG_PROVE_LOCKING
>>
>> which btw reminds me there's an optimization that can be done in that one should really only
>> call atomic_cmpxchg() if a preceding atomic_read() hints that it will succeed.
>>
>> Now, does this mean TTM can keep the atomic reserve <-> lru list removal?
> I don't think it would be a good idea to keep this across devices,
Why?

>   there's currently no
> callback to remove buffers off the lru list.

So why don't we add one, and one to put them on the *correct* LRU list while
unreserving.

/Thomas