Re: [PATCH 03/20] dma-buf: protected fence ops by RCU v2

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

On 10/31/25 15:29, Tvrtko Ursulin wrote:
> On 31/10/2025 13:16, Christian König wrote:
>> At first glance it is counter intuitive to protect a constant function
>> pointer table by RCU, but this allows modules providing the function
>> table to unload by waiting for an RCU grace period.
>>
>> v2: make one the now duplicated lockdep warnings a comment instead.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++------------
>>   include/linux/dma-fence.h   | 18 ++++++++--
>>   2 files changed, 62 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index b229d96f551c..ed82e8361096 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -498,6 +498,7 @@ EXPORT_SYMBOL(dma_fence_signal);
>>   signed long
>>   dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>>   {
>> +    const struct dma_fence_ops *ops;
>>       signed long ret;
>>         if (WARN_ON(timeout < 0))
>> @@ -509,15 +510,21 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>>         dma_fence_enable_sw_signaling(fence);
>>   -    if (trace_dma_fence_wait_start_enabled()) {
>> -        rcu_read_lock();
>> -        trace_dma_fence_wait_start(fence);
>> +    rcu_read_lock();
>> +    ops = rcu_dereference(fence->ops);
>> +    trace_dma_fence_wait_start(fence);
>> +    if (ops->wait) {
>> +        /*
>> +         * Implementing the wait ops is deprecated and not supported for
>> +         * issuer independent fences, so it is ok to use the ops outside
>> +         * the RCU protected section.
>> +         */
> 
> Probably a good idea to put this explanation about issue independent fences to struct dma_fence_ops kerneldoc. At the moment only .wait is documented as deprecated, so both it and .release can be expanded with this additional angle.

Done, but I'm not sure if my documentation is sufficient. You should probably take a look at the next version.

> 
>> +        rcu_read_unlock();
>> +        ret = ops->wait(fence, intr, timeout);
>> +    } else {
>>           rcu_read_unlock();
>> -    }
>> -    if (fence->ops->wait)
>> -        ret = fence->ops->wait(fence, intr, timeout);
>> -    else
>>           ret = dma_fence_default_wait(fence, intr, timeout);
>> +    }
>>       if (trace_dma_fence_wait_end_enabled()) {
>>           rcu_read_lock();
>>           trace_dma_fence_wait_end(fence);
>> @@ -538,6 +545,7 @@ void dma_fence_release(struct kref *kref)
>>   {
>>       struct dma_fence *fence =
>>           container_of(kref, struct dma_fence, refcount);
>> +    const struct dma_fence_ops *ops;
>>         rcu_read_lock();
>>       trace_dma_fence_destroy(fence);
>> @@ -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();
>>   }
>>   EXPORT_SYMBOL(dma_fence_release);
>>   @@ -593,6 +601,7 @@ EXPORT_SYMBOL(dma_fence_free);
>>     static bool __dma_fence_enable_signaling(struct dma_fence *fence)
>>   {
>> +    const struct dma_fence_ops *ops;
>>       bool was_set;
>>         lockdep_assert_held(fence->lock);
>> @@ -603,14 +612,18 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
>>       if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>           return false;
>>   -    if (!was_set && fence->ops->enable_signaling) {
>> +    rcu_read_lock();
>> +    ops = rcu_dereference(fence->ops);
>> +    if (!was_set && ops->enable_signaling) {
>>           trace_dma_fence_enable_signal(fence);
>>   -        if (!fence->ops->enable_signaling(fence)) {
>> +        if (!ops->enable_signaling(fence)) {
> 
> Have you tried the series with PREEMPT_RT enabled?

No, that is not something we usually test with.

> I am worried about spin locks in any fence ops callbacks which now run with preemption disabled.

Hui? Why would spin_locks be problematic here?

Regards,
Christian.

> 
> Regards,
> 
> Tvrtko
> 
>> +            rcu_read_unlock();
>>               dma_fence_signal_locked(fence);
>>               return false;
>>           }
>>       }
>> +    rcu_read_unlock();
>>         return true;
>>   }
>> @@ -983,8 +996,13 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
>>    */
>>   void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
>>   {
>> -    if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
>> -        fence->ops->set_deadline(fence, deadline);
>> +    const struct dma_fence_ops *ops;
>> +
>> +    rcu_read_lock();
>> +    ops = rcu_dereference(fence->ops);
>> +    if (ops->set_deadline && !dma_fence_is_signaled(fence))
>> +        ops->set_deadline(fence, deadline);
>> +    rcu_read_unlock();
>>   }
>>   EXPORT_SYMBOL(dma_fence_set_deadline);
>>   @@ -1024,7 +1042,12 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>>       BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
>>         kref_init(&fence->refcount);
>> -    fence->ops = ops;
>> +    /*
>> +     * At first glance it is counter intuitive to protect a constant
>> +     * function pointer table by RCU, but this allows modules providing the
>> +     * function table to unload by waiting for an RCU grace period.
>> +     */
>> +    RCU_INIT_POINTER(fence->ops, ops);
>>       INIT_LIST_HEAD(&fence->cb_list);
>>       fence->lock = lock;
>>       fence->context = context;
>> @@ -1104,11 +1127,12 @@ EXPORT_SYMBOL(dma_fence_init64);
>>    */
>>   const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
>>   {
>> -    RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>> -             "RCU protection is required for safe access to returned string");
>> +    const struct dma_fence_ops *ops;
>>   +    /* RCU protection is required for safe access to returned string */
>> +    ops = rcu_dereference(fence->ops);
>>       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> -        return fence->ops->get_driver_name(fence);
>> +        return ops->get_driver_name(fence);
>>       else
>>           return "detached-driver";
>>   }
>> @@ -1136,11 +1160,12 @@ EXPORT_SYMBOL(dma_fence_driver_name);
>>    */
>>   const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
>>   {
>> -    RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
>> -             "RCU protection is required for safe access to returned string");
>> +    const struct dma_fence_ops *ops;
>>   +    /* RCU protection is required for safe access to returned string */
>> +    ops = rcu_dereference(fence->ops);
>>       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> -        return fence->ops->get_timeline_name(fence);
>> +        return ops->get_timeline_name(fence);
>>       else
>>           return "signaled-timeline";
>>   }
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 64639e104110..38421a0c7c5b 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
>> @@ -418,13 +418,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 +454,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();
>>           dma_fence_signal(fence);
>>           return true;
>>       }
>> +    rcu_read_unlock();
>>         return false;
>>   }
>