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 <[email protected]>
>> ---
>> 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;
>> }
>
