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

[PATCH] 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.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 28c34cd7f489..c6d85ae35ba6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3709,7 +3709,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
 	return vma;
 }
 
-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
@@ -3727,7 +3727,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))
 {
@@ -3745,18 +3745,19 @@ __busy_set_if_active(const struct i915_gem_active *active,
 		id = request->engine->exec_id;
 
 		/* Check that the pointer wasn't reassigned and overwritten. */
+		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);
-- 
2.8.1

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

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

[Daniel Vetter]

On Fri, Aug 05, 2016 at 09:11:14PM +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.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Oh now I get it, you pushed more patches meanwhile ;-)

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 28c34cd7f489..c6d85ae35ba6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3709,7 +3709,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>  	return vma;
>  }
>  
> -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
> @@ -3727,7 +3727,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))
>  {
> @@ -3745,18 +3745,19 @@ __busy_set_if_active(const struct i915_gem_active *active,
>  		id = request->engine->exec_id;
>  
>  		/* Check that the pointer wasn't reassigned and overwritten. */

Needs a comment, e.g.

		In __i915_gem_active_get_rcu we enforce ordering between
		the lockless access and the 2nd rcu pointer dereference
		through the barrier in fence_get_rcu/kref_get_unless_zero
		for the success case. Here there is no thing and we need
		an explicit read barrier. The corresponding write barrier
		is part of rcu_assign_pointer.


Hm reminds me, might be good to mention in active_get_rcu where the write
barrier is, too.

With the above (or similar) added:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		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);
> -- 
> 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

✗ Ro.CI.BAT: failure for drm/i915: Add smp_rmb() to busy ioctl's RCU dance

[Patchwork]

== Series Details ==

Series: drm/i915: Add smp_rmb() to busy ioctl's RCU dance
URL   : https://patchwork.freedesktop.org/series/10730/
State : failure

== Summary ==

Applying: drm/i915: Add smp_rmb() to busy ioctl's RCU dance
fatal: sha1 information is lacking or useless (drivers/gpu/drm/i915/i915_gem.c).
error: could not build fake ancestor
Patch failed at 0001 drm/i915: Add smp_rmb() to busy ioctl's RCU dance
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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