Re: [PATCH] drm/i915/gt: Use signalers_lock to prevent starvation of irq_work.

Hi Maarten,

...

> @@ -208,13 +209,13 @@ static void signal_irq_work(struct irq_work *work)
>  	if (!signal && READ_ONCE(b->irq_armed) && list_empty(&b->signalers))
>  		intel_breadcrumbs_disarm_irq(b);
>  
> -	rcu_read_lock();
> -	atomic_inc(&b->signaler_active);
> -	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
> +	spin_lock(&b->signalers_lock);
> +	list_for_each_entry_safe(ce, next, &b->signalers, signal_link) {

without converting rcu lists to normal lists, can't we simply use
spinlocks around this for_each to avoid PREEMPT_RT starvation?

>  		struct i915_request *rq;
> +		bool release;
>  
> -		list_for_each_entry_rcu(rq, &ce->signals, signal_link) {
> -			bool release;
> +		spin_lock(&ce->signal_lock);
> +		while ((rq = list_first_entry_or_null(&ce->signals, typeof(*rq), signal_link))) {
>  
>  			if (!__i915_request_is_complete(rq))
>  				break;

...

> @@ -427,30 +424,87 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>  		return true;
>  
>  	spin_lock(&ce->signal_lock);
> +	b = READ_ONCE(rq->engine)->breadcrumbs;
> +
>  	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
> -		insert_breadcrumb(rq);
> +		add_context = insert_breadcrumb(rq, b);
> +
> +	if (add_context && spin_trylock(&b->signalers_lock)) {
> +		add_signaling_context(b, ce);
> +		spin_unlock(&b->signalers_lock);
> +		add_context = false;
> +	}
>  	spin_unlock(&ce->signal_lock);
>  
> +	if (add_context) {
> +		/*
> +		 * Fast trylock didn't work, use slow locking.
> +		 *
> +		 * Dropping the lock to solve the inversion is safe, since
> +		 * no race is possible against remove_signaling_context()
> +		 * without being added as signaling context.
> +		 */
> +		spin_lock(&b->signalers_lock);
> +		spin_lock(&ce->signal_lock);
> +		add_signaling_context(b, ce);
> +		spin_unlock(&ce->signal_lock);
> +		spin_unlock(&b->signalers_lock);

If I want to be a bit paranoic, a new request can be added in
between the two locking blocks and "add_context" is not valid
anymore.

add_signaling_context() already checks for ce->signals, but it
does not check whether ce->signal_link is already linked. Should
we also check !list_empty(&ce->signal_link)?

Thanks,
Andi

> +	}
> +
> +	/*
> +	 * Defer enabling the interrupt to after HW submission and recheck
> +	 * the request as it may have completed and raised the interrupt as
> +	 * we were attaching it into the lists.
> +	 */
> +	if (!READ_ONCE(b->irq_armed) || __i915_request_is_complete(rq))
> +		irq_work_queue(&b->irq_work);
> +
>  	return true;
>  }