From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu Date: Tue, 20 Feb 2018 12:33:18 +0100 Message-ID: <20180220113318.GN22199@phenom.ffwll.local> References: <20180215141944.4332-1-christian.koenig@amd.com> <20180219152421.GQ22199@phenom.ffwll.local> <20180219161544.GY22199@phenom.ffwll.local> <2e696981-6b4a-e292-c16a-0f3477dbf8ce@gmail.com> <20180219164354.GB22199@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: christian.koenig-5C7GfCeVMHo@public.gmane.org Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org T24gVHVlLCBGZWIgMjAsIDIwMTggYXQgMTA6NDM6NDhBTSArMDEwMCwgQ2hyaXN0aWFuIEvDtm5p ZyB3cm90ZToKPiBBbSAxOS4wMi4yMDE4IHVtIDE3OjQzIHNjaHJpZWIgRGFuaWVsIFZldHRlcjoK PiA+IE9uIE1vbiwgRmViIDE5LCAyMDE4IGF0IDA1OjI5OjQ2UE0gKzAxMDAsIENocmlzdGlhbiBL w7ZuaWcgd3JvdGU6Cj4gPiA+IFtTTklQXQo+ID4gPiBXZWxsIHRoYXQgaXMgbm90IGEgcHJvYmxl bSBhdCBhbGwuIFNlZSB3ZSBkb24ndCBuZXN0IHRyeWxvY2sgd2l0aCBub3JtYWwKPiA+ID4gbG9j ayBhY3F1aXJpbmcsIGNhdXNlIHRoYXQgd291bGQgaW5kZWVkIGJ5cGFzcyB0aGUgd2hvbGUgZGVh ZGxvY2sgZGV0ZWN0aW9uLgo+ID4gPiAKPiA+ID4gSW5zdGVhZCB3ZSBmaXJzdCB1c2Ugd3dfbXV0 ZXhfYWNxdWlyZSB0byBsb2NrIGFsbCBCT3Mgd2hpY2ggYXJlIG5lZWRlZCBmb3IgYQo+ID4gPiBj b21tYW5kIHN1Ym1pc3Npb24sIGluY2x1ZGluZyB0aGUgZGVhZGxvY2sgZGV0ZWN0aW9uLgo+ID4g PiAKPiA+ID4gVGhlbiBhbGwgYWRkaXRpb25hbCBCT3Mgd2hpY2ggbmVlZGVkIHRvIGJlIGV2aWN0 ZWQgdG8gZnVsZmlsbCB0aGUgY3VycmVudAo+ID4gPiByZXF1ZXN0IGFyZSB0cnlsb2NrZWQuCj4g PiBZZWFoIGl0J3MgdGhlIGxvY2s7IHRyeWxvY2s7IGxvY2sgY29tYm8gdGhhdCBkZWFkbG9ja3Mu IElmIGxvY2tkZXAgaW5kZWVkCj4gPiBjYXRjaGVzIHRoYXQgb25lIChhbmQgbm90IHNvbWUgb3Ro ZXIgcmVjdXJzaW9uIGNvbWJvKSB0aGVuIEkgdGhpbmsgd2UKPiA+IGRvbid0IGhhdmUgdG8gd29y cnkgYWJvdXQgaG9sZGluZyB0b25zIG9mIHRyeWxvY2snZWQgbG9ja3MuCj4gCj4gV2VsbCBJIGhh dmVuJ3QgZXhwbGljaXRseSB0ZXN0ZWQgdGhlIGxvY2s7IHRyeWxvY2s7IGxvY2sgY2FzZSwgYnV0 IHlvdSBnZXQgYQo+IHdhcm5pbmcgYW55d2F5IGluIHRoZSBsb2NrOyAuLi4gYW55dGhpbmcgLi4u OyBsb2NrIGNhc2UuCj4gCj4gU2VlIHRoZSBmaXJzdCBhbmQgdGhlIHNlY29uZCBsb2NrIGNhbid0 IHVzZSB0aGUgc2FtZSBhY3F1aXJlIGNvbnRleHQsCj4gYmVjYXVzZSB0aGF0IGlzbid0IGtub3du IGRvd24gdGhlIGNhbGwgc3RhY2sgYW5kIGxvY2tkZXAgd2FybnMgYWJvdXQgdGhhdAo+IHF1aXRl IGludGVuc2l2ZWx5LgoKQWgsIHNvIHRoZSB0dG1fY3R4IEkndmUgc3BvdHRlZCB3YXMgc29tZXRo aW5nIGVudGlyZWx5IGRpZmZlcmVudCBhbmQKZG9lc24ndCBjb250YWluIHRoZSB3d19hY3F1aXJl X2N0eCAoSSBkaWRuJ3QgY2hlY2spPyBJJ2QgYXNzdW1lIHlvdSBoYXZlCnRoZSBzYW1lIGN0eCBw YXNzZWQgYXJvdW5kIHRvIGV2ZXJ5dGhpbmcgaW4gdHRtLCBidXQgaWYgdGhhdCBkb2Vzbid0IGV4 aXN0CnRoZW4gd2UgY2FuIGluZGVlZCBub3QgYW5ub3RhdGUgd3dfbXV0ZXhfdHJ5bG9ja19jdHgg d2l0aCB0aGUgcmlnaHQgY3R4LgoKPiBXaGF0IGlzIGEgcHJvYmxlbSBpcyB0aGF0IGxvY2tkZXAg c29tZXRpbWVzIHJ1bnMgb3V0IG9mIHNwYWNlIHRvIGtlZXAgdHJhY2sKPiBvZiBhbGwgdGhlIHRy eWxvY2tlZCBtdXRleGVzLCBidXQgdGhhdCBjb3VsZCBoYXZlIGhhcHBlbmVkIGJlZm9yZSBhcyB3 ZWxsIGlmCj4gSSdtIG5vdCBjb21wbGV0ZWx5IG1pc3Rha2VuLgo+IAo+ID4gPiA+IElmIHRoaXMg aXMgcmVhbGx5IHdoYXQgeW91IHdhbnQgdG8gZG8sIHRoZW4gd2UgbmVlZCBhCj4gPiA+ID4gd3df bXV0ZXhfdHJ5bG9ja19jdHgsIHdoaWNoIGFsc28gZmlsbHMgb3V0IHRoZSBjdHggdmFsdWUgKHNv IHRoYXQgb3RoZXIKPiA+ID4gPiB0aHJlYWRzIGNhbiBjb3JyZWN0bHkgcmVzb2x2ZSBkZWFkbG9j a3Mgd2hlbiB5b3UgaG9sZCB0aGF0IGxvY2sgd2hpbGUKPiA+ID4gPiB0cnlpbmcgdG8gZ3JhYiBh ZGRpdGlvbmFsIGxvY2tzKS4gSW4gd2hpY2ggY2FzZSB5b3UgcmVhbGx5IGRvbid0IG5lZWQgdGhl Cj4gPiA+ID4gdGFzayBwb2ludGVyLgo+ID4gPiBBY3R1YWxseSBjb25zaWRlcmVkIHRoYXQgYXMg d2VsbCwgYnV0IGl0IHR1cm5lZCBvdXQgdGhhdCB0aGlzIGlzIGV4YWN0bHkKPiA+ID4gd2hhdCBJ IGRvbid0IHdhbnQuCj4gPiA+IAo+ID4gPiBDYXVzZSB0aGVuIHdlIHdvdWxkbid0IGJlIGFibGUg dG8gZGlzdGluY3Qgd3dfbXV0ZXggbG9ja2VkIHdpdGggYSBjb250ZXh0Cj4gPiA+IChiZWNhdXNl IHRoZXkgYXJlIHBhcnQgb2YgdGhlIHN1Ym1pc3Npb24pIGFuZCB3aXRob3V0IChiZWNhdXNlIFRU TSB0cnlsb2NrZWQKPiA+ID4gdGhlbSkuCj4gPiBPdXQgb2YgY3VyaW9zaXR5LCB3aHkgZG8geW91 IG5lZWQgdG8ga25vdyB0aGF0Pwo+IAo+IFRoZSBjb250cm9sIGZsb3cgaW4gVFRNIGlzIHRoYXQg d2hlbiB5b3UgdHJ5bG9ja2VkIGEgQk8geW91IHN0YXJ0IHRvIGV2aWN0Cj4gaXQuCj4gCj4gTm93 IHNvbWV0aW1lcyBpdCBoYXBwZW5zIHRoYXQgd2UgZXZpY3QgaXQgZnJvbSBWUkFNIHRvIEdUVCwg YnV0IHRoZW4gZmluZAo+IHRoYXQgd2UgZG9uJ3QgaGF2ZSBlbm91Z2ggR1RUIHNwYWNlIGFuZCBu ZWVkIHRvIGV2aWN0IHNvbWV0aGluZyBmcm9tIHRoZXJlCj4gdG8gdGhlIFNZU1RFTSBkb21haW4u Cj4gCj4gVGhlIHByb2JsZW0gaXMgbm93IHNpbmNlIHRoZSByZXNlcnZhdGlvbiBvYmplY3QgaXMg dHJ5bG9ja2VkIGJlY2F1c2Ugb2YgdGhlCj4gVlJBTSB0byBHVFQgZXZpY3Rpb24gd2UgY2FuJ3Qg bG9jayBpdCBhZ2FpbiBiZWNhdXNlIG9mIHRoZSBHVFQgdG8gdGhlIFNZU1RFTQo+IGRvbWFpbi4K PiAKPiA+ID4gPiBZZXMgaXQncyBhIGRpc2FwcG9pbnRtZW50IHRoYXQgbG9ja2RlcCBkb2Vzbid0 IGNvcnJlY3RseSB0cmFjayB0cnlsb2NrcywKPiA+ID4gPiBpdCBqdXN0IGRvZXMgYmFzaWMgc2Fu aXR5IGNoZWNrcywgYnV0IHRoZW4gZHJvcHMgdGhlbSBvbiB0aGUgZmxvb3Igd3J0Cj4gPiA+ID4g ZGVwZW5jeSB0cmFja2luZy4gSnVzdCBpbiBjYXNlIHlvdSB3b25kZXIgd2h5IHlvdSdyZSBub3Qg Z2V0dGluZyBhCj4gPiA+ID4gbG9ja2RlcHMgc3BsYXQgZm9yIHRoaXMuIFVuZm9ydHVuYXRlbHkg SSBkb24ndCB1bmRlcnN0YW5kIGxvY2tkZXAgZW5vdWdoCj4gPiA+ID4gdG8gYmUgYWJsZSB0byBm aXggdGhpcyBnYXAuCj4gPiA+IFNvcnJ5IHRvIGRpc2FwcG9pbnQgeW91LCBidXQgbG9ja2RlcCBp cyBpbmRlZWQgY2FwYWJsZSB0byBjb3JyZWN0bHkgdHJhY2sKPiA+ID4gdGhvc2UgdHJ5bG9ja2Vk IEJPcy4KPiA+ID4gCj4gPiA+IEdvdCBxdWl0ZSBhIGJ1bmNoIG9mIHdhcm5pbmcgYmVmb3JlIEkg d2FzIGFibGUgdG8gcmVzb2x2ZSB0byB0aGlzIHNvbHV0aW9uLgo+ID4gSG0sIEkgdGhvdWdodCBp dCBkaWRuJ3QgZGV0ZWN0IGEgbG9jazsgdHJ5bG9jazsgbG9jayBjb21ibyBiZWNhdXNlIHRoZQo+ ID4gdHJ5bG9jayBkaWRuJ3Qgc2hvdyB1cCBpbiB0aGUgZGVwZW5kZW5jeSBzdGFjay4gTWF5YmUg dGhhdCBnb3QgZml4ZWQKPiA+IG1lYW53aGlsZS4KPiAKPiBZZWFoIEkgY2FuIGNvbmZpcm0gdGhh dCB0aGlzIGluZGVlZCBnb3QgZml4ZWQuCj4gCj4gPiBBc3N1bWluZyB0aGF0IHdlIGluZGVlZCBu ZWVkIGJvdGgsIGNvdWxkIHdlIHNwbGl0IHVwIHRoZSB0d28gdXNlLWNhc2VzIGZvcgo+ID4gY2xh cml0eT8gSS5lLiB3d19tdXRleF9pc19vd25lZF9ieV9jdHgsIHdoaWNoIG9ubHkgdGFrZXMgdGhl IGN0eCAoYW5kCj4gPiBmb3Jnb2VzIGNoZWNraW5nIGZvciBhIHRhc2ssIHNpbmNlIHRoYXQncyBp bXBsaWVkKSBhbmQgcmVxdWlyZXMgYSBub24tTlVMTAo+ID4gY3R4LiBBbmQgd3dfbXV0ZXhfaXNf b3duZWRfYnlfdGFzaywgd2hpY2ggb25seSB0YWtlcyB0aGUgdGFzayAoYW5kIG1heWJlCj4gPiB3 ZSBzaG91bGQgbGltaXQgaXQgdG8gdHJ5bG9jayBhbmQgX3NpbmdsZSBsb2NrcywgaS5lLiBXQVJO X09OKGxvY2stPmN0eCkKPiA+IGlmIHd3LW11dGV4IGRlYnVnZ2luZyBpcyBlbmFibGVkKS4KPiA+ IAo+ID4gT3IgZG9lcyB0aGF0IGhpdCBhbm90aGVyIHJlcXVpcmVtZW50IG9mIHlvdXIgdXNlLWNh c2U/Cj4gCj4gV2VsbCB3ZSBjb3VsZCBhZGQgdHdvIHRlc3RzLCBvbmUgd2hpY2ggb25seSBjaGVj a3MgdGhlIGNvbnRleHQgYW5kIG9uZSB3aGljaAo+IGNoZWNrcyB0aGF0IHRoZSBjb250ZXh0IGlz IE5VTEwgYW5kIHRoZW4gY2hlY2tzIHRoZSBtdXRleCBvd25lci4KPiAKPiBCdXQgdG8gbWUgaXQg YWN0dWFsbHkgbG9va3MgbW9yZSBsaWtlIHRoYXQgbWFrZXMgaXQgdW5uZWNlc3NhcnkgY29tcGxp Y2F0ZWQuCj4gVGhlIHVzZSBjYXNlIGluIGFtZGdwdSB3aGljaCBjb3VsZCBvbmx5IGNoZWNrIHRo ZSBjb250ZXh0IGlzbid0IHBlcmZvcm1hbmNlCj4gY3JpdGljYWwuCgpPaCBJJ20gbm90IHdvcnJp ZWQgYWJvdXQgdGhlIHJ1bnRpbWUgb3ZlcmhlYWQgYXQgYWxsLCBJJ20gd29ycmllZCBhYm91dApj b25jZXB0dWFsIGNsYXJpdHkgb2YgdGhpcyBzdHVmZi4gSWYgeW91IGhhdmUgYSBjdHggdGhlcmUn cyBubyBuZWVkIHRvCmFsc28gbG9vayBhdCAtPm93bmVyLgoKQW5vdGhlciBpZGVhOiBXZSBkcm9w IHRoZSB0YXNrIGFyZ3VtZW50IGZyb20gZnVuY3Rpb25zIGFuZCBnbyB3aXRoIHRoZQpmb2xsb3dp bmcgbG9naWM6Cgp3d19tdXRleF9pc19vd25lcihsb2NrLCBjdHgpCnsKCWlmIChjdHgpCgkJcmV0 dXJuIGxvY2stPmN0eCA9PSBjdHg7CgllbHNlCgkJcmV0dXJuIGxvY2stPm93bmVyID09IGN1cnJl bnQ7Cn0KCkkgdGhpbmsgdGhhdCB3b3VsZCBzb2x2ZSB5b3VyIHVzZSBjYXNlLCBhbmQgZ2l2ZXMg dXMgdGhlIG5lYXQgaW50ZXJmYWNlCkknbSBhaW1pbmcgZm9yLiBLZXJuZWxkb2MgY2FuIHRoZW4g ZXhwbGFpbiB3aGF0J3MgaGFwcGVuaW5nIGZvciBhIE5VTEwKY3R4LgotRGFuaWVsCi0tIApEYW5p ZWwgVmV0dGVyClNvZnR3YXJlIEVuZ2luZWVyLCBJbnRlbCBDb3Jwb3JhdGlvbgpodHRwOi8vYmxv Zy5mZndsbC5jaApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwphbWQtZ2Z4IG1haWxpbmcgbGlzdAphbWQtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRw czovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2FtZC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751485AbeBTLdY (ORCPT ); Tue, 20 Feb 2018 06:33:24 -0500 Received: from mail-wr0-f169.google.com ([209.85.128.169]:41855 "EHLO mail-wr0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751348AbeBTLdW (ORCPT ); Tue, 20 Feb 2018 06:33:22 -0500 X-Google-Smtp-Source: AH8x2254PiCwPtg1OGII2G4w0DnW0FwLcvL9c17feAQ4JWdAwXGLDj+pKSDQHEW5Z/IT1GPekg4nlg== Date: Tue, 20 Feb 2018 12:33:18 +0100 From: Daniel Vetter To: christian.koenig@amd.com Cc: dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu Message-ID: <20180220113318.GN22199@phenom.ffwll.local> Mail-Followup-To: christian.koenig@amd.com, dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20180215141944.4332-1-christian.koenig@amd.com> <20180219152421.GQ22199@phenom.ffwll.local> <20180219161544.GY22199@phenom.ffwll.local> <2e696981-6b4a-e292-c16a-0f3477dbf8ce@gmail.com> <20180219164354.GB22199@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Operating-System: Linux phenom 4.14.0-3-amd64 User-Agent: Mutt/1.9.3 (2018-01-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 20, 2018 at 10:43:48AM +0100, Christian König wrote: > Am 19.02.2018 um 17:43 schrieb Daniel Vetter: > > On Mon, Feb 19, 2018 at 05:29:46PM +0100, Christian König wrote: > > > [SNIP] > > > Well that is not a problem at all. See we don't nest trylock with normal > > > lock acquiring, cause that would indeed bypass the whole deadlock detection. > > > > > > Instead we first use ww_mutex_acquire to lock all BOs which are needed for a > > > command submission, including the deadlock detection. > > > > > > Then all additional BOs which needed to be evicted to fulfill the current > > > request are trylocked. > > Yeah it's the lock; trylock; lock combo that deadlocks. If lockdep indeed > > catches that one (and not some other recursion combo) then I think we > > don't have to worry about holding tons of trylock'ed locks. > > Well I haven't explicitly tested the lock; trylock; lock case, but you get a > warning anyway in the lock; ... anything ...; lock case. > > See the first and the second lock can't use the same acquire context, > because that isn't known down the call stack and lockdep warns about that > quite intensively. Ah, so the ttm_ctx I've spotted was something entirely different and doesn't contain the ww_acquire_ctx (I didn't check)? I'd assume you have the same ctx passed around to everything in ttm, but if that doesn't exist then we can indeed not annotate ww_mutex_trylock_ctx with the right ctx. > What is a problem is that lockdep sometimes runs out of space to keep track > of all the trylocked mutexes, but that could have happened before as well if > I'm not completely mistaken. > > > > > If this is really what you want to do, then we need a > > > > ww_mutex_trylock_ctx, which also fills out the ctx value (so that other > > > > threads can correctly resolve deadlocks when you hold that lock while > > > > trying to grab additional locks). In which case you really don't need the > > > > task pointer. > > > Actually considered that as well, but it turned out that this is exactly > > > what I don't want. > > > > > > Cause then we wouldn't be able to distinct ww_mutex locked with a context > > > (because they are part of the submission) and without (because TTM trylocked > > > them). > > Out of curiosity, why do you need to know that? > > The control flow in TTM is that when you trylocked a BO you start to evict > it. > > Now sometimes it happens that we evict it from VRAM to GTT, but then find > that we don't have enough GTT space and need to evict something from there > to the SYSTEM domain. > > The problem is now since the reservation object is trylocked because of the > VRAM to GTT eviction we can't lock it again because of the GTT to the SYSTEM > domain. > > > > > Yes it's a disappointment that lockdep doesn't correctly track trylocks, > > > > it just does basic sanity checks, but then drops them on the floor wrt > > > > depency tracking. Just in case you wonder why you're not getting a > > > > lockdeps splat for this. Unfortunately I don't understand lockdep enough > > > > to be able to fix this gap. > > > Sorry to disappoint you, but lockdep is indeed capable to correctly track > > > those trylocked BOs. > > > > > > Got quite a bunch of warning before I was able to resolve to this solution. > > Hm, I thought it didn't detect a lock; trylock; lock combo because the > > trylock didn't show up in the dependency stack. Maybe that got fixed > > meanwhile. > > Yeah I can confirm that this indeed got fixed. > > > Assuming that we indeed need both, could we split up the two use-cases for > > clarity? I.e. ww_mutex_is_owned_by_ctx, which only takes the ctx (and > > forgoes checking for a task, since that's implied) and requires a non-NULL > > ctx. And ww_mutex_is_owned_by_task, which only takes the task (and maybe > > we should limit it to trylock and _single locks, i.e. WARN_ON(lock->ctx) > > if ww-mutex debugging is enabled). > > > > Or does that hit another requirement of your use-case? > > Well we could add two tests, one which only checks the context and one which > checks that the context is NULL and then checks the mutex owner. > > But to me it actually looks more like that makes it unnecessary complicated. > The use case in amdgpu which could only check the context isn't performance > critical. Oh I'm not worried about the runtime overhead at all, I'm worried about conceptual clarity of this stuff. If you have a ctx there's no need to also look at ->owner. Another idea: We drop the task argument from functions and go with the following logic: ww_mutex_is_owner(lock, ctx) { if (ctx) return lock->ctx == ctx; else return lock->owner == current; } I think that would solve your use case, and gives us the neat interface I'm aiming for. Kerneldoc can then explain what's happening for a NULL ctx. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch