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