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