* ✗ Ro.CI.BAT: failure for drm/i915: Do not overwrite the request with zero on reallocation
2016-08-05 15:13 [PATCH] drm/i915: Do not overwrite the request with zero on reallocation Chris Wilson
@ 2016-08-05 15:42 ` Patchwork
2016-08-05 16:17 ` [PATCH v2] " Chris Wilson
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2016-08-05 15:42 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Do not overwrite the request with zero on reallocation
URL : https://patchwork.freedesktop.org/series/10719/
State : failure
== Summary ==
Series 10719v1 drm/i915: Do not overwrite the request with zero on reallocation
http://patchwork.freedesktop.org/api/1.0/series/10719/revisions/1/mbox
Test core_auth:
Subgroup basic-auth:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Test core_prop_blob:
Subgroup basic:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Test drv_getparams_basic:
Subgroup basic-eu-total:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Subgroup basic-subslice-total:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Test drv_hangman:
Subgroup error-state-basic:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Test drv_module_reload_basic:
dmesg-warn -> PASS (ro-hsw-i7-4770r)
skip -> DMESG-WARN (ro-skl3-i5-6260u)
Test gem_basic:
Subgroup bad-close:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Subgroup create-close:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Subgroup create-fd-close:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Test gem_busy:
Subgroup basic-blt:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-bsd:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-bsd1:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-bsd2:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-parallel-blt:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-parallel-bsd:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-parallel-bsd1:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-parallel-bsd2:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-parallel-render:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-parallel-vebox:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-render:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Subgroup basic-vebox:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Test gem_close_race:
Subgroup basic-process:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Subgroup basic-threads:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Test gem_cpu_reloc:
Subgroup basic:
pass -> DMESG-FAIL (ro-skl3-i5-6260u)
pass -> DMESG-FAIL (ro-bdw-i5-5250u)
Test gem_cs_tlb:
Subgroup basic-default:
pass -> SKIP (ro-skl3-i5-6260u)
pass -> SKIP (ro-bdw-i5-5250u)
Test gem_ctx_basic:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Test gem_ctx_create:
Subgroup basic:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
pass -> DMESG-WARN (ro-bdw-i5-5250u)
Subgroup basic-files:
pass -> DMESG-FAIL (ro-skl3-i5-6260u)
pass -> DMESG-FAIL (ro-bdw-i5-5250u)
Test gem_ctx_exec:
Subgroup basic:
pass -> INCOMPLETE (ro-skl3-i5-6260u)
WARNING: Long output truncated
fi-kbl-qkkr failed to connect after reboot
fi-skl-i5-6260u failed to connect after reboot
fi-skl-i7-6700k failed to connect after reboot
Results at /archive/results/CI_IGT_test/RO_Patchwork_1730/
7b03586 drm-intel-nightly: 2016y-08m-05d-10h-17m-33s UTC integration manifest
3b2b18f drm/i915: Do not overwrite the request with zero on reallocation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2] drm/i915: Do not overwrite the request with zero on reallocation
2016-08-05 15:13 [PATCH] drm/i915: Do not overwrite the request with zero on reallocation Chris Wilson
2016-08-05 15:42 ` ✗ Ro.CI.BAT: failure for " Patchwork
@ 2016-08-05 16:17 ` Chris Wilson
2016-08-05 16:17 ` [PATCH] " Daniel Vetter
2016-08-06 5:56 ` ✗ Ro.CI.BAT: failure for drm/i915: Do not overwrite the request with zero on reallocation (rev3) Patchwork
3 siblings, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2016-08-05 16:17 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Goel, Akash
When using RCU lookup for the request, commit 0eafec6d3244 ("drm/i915:
Enable lockless lookup of request tracking via RCU"), we acknowledge that
we may race with another thread that could have reallocated the request.
In order for the first thread not to blow up, the second thread must not
clear the request completed before overwriting it. In the RCU lookup, we
allow for the engine/seqno to be replaced but we do not allow for it to
be zeroed.
The choice we make is to either add extra checking to the RCU lookup, or
embrace the inherent races (as intended). It is more complicated as we
need to manually clear everything we depend upon being zero initialised,
but we benefit from not emiting the memset() to clear the entire
frequently allocated structure (that memset turns up in throughput
profiles). And at the same time, the lookup remains flexible for future
adjustments.
v2: Old style LRC requires another variable to be initialize. (The
danger inherent in not zeroing everything.)
Fixes: 0eafec6d3244 ("drm/i915: Enable lockless lookup of request...")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem_request.c | 35 ++++++++++++++++++++++++++++++++-
drivers/gpu/drm/i915/i915_gem_request.h | 4 ++++
2 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index b317a672040f..968e7246fcde 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -355,7 +355,35 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
if (req && i915_gem_request_completed(req))
i915_gem_request_retire(req);
- req = kmem_cache_zalloc(dev_priv->requests, GFP_KERNEL);
+ /* Beware: Dragons be flying overhead.
+ *
+ * We use RCU to look up requests in flight. The lookups may
+ * race with the request being allocated from the slab freelist.
+ * That is the request we are writing to here, may be in the process
+ * of being read by __i915_gem_active_get_request_rcu(). As such,
+ * we have to be very careful when overwriting the contents. During
+ * the RCU lookup, we change chase the request->engine pointer,
+ * read the request->fence.seqno and increment the reference count.
+ *
+ * The reference count is incremented atomically. If it is zero,
+ * the lookup knows the request is unallocated and complete. Otherwise,
+ * it is either still in use, or has been reallocated and reset
+ * with fence_init(). This increment is safe for release as we check
+ * that the request we have a reference to and matches the active
+ * request.
+ *
+ * Before we increment the refcount, we chase the request->engine
+ * pointer. We must not call kmem_cache_zalloc() or else we set
+ * that pointer to NULL and cause a crash during the lookup. If
+ * we see the request is completed (based on the value of the
+ * old engine and seqno), the lookup is complete and reports NULL.
+ * If we decide the request is not completed (new engine or seqno),
+ * then we grab a reference and double check that it is still the
+ * active request - which it won't be and restart the lookup.
+ *
+ * Do not use kmem_cache_zalloc() here!
+ */
+ req = kmem_cache_alloc(dev_priv->requests, GFP_KERNEL);
if (!req)
return ERR_PTR(-ENOMEM);
@@ -375,6 +403,11 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
req->engine = engine;
req->ctx = i915_gem_context_get(ctx);
+ req->signaling.wait.tsk = NULL;
+ req->previous_context = NULL;
+ req->file_priv = NULL;
+ req->elsp_submitted = 0;
+
/*
* Reserve space in the ring buffer for all the commands required to
* eventually emit this request. This is to guarantee that the
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 3496e28785e7..1508eae4a258 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -465,6 +465,10 @@ __i915_gem_active_get_rcu(const struct i915_gem_active *active)
* just report the active tracker is idle. If the new request is
* incomplete, then we acquire a reference on it and check that
* it remained the active request.
+ *
+ * It is then imperative that we do not zero the request on
+ * reallocation, so that we can chase the dangling pointers!
+ * See i915_gem_request_alloc().
*/
do {
struct drm_i915_gem_request *request;
--
2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Do not overwrite the request with zero on reallocation
2016-08-05 15:13 [PATCH] drm/i915: Do not overwrite the request with zero on reallocation Chris Wilson
2016-08-05 15:42 ` ✗ Ro.CI.BAT: failure for " Patchwork
2016-08-05 16:17 ` [PATCH v2] " Chris Wilson
@ 2016-08-05 16:17 ` Daniel Vetter
2016-08-06 5:56 ` ✗ Ro.CI.BAT: failure for drm/i915: Do not overwrite the request with zero on reallocation (rev3) Patchwork
3 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2016-08-05 16:17 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx, Goel, Akash
On Fri, Aug 05, 2016 at 04:13:28PM +0100, Chris Wilson wrote:
> When using RCU lookup for the request, commit 0eafec6d3244 ("drm/i915:
> Enable lockless lookup of request tracking via RCU"), we acknowledge that
> we may race with another thread that could have reallocated the request.
> In order for the first thread not to blow up, the second thread must not
> clear the request completed before overwriting it. In the RCU lookup, we
> allow for the engine/seqno to be replaced but we do not allow for it to
> be zeroed.
First few remarks:
- Commit message definitely needs to explain the tradeoff between avoiding
the memset and just making req->engine lookup a bit safer for _rcu like
below:
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 6002adc43523..e55492ba20ec 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -244,6 +244,26 @@ i915_gem_request_started(const struct drm_i915_gem_request *req)
}
static inline bool
+i915_gem_request_completed_rcu(const struct drm_i915_gem_request *req)
+{
+ struct intel_engine_cs *engine = READ_ONCE(req->engine);
+
+ /* When we peek at a request solely under rcu protection, without
+ * hodling a full reference, the request might be in the process of
+ * getting freed and reallocated. Make sure we don't stumble over a NULL
+ * engine in that case.
+ *
+ * If we are hitting this race it means that the old request has been
+ * released, which only happens once it has completed.
+ */
+ if (!engine)
+ return true;
+
+ return i915_seqno_passed(intel_engine_get_seqno(engine),
+ req->fence.seqno);
+}
+
+static inline bool
i915_gem_request_completed(const struct drm_i915_gem_request *req)
{
return i915_seqno_passed(intel_engine_get_seqno(req->engine),
@@ -384,7 +404,7 @@ i915_gem_active_peek_rcu(const struct i915_gem_active *active)
struct drm_i915_gem_request *request;
request = rcu_dereference(active->request);
- if (!request || i915_gem_request_completed(request))
+ if (!request || i915_gem_request_completed_rcu(request))
return NULL;
return request;
@@ -459,7 +479,7 @@ __i915_gem_active_get_rcu(const struct i915_gem_active *active)
struct drm_i915_gem_request *request;
request = rcu_dereference(active->request);
- if (!request || i915_gem_request_completed(request))
+ if (!request || i915_gem_request_completed_rcu(request))
return NULL;
request = i915_gem_request_get_rcu(request);
I'd go as far as putting this as an alternative fix into the changelog.
- We need a big hoonking comment somewhere (probably right above the
kmem_cache_alloc) why this is not zalloc. Proposal:
/* Reallocation can race with rcu-protected request lookup. The
* request look code does eventually acquire a full reference, but
* before that it has a fast-path to peek at the request
* completion. We must make sure that that code can't fall over
* a request in the process of getting reinitialized here. Since
* it's a pure optimization data integrity is not important, the
* only risk is in chasing NULL pointers. Currently this is only
* request->engine which must not be cleared.
*
* Alternative fix would be to make the request peeking more
* robust, but that's overhead. Also, requests get reallocated a
* lot, avoid the memset makes sense. Hence this is not allocated
* with kzalloc, which is a rare exception in the i915 driver.
*
* BEWARE: Everything must be correctly initialized or set to
* NULL!
*/
>
> Fixes: 0eafec6d3244 ("drm/i915: Enable lockless lookup of request...")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_request.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index b317a672040f..7529b6b5deda 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -355,7 +355,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> if (req && i915_gem_request_completed(req))
> i915_gem_request_retire(req);
>
> - req = kmem_cache_zalloc(dev_priv->requests, GFP_KERNEL);
> + req = kmem_cache_alloc(dev_priv->requests, GFP_KERNEL);
> if (!req)
> return ERR_PTR(-ENOMEM);
>
> @@ -375,6 +375,10 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> req->engine = engine;
> req->ctx = i915_gem_context_get(ctx);
>
> + req->signaling.wait.tsk = NULL;
Do we need to reinit this? The important bit is that we remove ourselves
from the rb tree, and we do that in intel_engine_remove_wait.
> + req->previous_context = NULL;
Should we move that into the retire function where we call the lrc unpin?
> + req->file_priv = NULL;
We already clear this in remove_from_client.
Admittedly didn't do a full audit whether those are all we need yet.
-Daniel
> +
> /*
> * Reserve space in the ring buffer for all the commands required to
> * eventually emit this request. This is to guarantee that the
> --
> 2.8.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* ✗ Ro.CI.BAT: failure for drm/i915: Do not overwrite the request with zero on reallocation (rev3)
2016-08-05 15:13 [PATCH] drm/i915: Do not overwrite the request with zero on reallocation Chris Wilson
` (2 preceding siblings ...)
2016-08-05 16:17 ` [PATCH] " Daniel Vetter
@ 2016-08-06 5:56 ` Patchwork
3 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2016-08-06 5:56 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Do not overwrite the request with zero on reallocation (rev3)
URL : https://patchwork.freedesktop.org/series/10719/
State : failure
== Summary ==
Series 10719v3 drm/i915: Do not overwrite the request with zero on reallocation
http://patchwork.freedesktop.org/api/1.0/series/10719/revisions/3/mbox
Test kms_cursor_legacy:
Subgroup basic-cursor-vs-flip-varying-size:
pass -> FAIL (ro-ilk1-i5-650)
Subgroup basic-flip-vs-cursor-legacy:
pass -> FAIL (ro-skl3-i5-6260u)
fail -> PASS (ro-bdw-i5-5250u)
fi-kbl-qkkr total:244 pass:186 dwarn:29 dfail:0 fail:3 skip:26
ro-bdw-i5-5250u total:240 pass:219 dwarn:4 dfail:0 fail:1 skip:16
ro-bdw-i7-5557U total:240 pass:224 dwarn:0 dfail:0 fail:0 skip:16
ro-bdw-i7-5600u total:240 pass:207 dwarn:0 dfail:0 fail:1 skip:32
ro-bsw-n3050 total:240 pass:194 dwarn:0 dfail:0 fail:4 skip:42
ro-hsw-i3-4010u total:240 pass:214 dwarn:0 dfail:0 fail:0 skip:26
ro-hsw-i7-4770r total:240 pass:214 dwarn:0 dfail:0 fail:0 skip:26
ro-ilk-i7-620lm total:240 pass:173 dwarn:1 dfail:0 fail:1 skip:65
ro-ilk1-i5-650 total:235 pass:173 dwarn:0 dfail:0 fail:2 skip:60
ro-ivb-i7-3770 total:240 pass:205 dwarn:0 dfail:0 fail:0 skip:35
ro-ivb2-i7-3770 total:240 pass:209 dwarn:0 dfail:0 fail:0 skip:31
ro-skl3-i5-6260u total:240 pass:222 dwarn:0 dfail:0 fail:4 skip:14
ro-snb-i7-2620M total:240 pass:198 dwarn:0 dfail:0 fail:1 skip:41
ro-byt-n2820 failed to connect after reboot
Results at /archive/results/CI_IGT_test/RO_Patchwork_1733/
b834992 drm-intel-nightly: 2016y-08m-05d-20h-40m-44s UTC integration manifest
55318e2 drm/i915: Do not overwrite the request with zero on reallocation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread