On 06/11/2025 13:14, Christian König wrote:
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.
Will do.
+ 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?
They become sleeping locks and IIRC there's a might_sleep equivalent in
there somewhere which fires when inside the preempt_disable section.
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;
}
