Re: [PATCH 4/4] dma-buf/fence-chain: Speed up processing of rearmed callbacks

["Christian König" <christian.koenig@amd.com>]

On 14.08.25 10:16, Janusz Krzysztofik wrote:
> When first user starts waiting on a not yet signaled fence of a chain
> link, a dma_fence_chain callback is added to a user fence of that link.
> When the user fence of that chain link is then signaled, the chain is
> traversed in search for a first not signaled link and the callback is
> rearmed on a user fence of that link.
> 
> Since chain fences may be exposed to user space, e.g. over drm_syncobj
> IOCTLs, users may start waiting on any link of the chain, then many links
> of a chain may have signaling enabled and their callbacks added to their
> user fences.  Once an arbitrary user fence is signaled, all
> dma_fence_chain callbacks added to it so far must be rearmed to another
> user fence of the chain.  In extreme scenarios, when all N links of a
> chain are awaited and then signaled in reverse order, the dma_fence_chain
> callback may be called up to N * (N + 1) / 2 times (an arithmetic series).
> 
> To avoid that potential excessive accumulation of dma_fence_chain
> callbacks, rearm a trimmed-down, signal only callback version to the base
> fence of a previous link, if not yet signaled, otherwise just signal the
> base fence of the current link instead of traversing the chain in search
> for a first not signaled link and moving all callbacks collected so far to
> a user fence of that link.

Well clear NAK to that! You can easily overflow the kernel stack with that!

Additional to this messing with the fence ops outside of the dma_fence code is an absolute no-go.

Regards,
Christian.

> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12904
> Suggested-by: Chris Wilson <chris.p.wilson@linux.intel.com>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> ---
>  drivers/dma-buf/dma-fence-chain.c | 101 +++++++++++++++++++++++++-----
>  1 file changed, 84 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index a8a90acf4f34d..90eff264ee05c 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -119,46 +119,113 @@ static const char *dma_fence_chain_get_timeline_name(struct dma_fence *fence)
>          return "unbound";
>  }
>  
> -static void dma_fence_chain_irq_work(struct irq_work *work)
> +static void signal_irq_work(struct irq_work *work)
>  {
>  	struct dma_fence_chain *chain;
>  
>  	chain = container_of(work, typeof(*chain), work);
>  
> -	/* Try to rearm the callback */
> -	if (!dma_fence_chain_enable_signaling(&chain->base))
> -		/* Ok, we are done. No more unsignaled fences left */
> -		dma_fence_signal(&chain->base);
> +	dma_fence_signal(&chain->base);
>  	dma_fence_put(&chain->base);
>  }
>  
> -static void dma_fence_chain_cb(struct dma_fence *f, struct dma_fence_cb *cb)
> +static void signal_cb(struct dma_fence *f, struct dma_fence_cb *cb)
> +{
> +	struct dma_fence_chain *chain;
> +
> +	chain = container_of(cb, typeof(*chain), cb);
> +	init_irq_work(&chain->work, signal_irq_work);
> +	irq_work_queue(&chain->work);
> +}
> +
> +static void rearm_irq_work(struct irq_work *work)
> +{
> +	struct dma_fence_chain *chain;
> +	struct dma_fence *prev;
> +
> +	chain = container_of(work, typeof(*chain), work);
> +
> +	rcu_read_lock();
> +	prev = rcu_dereference(chain->prev);
> +	if (prev && dma_fence_add_callback(prev, &chain->cb, signal_cb))
> +		prev = NULL;
> +	rcu_read_unlock();
> +	if (prev)
> +		return;
> +
> +	/* Ok, we are done. No more unsignaled fences left */
> +	signal_irq_work(work);
> +}
> +
> +static inline bool fence_is_signaled__nested(struct dma_fence *fence)
> +{
> +	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +		return true;
> +
> +	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave_nested(fence->lock, flags, SINGLE_DEPTH_NESTING);
> +		dma_fence_signal_locked(fence);
> +		spin_unlock_irqrestore(fence->lock, flags);
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool prev_is_signaled(struct dma_fence_chain *chain)
> +{
> +	struct dma_fence *prev;
> +	bool result;
> +
> +	rcu_read_lock();
> +	prev = rcu_dereference(chain->prev);
> +	result = !prev || fence_is_signaled__nested(prev);
> +	rcu_read_unlock();
> +
> +	return result;
> +}
> +
> +static void rearm_or_signal_cb(struct dma_fence *f, struct dma_fence_cb *cb)
>  {
>  	struct dma_fence_chain *chain;
>  
>  	chain = container_of(cb, typeof(*chain), cb);
> -	init_irq_work(&chain->work, dma_fence_chain_irq_work);
> +	if (prev_is_signaled(chain)) {
> +		/* Ok, we are done. No more unsignaled fences left */
> +		init_irq_work(&chain->work, signal_irq_work);
> +	} else {
> +		/* Try to rearm the callback */
> +		init_irq_work(&chain->work, rearm_irq_work);
> +	}
> +
>  	irq_work_queue(&chain->work);
> -	dma_fence_put(f);
>  }
>  
>  static bool dma_fence_chain_enable_signaling(struct dma_fence *fence)
>  {
>  	struct dma_fence_chain *head = to_dma_fence_chain(fence);
> +	int err = -ENOENT;
>  
> -	dma_fence_get(&head->base);
> -	dma_fence_chain_for_each(fence, &head->base) {
> -		struct dma_fence *f = dma_fence_chain_contained(fence);
> +	if (WARN_ON(!head))
> +		return false;
>  
> -		dma_fence_get(f);
> -		if (!dma_fence_add_callback(f, &head->cb, dma_fence_chain_cb)) {
> +	dma_fence_get(fence);
> +	if (head->fence)
> +		err = dma_fence_add_callback(head->fence, &head->cb, rearm_or_signal_cb);
> +	if (err) {
> +		if (prev_is_signaled(head)) {
>  			dma_fence_put(fence);
> -			return true;
> +		} else {
> +			init_irq_work(&head->work, rearm_irq_work);
> +			irq_work_queue(&head->work);
> +			err = 0;
>  		}
> -		dma_fence_put(f);
>  	}
> -	dma_fence_put(&head->base);
> -	return false;
> +
> +	return !err;
>  }
>  
>  static bool dma_fence_chain_signaled(struct dma_fence *fence)