[PATCH 1/2] drm/i915: Add smp_rmb() to busy ioctl's RCU dance

[PATCH 1/2] drm/i915: Add smp_rmb() to busy ioctl's RCU dance

[Chris Wilson]

In the debate as to whether the second read of active->request is
ordered after the dependent reads of the first read of active->request,
just give in and throw a smp_rmb() in there so that ordering of loads is
assured.

v2: Explain the manual smp_rmb()

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c         | 25 ++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_gem_request.h |  3 +++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f4f8eaa90f2a..654f0b015f97 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3735,7 +3735,7 @@ i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
 	i915_vma_unpin(i915_gem_obj_to_ggtt_view(obj, view));
 }
 
-static __always_inline unsigned __busy_read_flag(unsigned int id)
+static __always_inline unsigned int __busy_read_flag(unsigned int id)
 {
 	/* Note that we could alias engines in the execbuf API, but
 	 * that would be very unwise as it prevents userspace from
@@ -3753,7 +3753,7 @@ static __always_inline unsigned int __busy_write_id(unsigned int id)
 	return id;
 }
 
-static __always_inline unsigned
+static __always_inline unsigned int
 __busy_set_if_active(const struct i915_gem_active *active,
 		     unsigned int (*flag)(unsigned int id))
 {
@@ -3770,19 +3770,34 @@ __busy_set_if_active(const struct i915_gem_active *active,
 
 		id = request->engine->exec_id;
 
-		/* Check that the pointer wasn't reassigned and overwritten. */
+		/* Check that the pointer wasn't reassigned and overwritten.
+		 *
+		 * In __i915_gem_active_get_rcu(), we enforce ordering between
+		 * the first rcu pointer dereference (imposing a
+		 * read-dependency only on access through the pointer) and
+		 * the second lockless access through the memory barrier
+		 * following a successful atomic_inc_not_zero(). Here there
+		 * is no such barrier, and so we must manually insert an
+		 * explicit read barrier to ensure that the following
+		 * access occurs after all the loads through the first
+		 * pointer.
+		 *
+		 * The corresponding write barrier is part of
+		 * rcu_assign_pointer().
+		 */
+		smp_rmb();
 		if (request == rcu_access_pointer(active->request))
 			return flag(id);
 	} while (1);
 }
 
-static inline unsigned
+static __always_inline unsigned int
 busy_check_reader(const struct i915_gem_active *active)
 {
 	return __busy_set_if_active(active, __busy_read_flag);
 }
 
-static inline unsigned
+static __always_inline unsigned int
 busy_check_writer(const struct i915_gem_active *active)
 {
 	return __busy_set_if_active(active, __busy_write_id);
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 3496e28785e7..b2456dede3ad 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -497,6 +497,9 @@ __i915_gem_active_get_rcu(const struct i915_gem_active *active)
 		 * incremented) then the following read for rcu_access_pointer()
 		 * must occur after the atomic operation and so confirm
 		 * that this request is the one currently being tracked.
+		 *
+		 * The corresponding write barrier is part of
+		 * rcu_assign_pointer().
 		 */
 		if (!request || request == rcu_access_pointer(active->request))
 			return rcu_pointer_handoff(request);
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Do not overwrite the request with zero on reallocation

[Chris Wilson]

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.)
v3: request->batch also needs to be cleared

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 | 37 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_request.h | 11 ++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 6a1661643d3d..b7ffde002a62 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,13 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
 	req->engine = engine;
 	req->ctx = i915_gem_context_get(ctx);
 
+	/* No zalloc, must clear what we need by hand */
+	req->signaling.wait.tsk = NULL;
+	req->previous_context = NULL;
+	req->file_priv = NULL;
+	req->batch_obj = 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 b2456dede3ad..721eb8cbce9b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -51,6 +51,13 @@ struct intel_signal_node {
  * emission time to be associated with the request for tracking how far ahead
  * of the GPU the submission is.
  *
+ * When modifying this structure be very aware that we perform a lockless
+ * RCU lookup of it that may race against reallocation of the struct
+ * from the slab freelist. We intentionally do not zero the structure on
+ * allocation so that the lookup can use the dangling pointers (and is
+ * cogniscent that those pointers may be wrong). Instead, everything that
+ * needs to be initialised must be done so explicitly.
+ *
  * The requests are reference counted.
  */
 struct drm_i915_gem_request {
@@ -465,6 +472,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

✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Add smp_rmb() to busy ioctl's RCU dance

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add smp_rmb() to busy ioctl's RCU dance
URL   : https://patchwork.freedesktop.org/series/10733/
State : failure

== Summary ==

Series 10733v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/10733/revisions/1/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)
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (ro-bdw-i5-5250u)

fi-kbl-qkkr      total:244  pass:185  dwarn:29  dfail:0   fail:3   skip:27 
ro-bdw-i5-5250u  total:240  pass:220  dwarn:4   dfail:0   fail:0   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-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
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:172  dwarn:1   dfail:0   fail:2   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-bsw-n3050 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1743/

b834992 drm-intel-nightly: 2016y-08m-05d-20h-40m-44s UTC integration manifest
85ab9b3 drm/i915: Do not overwrite the request with zero on reallocation
467ed218 drm/i915: Add smp_rmb() to busy ioctl's RCU dance

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add smp_rmb() to busy ioctl's RCU dance

[Chris Wilson]

On Fri, Aug 05, 2016 at 10:13:22PM +0100, Chris Wilson wrote:
> In the debate as to whether the second read of active->request is
> ordered after the dependent reads of the first read of active->request,
> just give in and throw a smp_rmb() in there so that ordering of loads is
> assured.
> 
> v2: Explain the manual smp_rmb()
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 25 ++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_gem_request.h |  3 +++
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f4f8eaa90f2a..654f0b015f97 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3735,7 +3735,7 @@ i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
>  	i915_vma_unpin(i915_gem_obj_to_ggtt_view(obj, view));
>  }
>  
> -static __always_inline unsigned __busy_read_flag(unsigned int id)
> +static __always_inline unsigned int __busy_read_flag(unsigned int id)
>  {
>  	/* Note that we could alias engines in the execbuf API, but
>  	 * that would be very unwise as it prevents userspace from
> @@ -3753,7 +3753,7 @@ static __always_inline unsigned int __busy_write_id(unsigned int id)
>  	return id;
>  }
>  
> -static __always_inline unsigned
> +static __always_inline unsigned int
>  __busy_set_if_active(const struct i915_gem_active *active,
>  		     unsigned int (*flag)(unsigned int id))
>  {
> @@ -3770,19 +3770,34 @@ __busy_set_if_active(const struct i915_gem_active *active,
>  
>  		id = request->engine->exec_id;
>  
> -		/* Check that the pointer wasn't reassigned and overwritten. */
> +		/* Check that the pointer wasn't reassigned and overwritten.
> +		 *
> +		 * In __i915_gem_active_get_rcu(), we enforce ordering between
> +		 * the first rcu pointer dereference (imposing a
> +		 * read-dependency only on access through the pointer) and
> +		 * the second lockless access through the memory barrier
> +		 * following a successful atomic_inc_not_zero(). Here there
> +		 * is no such barrier, and so we must manually insert an
> +		 * explicit read barrier to ensure that the following
> +		 * access occurs after all the loads through the first
> +		 * pointer.
> +		 *
> +		 * The corresponding write barrier is part of
> +		 * rcu_assign_pointer().
> +		 */
> +		smp_rmb();

Are you sure this should not just be a read_barrier_depends()?

active->request is data dependent on the earlier reads through it, and
here we are only caring that those loads are completed before we double
check the request hasn't been overwritten.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add smp_rmb() to busy ioctl's RCU dance

[Daniel Vetter]

On Sat, Aug 06, 2016 at 11:26:22AM +0100, Chris Wilson wrote:
> On Fri, Aug 05, 2016 at 10:13:22PM +0100, Chris Wilson wrote:
> > In the debate as to whether the second read of active->request is
> > ordered after the dependent reads of the first read of active->request,
> > just give in and throw a smp_rmb() in there so that ordering of loads is
> > assured.
> > 
> > v2: Explain the manual smp_rmb()
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         | 25 ++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/i915_gem_request.h |  3 +++
> >  2 files changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index f4f8eaa90f2a..654f0b015f97 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3735,7 +3735,7 @@ i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> >  	i915_vma_unpin(i915_gem_obj_to_ggtt_view(obj, view));
> >  }
> >  
> > -static __always_inline unsigned __busy_read_flag(unsigned int id)
> > +static __always_inline unsigned int __busy_read_flag(unsigned int id)
> >  {
> >  	/* Note that we could alias engines in the execbuf API, but
> >  	 * that would be very unwise as it prevents userspace from
> > @@ -3753,7 +3753,7 @@ static __always_inline unsigned int __busy_write_id(unsigned int id)
> >  	return id;
> >  }
> >  
> > -static __always_inline unsigned
> > +static __always_inline unsigned int
> >  __busy_set_if_active(const struct i915_gem_active *active,
> >  		     unsigned int (*flag)(unsigned int id))
> >  {
> > @@ -3770,19 +3770,34 @@ __busy_set_if_active(const struct i915_gem_active *active,
> >  
> >  		id = request->engine->exec_id;
> >  
> > -		/* Check that the pointer wasn't reassigned and overwritten. */
> > +		/* Check that the pointer wasn't reassigned and overwritten.
> > +		 *
> > +		 * In __i915_gem_active_get_rcu(), we enforce ordering between
> > +		 * the first rcu pointer dereference (imposing a
> > +		 * read-dependency only on access through the pointer) and
> > +		 * the second lockless access through the memory barrier
> > +		 * following a successful atomic_inc_not_zero(). Here there
> > +		 * is no such barrier, and so we must manually insert an
> > +		 * explicit read barrier to ensure that the following
> > +		 * access occurs after all the loads through the first
> > +		 * pointer.
> > +		 *
> > +		 * The corresponding write barrier is part of
> > +		 * rcu_assign_pointer().
> > +		 */
> > +		smp_rmb();
> 
> Are you sure this should not just be a read_barrier_depends()?
> 
> active->request is data dependent on the earlier reads through it, and
> here we are only caring that those loads are completed before we double
> check the request hasn't been overwritten.

There's no data depency between loading request->engine->exec_id and
(re)loading active->request. I think full smp_rmb it needs to be.
-Daniel
-- 
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