[Intel-gfx] [PATCH 11/20] drm/i915: Protect i915_request_await_start from early waits

[Tvrtko Ursulin]

On 27/02/2020 08:57, Chris Wilson wrote:
> We need to be extremely careful inside i915_request_await_start() as it
> needs to walk the list of requests in the foreign timeline with very
> little protection. As we hold our own timeline mutex, we can not nest
> inside the signaler's timeline mutex, so all that remains is our RCU
> protection. However, to be safe we need to tell the compiler that we may
> be traversing the list only under RCU protection, and furthermore we
> need to start declaring requests as elements of the timeline from their
> construction.
> 
> Fixes: 9ddc8ec027a3 ("drm/i915: Eliminate the trylock for awaiting an earlier request")
> Fixes: 6a79d848403d ("drm/i915: Lock signaler timeline while navigating")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 41 ++++++++++++++++++++---------
>   1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index d53af93b919b..e5a55801f753 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -290,7 +290,7 @@ bool i915_request_retire(struct i915_request *rq)
>   	spin_unlock_irq(&rq->lock);
>   
>   	remove_from_client(rq);
> -	list_del(&rq->link);
> +	list_del_rcu(&rq->link);
>   
>   	intel_context_exit(rq->context);
>   	intel_context_unpin(rq->context);
> @@ -736,6 +736,8 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
>   	rq->infix = rq->ring->emit; /* end of header; start of user payload */
>   
>   	intel_context_mark_active(ce);
> +	list_add_tail_rcu(&rq->link, &tl->requests);
> +
>   	return rq;
>   
>   err_unwind:
> @@ -792,13 +794,23 @@ i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
>   	GEM_BUG_ON(i915_request_timeline(rq) ==
>   		   rcu_access_pointer(signal->timeline));
>   
> +	if (i915_request_started(signal))
> +		return 0;
> +
>   	fence = NULL;
>   	rcu_read_lock();
>   	spin_lock_irq(&signal->lock);
> -	if (!i915_request_started(signal) &&
> -	    !list_is_first(&signal->link,
> -			   &rcu_dereference(signal->timeline)->requests)) {
> -		struct i915_request *prev = list_prev_entry(signal, link);
> +	do {
> +		struct list_head *pos = READ_ONCE(signal->link.prev);
> +		struct i915_request *prev;
> +
> +		/* Confirm signal has not been retired, the link is valid */
> +		if (unlikely(i915_request_started(signal)))
> +			break;
> +
> +		/* Is signal the earliest request on its timeline? */
> +		if (pos == &rcu_dereference(signal->timeline)->requests)
> +			break;
>   
>   		/*
>   		 * Peek at the request before us in the timeline. That
> @@ -806,13 +818,18 @@ i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
>   		 * after acquiring a reference to it, confirm that it is
>   		 * still part of the signaler's timeline.
>   		 */
> -		if (i915_request_get_rcu(prev)) {
> -			if (list_next_entry(prev, link) == signal)
> -				fence = &prev->fence;
> -			else
> -				i915_request_put(prev);
> +		prev = list_entry(pos, typeof(*prev), link);
> +		if (!i915_request_get_rcu(prev))
> +			break;
> +
> +		/* After the strong barrier, confirm prev is still attached */
> +		if (unlikely(READ_ONCE(prev->link.next) != &signal->link)) {
> +			i915_request_put(prev);
> +			break;
>   		}
> -	}
> +
> +		fence = &prev->fence;
> +	} while (0);
>   	spin_unlock_irq(&signal->lock);
>   	rcu_read_unlock();
>   	if (!fence)
> @@ -1253,8 +1270,6 @@ __i915_request_add_to_timeline(struct i915_request *rq)
>   							 0);
>   	}
>   
> -	list_add_tail(&rq->link, &timeline->requests);
> -
>   	/*
>   	 * Make sure that no request gazumped us - if it was allocated after
>   	 * our i915_request_alloc() and called __i915_request_add() before
> 

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

Regards,

Tvrtko