Re: [PATCH v2 2/8] dma-buf/dma-fence: Add dma_fence_check_and_signal()

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

On 12/1/25 14:55, Philipp Stanner wrote:
> On Mon, 2025-12-01 at 14:23 +0100, Christian König wrote:
>> On 12/1/25 11:50, Philipp Stanner wrote:
>>> The overwhelming majority of users of dma_fence signaling functions
>>> don't care about whether the fence had already been signaled by someone
>>> else. Therefore, the return code shall be removed from those functions.
>>>
>>> For the few users who rely on the check, a new, specialized function
>>> shall be provided.
>>>
>>> Add dma_fence_check_and_signal(), which signals a fence if it had not
>>> yet been signaled, and informs the user about that.
>>>
>>> Add a counter part, dma_fence_check_and_signal_locked(), which doesn't
>>> take the spinlock.
>>>
>>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>>> ---
>>>  drivers/dma-buf/dma-fence.c | 44 +++++++++++++++++++++++++++++++++++++
>>>  include/linux/dma-fence.h   |  2 ++
>>>  2 files changed, 46 insertions(+)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>> index 96d72ffc0750..146de62887cf 100644
>>> --- a/drivers/dma-buf/dma-fence.c
>>> +++ b/drivers/dma-buf/dma-fence.c
>>> @@ -445,6 +445,50 @@ int dma_fence_signal_locked(struct dma_fence *fence)
>>>  }
>>>  EXPORT_SYMBOL(dma_fence_signal_locked);
>>>  
>>> +/**
>>> + * dma_fence_check_and_signal_locked - signal the fence if it's not yet signaled
>>> + * @fence: the fence to check and signal
>>> + *
>>> + * Checks whether a fence was signaled and signals it if it was not yet signaled.
>>> + *
>>> + * Unlike dma_fence_check_and_signal(), this function must be called with
>>> + * &struct dma_fence.lock being held.
>>> + *
>>> + * Return: true if fence has been signaled already, false otherwise.
>>> + */
>>> +bool dma_fence_check_and_signal_locked(struct dma_fence *fence)
>>
>> I'm seriously considering to nuke all the unlocked variants of dma_fence functions and just make it mandatory for callers to grab the lock manually.
>>
> 
> You mean "nuke the *locked* variants.

Sorry, that wasn't specific enough.

What I meant was making the locked variants the default instead of the unlocked ones.

> 
> Why, though? Aren't they enough for most users?
> I suppose you have all those subtle races in mind..

Yeah, exactly that.

> 
>>> +{
>>> +	bool ret;
>>> +
>>> +	ret = dma_fence_test_signaled_flag(fence);
>>> +	dma_fence_signal_locked(fence);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL(dma_fence_check_and_signal_locked);
>>> +
>>> +/**
>>> + * dma_fence_check_and_signal - signal the fence if it's not yet signaled
>>> + * @fence: the fence to check and signal
>>> + *
>>> + * Checks whether a fence was signaled and signals it if it was not yet signaled.
>>> + * All this is done in a race-free manner.
>>> + *
>>> + * Return: true if fence has been signaled already, false otherwise.
>>> + */
>>> +bool dma_fence_check_and_signal(struct dma_fence *fence)
>>
>> So I think we should name this one here dma_fence_check_and_signal_unlocked() and drop the postfix from the locked variant.
> 
> postfix?
> 
> Well, now, IDK. Can't we, for this series, keep the _locked() variant
> so that it's congruent with all the other dma_fence code?

Good point. That thought was not really related to this series here.

> 
> And then later if you want to force manual locking you can add that
> kernel-wide in a separate series, since it'll be a discussion-worthy,
> bigger chunk of work.
> 
> That's cleaner, and my series here won't prevent that once merged.
> 
>>
>>> +{
>>> +	unsigned long flags;
>>> +	bool ret;
>>> +
>>> +	spin_lock_irqsave(fence->lock, flags);
>>> +	ret = dma_fence_check_and_signal_locked(fence);
>>> +	spin_unlock_irqrestore(fence->lock, flags);
>>
>> Could this use guard(fence->lock, flags) ?
> 
> guard? You mean a lockdep guard? Do you have a pointer to someplace in
> dma_fence who does what you mean / want?

E.g. like guard(spinlock_irqsave)(&fence->lock);

Regards,
Christian.

> 
> 
> P.
> 
>>
>> Regards,
>> Christian.
>>
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL(dma_fence_check_and_signal);
>>> +
>>>  /**
>>>   * dma_fence_signal - signal completion of a fence
>>>   * @fence: the fence to signal
>>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>>> index 19972f5d176f..0504afe52c2a 100644
>>> --- a/include/linux/dma-fence.h
>>> +++ b/include/linux/dma-fence.h
>>> @@ -365,6 +365,8 @@ static inline void __dma_fence_might_wait(void) {}
>>>  #endif
>>>  
>>>  int dma_fence_signal(struct dma_fence *fence);
>>> +bool dma_fence_check_and_signal(struct dma_fence *fence);
>>> +bool dma_fence_check_and_signal_locked(struct dma_fence *fence);
>>>  int dma_fence_signal_locked(struct dma_fence *fence);
>>>  int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
>>>  int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>
>