From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH 1/2] drm/i915: Break busywaiting for requests on pending signals Date: Mon, 16 Nov 2015 09:54:10 +0000 Message-ID: <5649A7C2.90206@linux.intel.com> References: <1447594364-4206-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1447594364-4206-1-git-send-email-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 , Jens Axboe , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: Daniel Vetter , Eero Tamminen , "Rantala, Valtteri" , stable@kernel.vger.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org CkhpLAoKT24gMTUvMTEvMTUgMTM6MzIsIENocmlzIFdpbHNvbiB3cm90ZToKPiBUaGUgYnVzeXdh aXQgaW4gX19pOTE1X3NwaW5fcmVxdWVzdCgpIGRvZXMgbm90IHJlc3BlY3QgcGVuZGluZyBzaWdu YWxzCj4gYW5kIHNvIG1heSBjb25zdW1lIHRoZSBlbnRpcmUgdGltZXNsaWNlIGZvciB0aGUgdGFz ayBpbnN0ZWFkIG9mCj4gcmV0dXJuaW5nIHRvIHVzZXJzcGFjZSB0byBoYW5kbGUgdGhlIHNpZ25h bC4KCk9idmlvdXNseSBjb3JyZWN0IHRvIGJyZWFrIHRoZSBzcGluLCBidXQgaWYgc3BlbmRpbmcg YSBqaWZmaWUgdG8gcmVhY3QgCnRvIHNpZ25hbHMgd2FzIHRoZSBvbmx5IHByb2JsZW0gdGhlbiBp dCBpcyBub3QgdG9vIHNldmVyZS4KCkFkZCBzb21ldGhpbmcgdG8gdGhlIGNvbW1pdCBtZXNzYWdl IGFib3V0IGhvdyBpdCB3YXMgZm91bmQvcmVwb3J0ZWQgYW5kIAphYm91dCB0aGUgc2V2ZXJpdHkg b2YgaW1wYWN0LCBldGM/CgpPdGhlcndpc2UsCgpSZXZpZXdlZC1ieTogVHZydGtvIFVyc3VsaW4g PHR2cnRrby51cnN1bGluQGludGVsLmNvbT4KCj4gRml4ZXMgcmVncmVzc2lvbiBmcm9tCj4gY29t bWl0IDJkZWY0YWQ5OWJlZmEyNTc3NWRkMmY3MTRmZGQ0ZDkyZmFlYzZlMzQgW3Y0LjJdCj4gQXV0 aG9yOiBDaHJpcyBXaWxzb24gPGNocmlzQGNocmlzLXdpbHNvbi5jby51az4KPiBEYXRlOiAgIFR1 ZSBBcHIgNyAxNjoyMDo0MSAyMDE1ICswMTAwCj4KPiAgICAgICBkcm0vaTkxNTogT3B0aW1pc3Rp Y2FsbHkgc3BpbiBmb3IgdGhlIHJlcXVlc3QgY29tcGxldGlvbgo+Cj4gU2lnbmVkLW9mZi1ieTog Q2hyaXMgV2lsc29uIDxjaHJpc0BjaHJpcy13aWxzb24uY28udWs+Cj4gQ2M6IEplbnMgQXhib2Ug PGF4Ym9lQGtlcm5lbC5kaz4KPiBDYzsgIlJvZ296aGtpbiwgRG1pdHJ5IFYiIDxkbWl0cnkudi5y b2dvemhraW5AaW50ZWwuY29tPgo+IENjOiBEYW5pZWwgVmV0dGVyIDxkYW5pZWwudmV0dGVyQGZm d2xsLmNoPgo+IENjOiBUdnJ0a28gVXJzdWxpbiA8dHZydGtvLnVyc3VsaW5AbGludXguaW50ZWwu Y29tPgo+IENjOiBFZXJvIFRhbW1pbmVuIDxlZXJvLnQudGFtbWluZW5AaW50ZWwuY29tPgo+IENj OiAiUmFudGFsYSwgVmFsdHRlcmkiIDx2YWx0dGVyaS5yYW50YWxhQGludGVsLmNvbT4KPiBDYzog c3RhYmxlQGtlcm5lbC52Z2VyLm9yZwo+IC0tLQo+ICAgZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkx NV9nZW0uYyB8IDEzICsrKysrKysrLS0tLS0KPiAgIDEgZmlsZSBjaGFuZ2VkLCA4IGluc2VydGlv bnMoKyksIDUgZGVsZXRpb25zKC0pCj4KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5 MTUvaTkxNV9nZW0uYyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZ2VtLmMKPiBpbmRleCA1 Y2Y0YTE5OTgyNzMuLjc0MDUzMGM1NzFkMSAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0v aTkxNS9pOTE1X2dlbS5jCj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9nZW0uYwo+ IEBAIC0xMTQ2LDcgKzExNDYsNyBAQCBzdGF0aWMgYm9vbCBtaXNzZWRfaXJxKHN0cnVjdCBkcm1f aTkxNV9wcml2YXRlICpkZXZfcHJpdiwKPiAgIAlyZXR1cm4gdGVzdF9iaXQocmluZy0+aWQsICZk ZXZfcHJpdi0+Z3B1X2Vycm9yLm1pc3NlZF9pcnFfcmluZ3MpOwo+ICAgfQo+Cj4gLXN0YXRpYyBp bnQgX19pOTE1X3NwaW5fcmVxdWVzdChzdHJ1Y3QgZHJtX2k5MTVfZ2VtX3JlcXVlc3QgKnJlcSkK PiArc3RhdGljIGludCBfX2k5MTVfc3Bpbl9yZXF1ZXN0KHN0cnVjdCBkcm1faTkxNV9nZW1fcmVx dWVzdCAqcmVxLCBpbnQgc3RhdGUpCj4gICB7Cj4gICAJdW5zaWduZWQgbG9uZyB0aW1lb3V0Owo+ Cj4gQEAgLTExNTgsNiArMTE1OCw5IEBAIHN0YXRpYyBpbnQgX19pOTE1X3NwaW5fcmVxdWVzdChz dHJ1Y3QgZHJtX2k5MTVfZ2VtX3JlcXVlc3QgKnJlcSkKPiAgIAkJaWYgKGk5MTVfZ2VtX3JlcXVl c3RfY29tcGxldGVkKHJlcSwgdHJ1ZSkpCj4gICAJCQlyZXR1cm4gMDsKPgo+ICsJCWlmIChzaWdu YWxfcGVuZGluZ19zdGF0ZShzdGF0ZSwgY3VycmVudCkpCj4gKwkJCWJyZWFrOwo+ICsKPiAgIAkJ aWYgKHRpbWVfYWZ0ZXJfZXEoamlmZmllcywgdGltZW91dCkpCj4gICAJCQlicmVhazsKPgo+IEBA IC0xMTk3LDYgKzEyMDAsNyBAQCBpbnQgX19pOTE1X3dhaXRfcmVxdWVzdChzdHJ1Y3QgZHJtX2k5 MTVfZ2VtX3JlcXVlc3QgKnJlcSwKPiAgIAlzdHJ1Y3QgZHJtX2k5MTVfcHJpdmF0ZSAqZGV2X3By aXYgPSBkZXYtPmRldl9wcml2YXRlOwo+ICAgCWNvbnN0IGJvb2wgaXJxX3Rlc3RfaW5fcHJvZ3Jl c3MgPQo+ICAgCQlBQ0NFU1NfT05DRShkZXZfcHJpdi0+Z3B1X2Vycm9yLnRlc3RfaXJxX3Jpbmdz KSAmIGludGVsX3JpbmdfZmxhZyhyaW5nKTsKPiArCWludCBzdGF0ZSA9IGludGVycnVwdGlibGUg PyBUQVNLX0lOVEVSUlVQVElCTEUgOiBUQVNLX1VOSU5URVJSVVBUSUJMRTsKPiAgIAlERUZJTkVf V0FJVCh3YWl0KTsKPiAgIAl1bnNpZ25lZCBsb25nIHRpbWVvdXRfZXhwaXJlOwo+ICAgCXM2NCBi ZWZvcmUsIG5vdzsKPiBAQCAtMTIyMSw3ICsxMjI1LDcgQEAgaW50IF9faTkxNV93YWl0X3JlcXVl c3Qoc3RydWN0IGRybV9pOTE1X2dlbV9yZXF1ZXN0ICpyZXEsCj4gICAJYmVmb3JlID0ga3RpbWVf Z2V0X3Jhd19ucygpOwo+Cj4gICAJLyogT3B0aW1pc3RpYyBzcGluIGZvciB0aGUgbmV4dCBqaWZm aWUgYmVmb3JlIHRvdWNoaW5nIElSUXMgKi8KPiAtCXJldCA9IF9faTkxNV9zcGluX3JlcXVlc3Qo cmVxKTsKPiArCXJldCA9IF9faTkxNV9zcGluX3JlcXVlc3QocmVxLCBzdGF0ZSk7Cj4gICAJaWYg KHJldCA9PSAwKQo+ICAgCQlnb3RvIG91dDsKPgo+IEBAIC0xMjMzLDggKzEyMzcsNyBAQCBpbnQg X19pOTE1X3dhaXRfcmVxdWVzdChzdHJ1Y3QgZHJtX2k5MTVfZ2VtX3JlcXVlc3QgKnJlcSwKPiAg IAlmb3IgKDs7KSB7Cj4gICAJCXN0cnVjdCB0aW1lcl9saXN0IHRpbWVyOwo+Cj4gLQkJcHJlcGFy ZV90b193YWl0KCZyaW5nLT5pcnFfcXVldWUsICZ3YWl0LAo+IC0JCQkJaW50ZXJydXB0aWJsZSA/ IFRBU0tfSU5URVJSVVBUSUJMRSA6IFRBU0tfVU5JTlRFUlJVUFRJQkxFKTsKPiArCQlwcmVwYXJl X3RvX3dhaXQoJnJpbmctPmlycV9xdWV1ZSwgJndhaXQsIHN0YXRlKTsKPgo+ICAgCQkvKiBXZSBu ZWVkIHRvIGNoZWNrIHdoZXRoZXIgYW55IGdwdSByZXNldCBoYXBwZW5lZCBpbiBiZXR3ZWVuCj4g ICAJCSAqIHRoZSBjYWxsZXIgZ3JhYmJpbmcgdGhlIHNlcW5vIGFuZCBub3cgLi4uICovCj4gQEAg LTEyNTIsNyArMTI1NSw3IEBAIGludCBfX2k5MTVfd2FpdF9yZXF1ZXN0KHN0cnVjdCBkcm1faTkx NV9nZW1fcmVxdWVzdCAqcmVxLAo+ICAgCQkJYnJlYWs7Cj4gICAJCX0KPgo+IC0JCWlmIChpbnRl cnJ1cHRpYmxlICYmIHNpZ25hbF9wZW5kaW5nKGN1cnJlbnQpKSB7Cj4gKwkJaWYgKHNpZ25hbF9w ZW5kaW5nX3N0YXRlKHN0YXRlLCBjdXJyZW50KSkgewo+ICAgCQkJcmV0ID0gLUVSRVNUQVJUU1lT Owo+ICAgCQkJYnJlYWs7Cj4gICAJCX0KPgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5m cmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3Rp bmZvL2ludGVsLWdmeAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752797AbbKPJy3 (ORCPT ); Mon, 16 Nov 2015 04:54:29 -0500 Received: from mga09.intel.com ([134.134.136.24]:11385 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751917AbbKPJyO (ORCPT ); Mon, 16 Nov 2015 04:54:14 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,302,1444719600"; d="scan'208";a="851881503" Subject: Re: [PATCH 1/2] drm/i915: Break busywaiting for requests on pending signals To: Chris Wilson , Jens Axboe , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <1447594364-4206-1-git-send-email-chris@chris-wilson.co.uk> Cc: dri-devel@lists.freedesktop.org, Daniel Vetter , Eero Tamminen , "Rantala, Valtteri" , stable@kernel.vger.org From: Tvrtko Ursulin Organization: Intel Corporation UK Plc Message-ID: <5649A7C2.90206@linux.intel.com> Date: Mon, 16 Nov 2015 09:54:10 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1447594364-4206-1-git-send-email-chris@chris-wilson.co.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 15/11/15 13:32, Chris Wilson wrote: > The busywait in __i915_spin_request() does not respect pending signals > and so may consume the entire timeslice for the task instead of > returning to userspace to handle the signal. Obviously correct to break the spin, but if spending a jiffie to react to signals was the only problem then it is not too severe. Add something to the commit message about how it was found/reported and about the severity of impact, etc? Otherwise, Reviewed-by: Tvrtko Ursulin > Fixes regression from > commit 2def4ad99befa25775dd2f714fdd4d92faec6e34 [v4.2] > Author: Chris Wilson > Date: Tue Apr 7 16:20:41 2015 +0100 > > drm/i915: Optimistically spin for the request completion > > Signed-off-by: Chris Wilson > Cc: Jens Axboe > Cc; "Rogozhkin, Dmitry V" > Cc: Daniel Vetter > Cc: Tvrtko Ursulin > Cc: Eero Tamminen > Cc: "Rantala, Valtteri" > Cc: stable@kernel.vger.org > --- > drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 5cf4a1998273..740530c571d1 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1146,7 +1146,7 @@ static bool missed_irq(struct drm_i915_private *dev_priv, > return test_bit(ring->id, &dev_priv->gpu_error.missed_irq_rings); > } > > -static int __i915_spin_request(struct drm_i915_gem_request *req) > +static int __i915_spin_request(struct drm_i915_gem_request *req, int state) > { > unsigned long timeout; > > @@ -1158,6 +1158,9 @@ static int __i915_spin_request(struct drm_i915_gem_request *req) > if (i915_gem_request_completed(req, true)) > return 0; > > + if (signal_pending_state(state, current)) > + break; > + > if (time_after_eq(jiffies, timeout)) > break; > > @@ -1197,6 +1200,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > struct drm_i915_private *dev_priv = dev->dev_private; > const bool irq_test_in_progress = > ACCESS_ONCE(dev_priv->gpu_error.test_irq_rings) & intel_ring_flag(ring); > + int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; > DEFINE_WAIT(wait); > unsigned long timeout_expire; > s64 before, now; > @@ -1221,7 +1225,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > before = ktime_get_raw_ns(); > > /* Optimistic spin for the next jiffie before touching IRQs */ > - ret = __i915_spin_request(req); > + ret = __i915_spin_request(req, state); > if (ret == 0) > goto out; > > @@ -1233,8 +1237,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > for (;;) { > struct timer_list timer; > > - prepare_to_wait(&ring->irq_queue, &wait, > - interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE); > + prepare_to_wait(&ring->irq_queue, &wait, state); > > /* We need to check whether any gpu reset happened in between > * the caller grabbing the seqno and now ... */ > @@ -1252,7 +1255,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > break; > } > > - if (interruptible && signal_pending(current)) { > + if (signal_pending_state(state, current)) { > ret = -ERESTARTSYS; > break; > } >