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

On 13/11/2025 14:51, 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. v3: Add more documentation to ->wait and ->release callback.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++------------ include/linux/dma-fence.h | 29 ++++++++++++++-- 2 files changed, 73 insertions(+), 25 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index b4f5c8635276..ec21be9b089a 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.
  

•  */
  

• 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();

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.

} 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)) {
  

• 	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);

@@ -1025,7 +1043,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;

@@ -1105,11 +1128,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"; }

@@ -1137,11 +1161,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..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.

• * 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.

Regards,

Tvrtko

dma_fence_signal(fence);
return true;

}

• rcu_read_unlock();

return false; }