On 18/11/2025 14:28, Christian König wrote:
On 11/14/25 11:50, Tvrtko Ursulin wrote:
@@ -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.
I don't think that this is a problem. When PREEMPT_RT is enabled both RCU and
spinlocks become preemptible.
So as far as I know it is perfectly valid to grab a spinlock under an rcu read
side critical section.
Looking at the source just now, I think it is possible I mixed it up
with preempt_disable()+spin_lock().
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.
Both fixed, thanks.
+ * 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.
Well it's good to talk about it but I think that it is not necessary to protect
the lock in this particular case.
See the RCU protection is only for the fence->ops pointer, but the lock can be
taken way after the fence is already signaled.
That's why I came up with the patch to move the lock into the fence in the
first place.
Right. And you think there is nothing to gain with the option of keeping
the rcu_read_unlock() to after signalling? Ie. why not plug a potential
race if we can for no negative effect.
Regards,
Tvrtko
Regards,
Tvrtko
dma_fence_signal(fence);
return true;
}
+ rcu_read_unlock();
return false;
}
