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

[Thomas Hellstrom]

On 10/03/2012 09:57 AM, Maarten Lankhorst wrote:
> Hey,
>
> Op 03-10-12 09:45, Thomas Hellstrom schreef:
> > 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?).
> That would not find all bugs, lockdep is meant to find even theoretical bugs, so annotating it as a
> waiting lock makes more sense. Otherwise lockdep will only barf when the initial trylock fails.

Really, starting a waiting reserve with a call to wait_for_unreserve() 
if CONFIG_LOCKDEP is set
shouldn't be that bad :)? That would catch also the the theoretical errors.
In fact, it should suffice with annotating for such a call?

/Thomas

> ~Maarten

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/