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

[Tvrtko Ursulin <tursulin@ursulin.net>]

On 18/11/2025 14:28, Christian König wrote:
> 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.

Looking at the source just now, I think it is possible I mixed it up 
with preempt_disable()+spin_lock().
>>> 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.

Right. And you think there is nothing to gain with the option of keeping 
the rcu_read_unlock() to after signalling? Ie. why not plug a potential 
race if we can for no negative effect.

Regards,

Tvrtko

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