From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com ([134.134.136.24]:54925 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754618AbdC2Jd4 (ORCPT ); Wed, 29 Mar 2017 05:33:56 -0400 Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Avoid lock dropping between rescheduling To: Chris Wilson , intel-gfx@lists.freedesktop.org References: <20170326084420.3231-1-chris@chris-wilson.co.uk> <20170327202143.7972-1-chris@chris-wilson.co.uk> Cc: "# v4 . 10+" From: Tvrtko Ursulin Message-ID: Date: Wed, 29 Mar 2017 10:33:47 +0100 MIME-Version: 1.0 In-Reply-To: <20170327202143.7972-1-chris@chris-wilson.co.uk> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: On 27/03/2017 21:21, Chris Wilson wrote: > Unlocking is dangerous. In this case we combine an early update to the > out-of-queue request, because we know that it will be inserted into the > correct FIFO priority-ordered slot when it becomes ready in the future. > However, given sufficient enthusiasm, it may become ready as we are > continuing to reschedule, and so may gazump the FIFO if we have since > dropped its spinlock. The result is that it may be executed too early, > before its dependencies. > > v2: Move all work into the second phase over the topological sort. This > removes the shortcut on the out-of-rbtree request to ensure that we only > adjust its priority after adjusting all of its dependencies. > > Fixes: 20311bd35060 ("drm/i915/scheduler: Execute requests in order of priorities") > Testcase: igt/gem_exec_whisper > Signed-off-by: Chris Wilson > Cc: Tvrtko Ursulin > Cc: # v4.10+ > --- > drivers/gpu/drm/i915/intel_lrc.c | 44 ++++++++++++++++++---------------------- > 1 file changed, 20 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index b0c3a029b592..91e38e80a095 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -665,8 +665,8 @@ pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked) > priotree)->engine; > if (engine != locked) { > if (locked) Could replace "if (locked)" with a GEM_BUG_ON(!locked) now. > - spin_unlock_irq(&locked->timeline->lock); > - spin_lock_irq(&engine->timeline->lock); > + spin_unlock(&locked->timeline->lock); > + spin_lock(&engine->timeline->lock); > } > > return engine; > @@ -674,7 +674,7 @@ pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked) > > static void execlists_schedule(struct drm_i915_gem_request *request, int prio) > { > - struct intel_engine_cs *engine = NULL; > + struct intel_engine_cs *engine; > struct i915_dependency *dep, *p; > struct i915_dependency stack; > LIST_HEAD(dfs); > @@ -708,26 +708,23 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) > list_for_each_entry_safe(dep, p, &dfs, dfs_link) { > struct i915_priotree *pt = dep->signaler; > > - list_for_each_entry(p, &pt->signalers_list, signal_link) > + /* Within an engine, there can be no cycle, but we may > + * refer to the same dependency chain multiple times > + * (redundant dependencies are not eliminated) and across > + * engines. > + */ > + list_for_each_entry(p, &pt->signalers_list, signal_link) { > + GEM_BUG_ON(p->signaler->priority < pt->priority); > if (prio > READ_ONCE(p->signaler->priority)) > list_move_tail(&p->dfs_link, &dfs); > + } > > list_safe_reset_next(dep, p, dfs_link); > - if (!RB_EMPTY_NODE(&pt->node)) > - continue; > - > - engine = pt_lock_engine(pt, engine); > - > - /* If it is not already in the rbtree, we can update the > - * priority inplace and skip over it (and its dependencies) > - * if it is referenced *again* as we descend the dfs. > - */ > - if (prio > pt->priority && RB_EMPTY_NODE(&pt->node)) { > - pt->priority = prio; > - list_del_init(&dep->dfs_link); > - } > } > > + engine = request->engine; > + spin_lock_irq(&engine->timeline->lock); > + > /* Fifo and depth-first replacement ensure our deps execute before us */ > list_for_each_entry_safe_reverse(dep, p, &dfs, dfs_link) { > struct i915_priotree *pt = dep->signaler; > @@ -739,16 +736,15 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) > if (prio <= pt->priority) > continue; > > - GEM_BUG_ON(RB_EMPTY_NODE(&pt->node)); > - > pt->priority = prio; > - rb_erase(&pt->node, &engine->execlist_queue); > - if (insert_request(pt, &engine->execlist_queue)) > - engine->execlist_first = &pt->node; > + if (!RB_EMPTY_NODE(&pt->node)) { > + rb_erase(&pt->node, &engine->execlist_queue); > + if (insert_request(pt, &engine->execlist_queue)) > + engine->execlist_first = &pt->node; > + } > } > > - if (engine) > - spin_unlock_irq(&engine->timeline->lock); > + spin_unlock_irq(&engine->timeline->lock); > > /* XXX Do we need to preempt to make room for us and our deps? */ > } > Looks OK to me. Preferably with the tidy in pt_lock_engine: Reviewed-by: Tvrtko Ursulin Regards, Tvrtko From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH v2] drm/i915: Avoid lock dropping between rescheduling Date: Wed, 29 Mar 2017 10:33:47 +0100 Message-ID: References: <20170326084420.3231-1-chris@chris-wilson.co.uk> <20170327202143.7972-1-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 388A56E11D for ; Wed, 29 Mar 2017 09:33:55 +0000 (UTC) In-Reply-To: <20170327202143.7972-1-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , intel-gfx@lists.freedesktop.org Cc: "# v4 . 10+" List-Id: intel-gfx@lists.freedesktop.org Ck9uIDI3LzAzLzIwMTcgMjE6MjEsIENocmlzIFdpbHNvbiB3cm90ZToKPiBVbmxvY2tpbmcgaXMg ZGFuZ2Vyb3VzLiBJbiB0aGlzIGNhc2Ugd2UgY29tYmluZSBhbiBlYXJseSB1cGRhdGUgdG8gdGhl Cj4gb3V0LW9mLXF1ZXVlIHJlcXVlc3QsIGJlY2F1c2Ugd2Uga25vdyB0aGF0IGl0IHdpbGwgYmUg aW5zZXJ0ZWQgaW50byB0aGUKPiBjb3JyZWN0IEZJRk8gcHJpb3JpdHktb3JkZXJlZCBzbG90IHdo ZW4gaXQgYmVjb21lcyByZWFkeSBpbiB0aGUgZnV0dXJlLgo+IEhvd2V2ZXIsIGdpdmVuIHN1ZmZp Y2llbnQgZW50aHVzaWFzbSwgaXQgbWF5IGJlY29tZSByZWFkeSBhcyB3ZSBhcmUKPiBjb250aW51 aW5nIHRvIHJlc2NoZWR1bGUsIGFuZCBzbyBtYXkgZ2F6dW1wIHRoZSBGSUZPIGlmIHdlIGhhdmUg c2luY2UKPiBkcm9wcGVkIGl0cyBzcGlubG9jay4gVGhlIHJlc3VsdCBpcyB0aGF0IGl0IG1heSBi ZSBleGVjdXRlZCB0b28gZWFybHksCj4gYmVmb3JlIGl0cyBkZXBlbmRlbmNpZXMuCj4KPiB2Mjog TW92ZSBhbGwgd29yayBpbnRvIHRoZSBzZWNvbmQgcGhhc2Ugb3ZlciB0aGUgdG9wb2xvZ2ljYWwg c29ydC4gVGhpcwo+IHJlbW92ZXMgdGhlIHNob3J0Y3V0IG9uIHRoZSBvdXQtb2YtcmJ0cmVlIHJl cXVlc3QgdG8gZW5zdXJlIHRoYXQgd2Ugb25seQo+IGFkanVzdCBpdHMgcHJpb3JpdHkgYWZ0ZXIg YWRqdXN0aW5nIGFsbCBvZiBpdHMgZGVwZW5kZW5jaWVzLgo+Cj4gRml4ZXM6IDIwMzExYmQzNTA2 MCAoImRybS9pOTE1L3NjaGVkdWxlcjogRXhlY3V0ZSByZXF1ZXN0cyBpbiBvcmRlciBvZiBwcmlv cml0aWVzIikKPiBUZXN0Y2FzZTogaWd0L2dlbV9leGVjX3doaXNwZXIKPiBTaWduZWQtb2ZmLWJ5 OiBDaHJpcyBXaWxzb24gPGNocmlzQGNocmlzLXdpbHNvbi5jby51az4KPiBDYzogVHZydGtvIFVy c3VsaW4gPHR2cnRrby51cnN1bGluQGludGVsLmNvbT4KPiBDYzogPHN0YWJsZUB2Z2VyLmtlcm5l bC5vcmc+ICMgdjQuMTArCj4gLS0tCj4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2xyYy5j IHwgNDQgKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQo+ICAxIGZpbGUg Y2hhbmdlZCwgMjAgaW5zZXJ0aW9ucygrKSwgMjQgZGVsZXRpb25zKC0pCj4KPiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfbHJjLmMgYi9kcml2ZXJzL2dwdS9kcm0vaTkx NS9pbnRlbF9scmMuYwo+IGluZGV4IGIwYzNhMDI5YjU5Mi4uOTFlMzhlODBhMDk1IDEwMDY0NAo+ IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2xyYy5jCj4gKysrIGIvZHJpdmVycy9n cHUvZHJtL2k5MTUvaW50ZWxfbHJjLmMKPiBAQCAtNjY1LDggKzY2NSw4IEBAIHB0X2xvY2tfZW5n aW5lKHN0cnVjdCBpOTE1X3ByaW90cmVlICpwdCwgc3RydWN0IGludGVsX2VuZ2luZV9jcyAqbG9j a2VkKQo+ICAJCQkgICAgICBwcmlvdHJlZSktPmVuZ2luZTsKPiAgCWlmIChlbmdpbmUgIT0gbG9j a2VkKSB7Cj4gIAkJaWYgKGxvY2tlZCkKCkNvdWxkIHJlcGxhY2UgImlmIChsb2NrZWQpIiB3aXRo IGEgR0VNX0JVR19PTighbG9ja2VkKSBub3cuCgo+IC0JCQlzcGluX3VubG9ja19pcnEoJmxvY2tl ZC0+dGltZWxpbmUtPmxvY2spOwo+IC0JCXNwaW5fbG9ja19pcnEoJmVuZ2luZS0+dGltZWxpbmUt PmxvY2spOwo+ICsJCQlzcGluX3VubG9jaygmbG9ja2VkLT50aW1lbGluZS0+bG9jayk7Cj4gKwkJ c3Bpbl9sb2NrKCZlbmdpbmUtPnRpbWVsaW5lLT5sb2NrKTsKPiAgCX0KPgo+ICAJcmV0dXJuIGVu Z2luZTsKPiBAQCAtNjc0LDcgKzY3NCw3IEBAIHB0X2xvY2tfZW5naW5lKHN0cnVjdCBpOTE1X3By aW90cmVlICpwdCwgc3RydWN0IGludGVsX2VuZ2luZV9jcyAqbG9ja2VkKQo+Cj4gIHN0YXRpYyB2 b2lkIGV4ZWNsaXN0c19zY2hlZHVsZShzdHJ1Y3QgZHJtX2k5MTVfZ2VtX3JlcXVlc3QgKnJlcXVl c3QsIGludCBwcmlvKQo+ICB7Cj4gLQlzdHJ1Y3QgaW50ZWxfZW5naW5lX2NzICplbmdpbmUgPSBO VUxMOwo+ICsJc3RydWN0IGludGVsX2VuZ2luZV9jcyAqZW5naW5lOwo+ICAJc3RydWN0IGk5MTVf ZGVwZW5kZW5jeSAqZGVwLCAqcDsKPiAgCXN0cnVjdCBpOTE1X2RlcGVuZGVuY3kgc3RhY2s7Cj4g IAlMSVNUX0hFQUQoZGZzKTsKPiBAQCAtNzA4LDI2ICs3MDgsMjMgQEAgc3RhdGljIHZvaWQgZXhl Y2xpc3RzX3NjaGVkdWxlKHN0cnVjdCBkcm1faTkxNV9nZW1fcmVxdWVzdCAqcmVxdWVzdCwgaW50 IHByaW8pCj4gIAlsaXN0X2Zvcl9lYWNoX2VudHJ5X3NhZmUoZGVwLCBwLCAmZGZzLCBkZnNfbGlu aykgewo+ICAJCXN0cnVjdCBpOTE1X3ByaW90cmVlICpwdCA9IGRlcC0+c2lnbmFsZXI7Cj4KPiAt CQlsaXN0X2Zvcl9lYWNoX2VudHJ5KHAsICZwdC0+c2lnbmFsZXJzX2xpc3QsIHNpZ25hbF9saW5r KQo+ICsJCS8qIFdpdGhpbiBhbiBlbmdpbmUsIHRoZXJlIGNhbiBiZSBubyBjeWNsZSwgYnV0IHdl IG1heQo+ICsJCSAqIHJlZmVyIHRvIHRoZSBzYW1lIGRlcGVuZGVuY3kgY2hhaW4gbXVsdGlwbGUg dGltZXMKPiArCQkgKiAocmVkdW5kYW50IGRlcGVuZGVuY2llcyBhcmUgbm90IGVsaW1pbmF0ZWQp IGFuZCBhY3Jvc3MKPiArCQkgKiBlbmdpbmVzLgo+ICsJCSAqLwo+ICsJCWxpc3RfZm9yX2VhY2hf ZW50cnkocCwgJnB0LT5zaWduYWxlcnNfbGlzdCwgc2lnbmFsX2xpbmspIHsKPiArCQkJR0VNX0JV R19PTihwLT5zaWduYWxlci0+cHJpb3JpdHkgPCBwdC0+cHJpb3JpdHkpOwo+ICAJCQlpZiAocHJp byA+IFJFQURfT05DRShwLT5zaWduYWxlci0+cHJpb3JpdHkpKQo+ICAJCQkJbGlzdF9tb3ZlX3Rh aWwoJnAtPmRmc19saW5rLCAmZGZzKTsKPiArCQl9Cj4KPiAgCQlsaXN0X3NhZmVfcmVzZXRfbmV4 dChkZXAsIHAsIGRmc19saW5rKTsKPiAtCQlpZiAoIVJCX0VNUFRZX05PREUoJnB0LT5ub2RlKSkK PiAtCQkJY29udGludWU7Cj4gLQo+IC0JCWVuZ2luZSA9IHB0X2xvY2tfZW5naW5lKHB0LCBlbmdp bmUpOwo+IC0KPiAtCQkvKiBJZiBpdCBpcyBub3QgYWxyZWFkeSBpbiB0aGUgcmJ0cmVlLCB3ZSBj YW4gdXBkYXRlIHRoZQo+IC0JCSAqIHByaW9yaXR5IGlucGxhY2UgYW5kIHNraXAgb3ZlciBpdCAo YW5kIGl0cyBkZXBlbmRlbmNpZXMpCj4gLQkJICogaWYgaXQgaXMgcmVmZXJlbmNlZCAqYWdhaW4q IGFzIHdlIGRlc2NlbmQgdGhlIGRmcy4KPiAtCQkgKi8KPiAtCQlpZiAocHJpbyA+IHB0LT5wcmlv cml0eSAmJiBSQl9FTVBUWV9OT0RFKCZwdC0+bm9kZSkpIHsKPiAtCQkJcHQtPnByaW9yaXR5ID0g cHJpbzsKPiAtCQkJbGlzdF9kZWxfaW5pdCgmZGVwLT5kZnNfbGluayk7Cj4gLQkJfQo+ICAJfQo+ Cj4gKwllbmdpbmUgPSByZXF1ZXN0LT5lbmdpbmU7Cj4gKwlzcGluX2xvY2tfaXJxKCZlbmdpbmUt PnRpbWVsaW5lLT5sb2NrKTsKPiArCj4gIAkvKiBGaWZvIGFuZCBkZXB0aC1maXJzdCByZXBsYWNl bWVudCBlbnN1cmUgb3VyIGRlcHMgZXhlY3V0ZSBiZWZvcmUgdXMgKi8KPiAgCWxpc3RfZm9yX2Vh Y2hfZW50cnlfc2FmZV9yZXZlcnNlKGRlcCwgcCwgJmRmcywgZGZzX2xpbmspIHsKPiAgCQlzdHJ1 Y3QgaTkxNV9wcmlvdHJlZSAqcHQgPSBkZXAtPnNpZ25hbGVyOwo+IEBAIC03MzksMTYgKzczNiwx NSBAQCBzdGF0aWMgdm9pZCBleGVjbGlzdHNfc2NoZWR1bGUoc3RydWN0IGRybV9pOTE1X2dlbV9y ZXF1ZXN0ICpyZXF1ZXN0LCBpbnQgcHJpbykKPiAgCQlpZiAocHJpbyA8PSBwdC0+cHJpb3JpdHkp Cj4gIAkJCWNvbnRpbnVlOwo+Cj4gLQkJR0VNX0JVR19PTihSQl9FTVBUWV9OT0RFKCZwdC0+bm9k ZSkpOwo+IC0KPiAgCQlwdC0+cHJpb3JpdHkgPSBwcmlvOwo+IC0JCXJiX2VyYXNlKCZwdC0+bm9k ZSwgJmVuZ2luZS0+ZXhlY2xpc3RfcXVldWUpOwo+IC0JCWlmIChpbnNlcnRfcmVxdWVzdChwdCwg JmVuZ2luZS0+ZXhlY2xpc3RfcXVldWUpKQo+IC0JCQllbmdpbmUtPmV4ZWNsaXN0X2ZpcnN0ID0g JnB0LT5ub2RlOwo+ICsJCWlmICghUkJfRU1QVFlfTk9ERSgmcHQtPm5vZGUpKSB7Cj4gKwkJCXJi X2VyYXNlKCZwdC0+bm9kZSwgJmVuZ2luZS0+ZXhlY2xpc3RfcXVldWUpOwo+ICsJCQlpZiAoaW5z ZXJ0X3JlcXVlc3QocHQsICZlbmdpbmUtPmV4ZWNsaXN0X3F1ZXVlKSkKPiArCQkJCWVuZ2luZS0+ ZXhlY2xpc3RfZmlyc3QgPSAmcHQtPm5vZGU7Cj4gKwkJfQo+ICAJfQo+Cj4gLQlpZiAoZW5naW5l KQo+IC0JCXNwaW5fdW5sb2NrX2lycSgmZW5naW5lLT50aW1lbGluZS0+bG9jayk7Cj4gKwlzcGlu X3VubG9ja19pcnEoJmVuZ2luZS0+dGltZWxpbmUtPmxvY2spOwo+Cj4gIAkvKiBYWFggRG8gd2Ug bmVlZCB0byBwcmVlbXB0IHRvIG1ha2Ugcm9vbSBmb3IgdXMgYW5kIG91ciBkZXBzPyAqLwo+ICB9 Cj4KCkxvb2tzIE9LIHRvIG1lLiBQcmVmZXJhYmx5IHdpdGggdGhlIHRpZHkgaW4gcHRfbG9ja19l bmdpbmU6CgpSZXZpZXdlZC1ieTogVHZydGtvIFVyc3VsaW4gPHR2cnRrby51cnN1bGluQGludGVs LmNvbT4KClJlZ2FyZHMsCgpUdnJ0a28KX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX18KSW50ZWwtZ2Z4IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJl ZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGlu Zm8vaW50ZWwtZ2Z4Cg==