On 10/03/2012 10:53 AM, Daniel Vetter wrote: > On Wed, Oct 3, 2012 at 10:37 AM, Thomas Hellstrom <thellstrom at 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? Thanks, Thomas
