Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit edf6b76f authored by Chris Wilson's avatar Chris Wilson
Browse files

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



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: default avatarChris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Link: http://patchwork.freedesktop.org/patch/msgid/1470731014-6894-1-git-send-email-chris@chris-wilson.co.uk
parent 87b723a1

drivers/gpu/drm/i915/i915_gem.c

+31 −5
Original line number Diff line number Diff line
@@ -3733,7 +3733,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
@@ -3751,7 +3751,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))

{
@@ -3768,19 +3768,45 @@ __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.

		 *

		 * It is worth comparing this sequence with

		 * raw_write_seqcount_latch() which operates very similarly.

		 * The challenge here is the visibility of the other CPU

		 * writes to the reallocated request vs the local CPU ordering.

		 * Before the other CPU can overwrite the request, it will

		 * have updated our active->request and gone through a wmb.

		 * During the read here, we want to make sure that the values

		 * we see have not been overwritten as we do so - and we do

		 * that by serialising the second pointer check with the writes

		 * on other other CPUs.

		 *

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

drivers/gpu/drm/i915/i915_gem_request.h

+3 −0
Original line number Diff line number Diff line
@@ -490,6 +490,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);