From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH 1/5] dma-buf: remove fallback for !CONFIG_DMA_SHARED_BUFFER Date: Wed, 03 Oct 2012 09:45:41 +0200 Message-ID: <506BED25.2060804@vmware.com> References: <20120928124148.14366.21063.stgit@patser.local> <5065B0C9.7040209@canonical.com> <5065FDAA.5080103@vmware.com> <50696699.7020009@canonical.com> <506A8DC8.5020706@vmware.com> <20121002080341.GA5679@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-outbound-2.vmware.com (smtp-outbound-2.vmware.com [208.91.2.13]) by gabe.freedesktop.org (Postfix) with ESMTP id 849529E82D for ; Wed, 3 Oct 2012 00:45:47 -0700 (PDT) In-Reply-To: <20121002080341.GA5679@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Maarten Lankhorst , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, sumit.semwal@linaro.org, linux-media@vger.kernel.org, Daniel Vetter List-Id: dri-devel@lists.freedesktop.org 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?). /Thomas