Re: [PATCH 02/18] dma-buf: protected fence ops by RCU v3

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

On 11/14/25 11:50, Tvrtko Ursulin wrote:
>> @@ -569,12 +577,12 @@ void dma_fence_release(struct kref *kref)
>>           spin_unlock_irqrestore(fence->lock, flags);
>>       }
>>   -    rcu_read_unlock();
>> -
>> -    if (fence->ops->release)
>> -        fence->ops->release(fence);
>> +    ops = rcu_dereference(fence->ops);
>> +    if (ops->release)
>> +        ops->release(fence);
>>       else
>>           dma_fence_free(fence);
>> +    rcu_read_unlock();
> 
> Risk being a spin lock in the release callback will trigger a warning on PREEMPT_RT. But at least the current code base does not have anything like that AFAICS so I guess it is okay.

I don't think that this is a problem. When PREEMPT_RT is enabled both RCU and spinlocks become preemptible.

So as far as I know it is perfectly valid to grab a spinlock under an rcu read side critical section.

>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 64639e104110..77f07735f556 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -66,7 +66,7 @@ struct seq_file;
>>    */
>>   struct dma_fence {
>>       spinlock_t *lock;
>> -    const struct dma_fence_ops *ops;
>> +    const struct dma_fence_ops __rcu *ops;
>>       /*
>>        * We clear the callback list on kref_put so that by the time we
>>        * release the fence it is unused. No one should be adding to the
>> @@ -218,6 +218,10 @@ struct dma_fence_ops {
>>        * timed out. Can also return other error values on custom implementations,
>>        * which should be treated as if the fence is signaled. For example a hardware
>>        * lockup could be reported like that.
>> +     *
>> +     * Implementing this callback prevents the BO from detaching after
> 
> s/BO/fence/
> 
>> +     * signaling and so it is mandatory for the module providing the
>> +     * dma_fence_ops to stay loaded as long as the dma_fence exists.
>>        */
>>       signed long (*wait)(struct dma_fence *fence,
>>                   bool intr, signed long timeout);
>> @@ -229,6 +233,13 @@ struct dma_fence_ops {
>>        * Can be called from irq context.  This callback is optional. If it is
>>        * NULL, then dma_fence_free() is instead called as the default
>>        * implementation.
>> +     *
>> +     * Implementing this callback prevents the BO from detaching after
> 
> Ditto.

Both fixed, thanks.

> 
>> +     * signaling and so it is mandatory for the module providing the
>> +     * dma_fence_ops to stay loaded as long as the dma_fence exists.
>> +     *
>> +     * If the callback is implemented the memory backing the dma_fence
>> +     * object must be freed RCU safe.
>>        */
>>       void (*release)(struct dma_fence *fence);
>>   @@ -418,13 +429,19 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>>   static inline bool
>>   dma_fence_is_signaled_locked(struct dma_fence *fence)
>>   {
>> +    const struct dma_fence_ops *ops;
>> +
>>       if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>           return true;
>>   -    if (fence->ops->signaled && fence->ops->signaled(fence)) {
>> +    rcu_read_lock();
>> +    ops = rcu_dereference(fence->ops);
>> +    if (ops->signaled && ops->signaled(fence)) {
>> +        rcu_read_unlock();
>>           dma_fence_signal_locked(fence);
>>           return true;
>>       }
>> +    rcu_read_unlock();
>>         return false;
>>   }
>> @@ -448,13 +465,19 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>>   static inline bool
>>   dma_fence_is_signaled(struct dma_fence *fence)
>>   {
>> +    const struct dma_fence_ops *ops;
>> +
>>       if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>           return true;
>>   -    if (fence->ops->signaled && fence->ops->signaled(fence)) {
>> +    rcu_read_lock();
>> +    ops = rcu_dereference(fence->ops);
>> +    if (ops->signaled && ops->signaled(fence)) {
>> +        rcu_read_unlock();
> 
> With the unlocked version two threads could race and one could make the fence->lock go away just around here, before the dma_fence_signal below will take it. It seems it is only safe to rcu_read_unlock before signaling if using the embedded fence (later in the series). Can you think of a downside to holding the rcu read lock to after signaling? that would make it safe I think.

Well it's good to talk about it but I think that it is not necessary to protect the lock in this particular case.

See the RCU protection is only for the fence->ops pointer, but the lock can be taken way after the fence is already signaled.

That's why I came up with the patch to move the lock into the fence in the first place.

Regards,
Christian.

> 
> Regards,
> 
> Tvrtko
> 
>>           dma_fence_signal(fence);
>>           return true;
>>       }
>> +    rcu_read_unlock();
>>         return false;
>>   }
>