On 13/10/2025 14:48, 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.
Signed-off-by: Christian König <[email protected]>
---
drivers/dma-buf/dma-fence.c | 65 +++++++++++++++++++++++++++----------
include/linux/dma-fence.h | 18 ++++++++--
2 files changed, 62 insertions(+), 21 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 51ee13d005bc..982f2b2a62c0 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();
I had it like this back in May you were worried release callback can
sleep. So I gather since then you figured out no one sleeps or takes a
sleeping lock?
I went through them all and it seems that could be (almost) so.
There is only vgem_fence_release() which calls timer_delete_sync(), and
while __timer_delete_sync() has a comment saying
del_timer_wait_running() has a sleeping slow path I think this is only
due spinlocks becoming sleeping locks.
Due this PREEMPT_RT might be a problem for the RCU approach in general,
not just for vgem.
Possibly if you enable it you would start seeing warnings fire for
sleeping while preemption disabled. Something to double check in case I
got confused.
Hm actually, do you even need to move the RCU section around the
.release() and .wait() if the premise of the series is drivers who
specify those will not be protected?
Regards,
Tvrtko
}
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);
@@ -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,14 @@ EXPORT_SYMBOL(dma_fence_init64);
*/
const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
{
+ const struct dma_fence_ops *ops;
+
RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
"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 +1162,14 @@ EXPORT_SYMBOL(dma_fence_driver_name);
*/
const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
{
+ const struct dma_fence_ops *ops;
+
RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
"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 "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;
}
