Re: [PATCH 2/3] drm/i915: Wake up the bottom-half if we steal their interrupt

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/07/16 08:45, Chris Wilson wrote:
> Following on from the scenario Tvrtko envision to explain a hard-to-hit
> race with multiple first waiters, we could also then race in the
> __i915_request_irq_complete() and the bottom-half may miss the vital
> irq-seqno barrier and so go to sleep not noticing their seqno is
> complete.
>
> v2: unlock, not double lock the rcu_read_lock.
>
> Fixes: 3d5564e91025 ("drm/i915: Only apply one barrier after...")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c269e0ad4057..11e9769411e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3998,7 +3998,10 @@ static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
>   	 * is woken.
>   	 */
>   	if (engine->irq_seqno_barrier &&
> +	    READ_ONCE(engine->breadcrumbs.tasklet) == current &&
>   	    cmpxchg_relaxed(&engine->irq_posted, 1, 0)) {
> +		struct task_struct *tsk;
> +
>   		/* The ordering of irq_posted versus applying the barrier
>   		 * is crucial. The clearing of the current irq_posted must
>   		 * be visible before we perform the barrier operation,
> @@ -4012,6 +4015,25 @@ static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
>   		 * seqno update.
>   		 */
>   		engine->irq_seqno_barrier(engine);
> +
> +		/* If we consume the irq, but we are no longer the bottom-half,
> +		 * the real bottom-half may not have serialised their own
> +		 * seqno check with the irq-barrier (i.e. may have inspected
> +		 * the seqno before we believe it coherent since they see
> +		 * irq_posted == false but we are still running).
> +		 */
> +		rcu_read_lock();
> +		tsk = READ_ONCE(engine->breadcrumbs.tasklet);
> +		if (tsk && tsk != current)
> +			/* Note that if the bottom-half is changed as we
> +			 * are sending the wake-up, the new bottom-half will
> +			 * be woken by whomever made the change. We only have
> +			 * to worry about when we steal the irq-posted for
> +			 * ourself.
> +			 */
> +			wake_up_process(tsk);
> +		rcu_read_unlock();
> +
>   		if (i915_gem_request_completed(req))
>   			return true;
>   	}
>

Looks like added safety which can't harm anything apart from causing 
some extra wakeups in cases where the code decides to wake up more than 
one waiter. But honestly it was so mind-bending to try to evaluate the 
impact of those so I capitulated there.

But as I said, don't see any issues with the code.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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