Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Tvrtko Ursulin]

On 12/06/2026 11:42, Philipp Stanner wrote:
> dma_fence_is_signaled() returns whether a fence has been signaled
> already. That function contains a fast path opportunistic check which is
> not guarded by the lock and, according to Christian, cannot be guarded
> by the lock without causing a massive performance regression.
>
> This now means that dma_fence_is_signaled() can return true WHILE the
> fence callbacks are still being executed. This is razy and has lead to
> at least one bug solved in:
>
> commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
>
> Make this race impossible, by simply setting the bit only once the
> callbacks are actually completed.

My 2c is that I would be wary of trying to turn around the semantics of 
the callbacks. They are not "run these to make the fence signaled" but 
instead "let me know please when fence is signaled". I do not see a good 
justification for the change.

The bugs such as above quoted c8a5d5ea3ba6 are in fact more about 
nouveau itself pulling data underneath fences with callbacks and not a 
bug in dma-fence. With the opportunistic cleanup it is doing in 
nouveau_cli_work() perhaps it would want to use 
dma_fence_remove_callback() to appear somewhat sensible, although that 
too may be racy, I did not think about it. Anyway, I've sent a patch to 
make that one clearer and more documented, even faster for the 
unsignaled case.

Regards,

Tvrtko

> Signed-off-by: Philipp Stanner <phasta@xxxxxxxxxx>
> ---
>   drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index c7ea1e75d38a..2416cc86ce93 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -359,8 +359,19 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>   
>   	dma_fence_assert_held(fence);
>   
> -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> -				      &fence->flags)))
> +	/*
> +	 * First test the bit, so we don't signal an already signaled fence again.
> +	 * The lock protects against multiple parties setting the bit. The bit
> +	 * is then set at the end of the function.
> +	 *
> +	 * The background is that there is a fast path check in
> +	 * dma_fence_is_signaled() which does not use lock protection and can
> +	 * return true *while* the fence callbacks are still executing.
> +	 *
> +	 * This fast path check supposedly cannot be guarded by the lock because
> +	 * of significant performance regressions.
> +	 */
> +	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
>   		return;
>   
>   	trace_dma_fence_signaled(fence);
> @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>   		INIT_LIST_HEAD(&cur->node);
>   		cur->func(fence, cur);
>   	}
> +
> +	// TODO: we need some barrier here, don't we?
> +	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
>   }
>   EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
>