Re: [PATCH v2] drm/i915/breadcrumbs: Drop request reference for the signaler thread

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

On 03/02/2018 10:19, Chris Wilson wrote:
> If we remember to cancel the signaler on a request when retiring it
> (after we know that the request has been signaled), we do not need to
> carry an additional request in the signaler itself. This prevents an
> issue whereby the signaler threads may be delayed and hold on to
> thousands of request references, causing severe memory fragmentation and
> premature oom (most noticeable on 32b snb due to the limited GFP_KERNEL
> and frequent use of inter-engine fences).
> 
> v2: Rename first_signal(), document reads outside of locks.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_request.c  |   6 +-
>   drivers/gpu/drm/i915/intel_breadcrumbs.c | 150 +++++++++++++++++--------------
>   2 files changed, 86 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 5ff57211ee06..dc7cc2c55bc7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -555,7 +555,10 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
>   	spin_lock_irq(&request->lock);
>   	if (request->waitboost)
>   		atomic_dec(&request->i915->gt_pm.rps.num_waiters);
> -	dma_fence_signal_locked(&request->fence);
> +	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &request->fence.flags))
> +		dma_fence_signal_locked(&request->fence);
> +	if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
> +		intel_engine_cancel_signaling(request);
>   	spin_unlock_irq(&request->lock);
>   
>   	i915_priotree_fini(request->i915, &request->priotree);
> @@ -864,6 +867,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>   
>   	/* No zalloc, must clear what we need by hand */
>   	req->global_seqno = 0;
> +	req->signaling.wait.seqno = 0;
>   	req->file_priv = NULL;
>   	req->batch = NULL;
>   	req->capture_list = NULL;
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 62b2a20bc24e..efbc627a2a25 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -224,7 +224,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>   	struct intel_wait *wait, *n;
>   
>   	if (!b->irq_armed)
> -		goto wakeup_signaler;
> +		return;
>   
>   	/*
>   	 * We only disarm the irq when we are idle (all requests completed),
> @@ -249,14 +249,6 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>   	b->waiters = RB_ROOT;
>   
>   	spin_unlock_irq(&b->rb_lock);
> -
> -	/*
> -	 * The signaling thread may be asleep holding a reference to a request,
> -	 * that had its signaling cancelled prior to being preempted. We need
> -	 * to kick the signaler, just in case, to release any such reference.
> -	 */
> -wakeup_signaler:
> -	wake_up_process(b->signaler);
>   }
>   
>   static bool use_fake_irq(const struct intel_breadcrumbs *b)
> @@ -633,6 +625,63 @@ static void signaler_set_rtpriority(void)
>   	 sched_setscheduler_nocheck(current, SCHED_FIFO, &param);
>   }
>   
> +static void __intel_engine_remove_signal(struct intel_engine_cs *engine,
> +					 struct drm_i915_gem_request *request)
> +{
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +
> +	lockdep_assert_held(&b->rb_lock);
> +
> +	/*
> +	 * Wake up all other completed waiters and select the
> +	 * next bottom-half for the next user interrupt.
> +	 */
> +	__intel_engine_remove_wait(engine, &request->signaling.wait);
> +
> +	/*
> +	 * Find the next oldest signal. Note that as we have
> +	 * not been holding the lock, another client may
> +	 * have installed an even older signal than the one
> +	 * we just completed - so double check we are still
> +	 * the oldest before picking the next one.
> +	 */
> +	if (request->signaling.wait.seqno) {
> +		if (request == rcu_access_pointer(b->first_signal)) {
> +			struct rb_node *rb = rb_next(&request->signaling.node);
> +			rcu_assign_pointer(b->first_signal,
> +					   rb ? to_signaler(rb) : NULL);
> +		}
> +
> +		rb_erase(&request->signaling.node, &b->signals);
> +		request->signaling.wait.seqno = 0;
> +	}
> +}
> +
> +static struct drm_i915_gem_request *
> +get_first_signal_rcu(struct intel_breadcrumbs *b)
> +{
> +	/*
> +	 * See the big warnings for i915_gem_active_get_rcu() and similarly
> +	 * for dma_fence_get_rcu_safe() that explain the intricacies involved
> +	 * here with defeating CPU/compiler speculation and enforcing
> +	 * the required memory barriers.
> +	 */
> +	do {
> +		struct drm_i915_gem_request *request;
> +
> +		request = rcu_dereference(b->first_signal);
> +		if (request)
> +			request = i915_gem_request_get_rcu(request);
> +
> +		barrier();
> +
> +		if (!request || request == rcu_access_pointer(b->first_signal))
> +			return rcu_pointer_handoff(request);
> +
> +		i915_gem_request_put(request);
> +	} while (1);
> +}
> +
>   static int intel_breadcrumbs_signaler(void *arg)
>   {
>   	struct intel_engine_cs *engine = arg;
> @@ -656,41 +705,21 @@ static int intel_breadcrumbs_signaler(void *arg)
>   		 * a new client.
>   		 */
>   		rcu_read_lock();
> -		request = rcu_dereference(b->first_signal);
> -		if (request)
> -			request = i915_gem_request_get_rcu(request);
> +		request = get_first_signal_rcu(b);
>   		rcu_read_unlock();
>   		if (signal_complete(request)) {
> -			local_bh_disable();
> -			dma_fence_signal(&request->fence);
> -			local_bh_enable(); /* kick start the tasklets */
> -
> -			spin_lock_irq(&b->rb_lock);
> -
> -			/* Wake up all other completed waiters and select the
> -			 * next bottom-half for the next user interrupt.
> -			 */
> -			__intel_engine_remove_wait(engine,
> -						   &request->signaling.wait);
> -
> -			/* Find the next oldest signal. Note that as we have
> -			 * not been holding the lock, another client may
> -			 * have installed an even older signal than the one
> -			 * we just completed - so double check we are still
> -			 * the oldest before picking the next one.
> -			 */
> -			if (request == rcu_access_pointer(b->first_signal)) {
> -				struct rb_node *rb =
> -					rb_next(&request->signaling.node);
> -				rcu_assign_pointer(b->first_signal,
> -						   rb ? to_signaler(rb) : NULL);
> +			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> +				      &request->fence.flags)) {
> +				local_bh_disable();
> +				dma_fence_signal(&request->fence);
> +				local_bh_enable(); /* kick start the tasklets */
>   			}
> -			rb_erase(&request->signaling.node, &b->signals);
> -			RB_CLEAR_NODE(&request->signaling.node);
> -
> -			spin_unlock_irq(&b->rb_lock);
>   
> -			i915_gem_request_put(request);
> +			if (READ_ONCE(request->signaling.wait.seqno)) {
> +				spin_lock_irq(&b->rb_lock);
> +				__intel_engine_remove_signal(engine, request);
> +				spin_unlock_irq(&b->rb_lock);
> +			}

How would you view taking the request->lock over this block in the 
signaler and then being able to call simply 
intel_engine_cancel_signaling - ending up with very similar code as in 
i915_gem_request_retire?

Only difference would be the tasklet kicking, but maybe still it would 
be possible to consolidate the two in one helper.

__i915_gem_request_retire_signaling(req, bool kick_tasklets)
{
	if (!DMA_FENCE_FLAG_SIGNALED_BIT) {
		dma_fence_signal_locked(...);
		if (kick_tasklets) {
			local_bh_disable();
			local_bh_enable();
		}
	}

	if (DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT)
		intel_engine_cancel_signaling(...);
}

i915_gem_request_retire_signaling(req)
{
	spin_lock_irq(req->lock);
	__i915_gem_request_retire_signaling(req, true);
	spin_unlock_irq(req->lock);
}

And call the former from request retire, and later from signaler?

Regards,

Tvrtko

>   
>   			/* If the engine is saturated we may be continually
>   			 * processing completed requests. This angers the
> @@ -701,19 +730,17 @@ static int intel_breadcrumbs_signaler(void *arg)
>   			 */
>   			do_schedule = need_resched();
>   		}
> +		i915_gem_request_put(request);
>   
>   		if (unlikely(do_schedule)) {
>   			if (kthread_should_park())
>   				kthread_parkme();
>   
> -			if (unlikely(kthread_should_stop())) {
> -				i915_gem_request_put(request);
> +			if (unlikely(kthread_should_stop()))
>   				break;
> -			}
>   
>   			schedule();
>   		}
> -		i915_gem_request_put(request);
>   	} while (1);
>   	__set_current_state(TASK_RUNNING);
>   
> @@ -742,12 +769,12 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request,
>   	if (!seqno)
>   		return;
>   
> +	spin_lock(&b->rb_lock);
> +
> +	GEM_BUG_ON(request->signaling.wait.seqno);
>   	request->signaling.wait.tsk = b->signaler;
>   	request->signaling.wait.request = request;
>   	request->signaling.wait.seqno = seqno;
> -	i915_gem_request_get(request);
> -
> -	spin_lock(&b->rb_lock);
>   
>   	/* First add ourselves into the list of waiters, but register our
>   	 * bottom-half as the signaller thread. As per usual, only the oldest
> @@ -786,7 +813,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request,
>   			rcu_assign_pointer(b->first_signal, request);
>   	} else {
>   		__intel_engine_remove_wait(engine, &request->signaling.wait);
> -		i915_gem_request_put(request);
> +		request->signaling.wait.seqno = 0;
>   		wakeup = false;
>   	}
>   
> @@ -798,32 +825,17 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request,
>   
>   void intel_engine_cancel_signaling(struct drm_i915_gem_request *request)
>   {
> -	struct intel_engine_cs *engine = request->engine;
> -	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -
>   	GEM_BUG_ON(!irqs_disabled());
>   	lockdep_assert_held(&request->lock);
> -	GEM_BUG_ON(!request->signaling.wait.seqno);
>   
> -	spin_lock(&b->rb_lock);
> +	if (READ_ONCE(request->signaling.wait.seqno)) {
> +		struct intel_engine_cs *engine = request->engine;
> +		struct intel_breadcrumbs *b = &engine->breadcrumbs;
>   
> -	if (!RB_EMPTY_NODE(&request->signaling.node)) {
> -		if (request == rcu_access_pointer(b->first_signal)) {
> -			struct rb_node *rb =
> -				rb_next(&request->signaling.node);
> -			rcu_assign_pointer(b->first_signal,
> -					   rb ? to_signaler(rb) : NULL);
> -		}
> -		rb_erase(&request->signaling.node, &b->signals);
> -		RB_CLEAR_NODE(&request->signaling.node);
> -		i915_gem_request_put(request);
> +		spin_lock(&b->rb_lock);
> +		__intel_engine_remove_signal(engine, request);
> +		spin_unlock(&b->rb_lock);
>   	}
> -
> -	__intel_engine_remove_wait(engine, &request->signaling.wait);
> -
> -	spin_unlock(&b->rb_lock);
> -
> -	request->signaling.wait.seqno = 0;
>   }
>   
>   int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx