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