On Wed, 2026-07-15 at 15:57 -0300, Daniel Almeida wrote:
Hi Phillipp,
Hello, thx for the review
[…]
new file mode 100644 index 000000000000..cbe8f447a603 --- /dev/null +++ b/rust/kernel/dma_buf/dma_fence.rs @@ -0,0 +1,894 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Copyright (T) 2025, 2026 Red Hat Inc.: +// - Philipp Stanner pstanner@redhat.com
SPDX-copyrightText?
ACK.
+//! DriverFence support.
This contains, Fence, DriverFence and FenceContext, along with other miscellaneous. I would spend a bit more time fleshing this out slightly.
OK.
+//! +//! Reference: https://docs.kernel.org/driver-api/dma-buf.html#c.dma_fence +//! +//! T header: [`include/linux/dma-fence.h`](srctree/include/linux/dma-fence.h)
T?
Relic. Will fix.
[…]
+ try_pin_init!(Self { + // SAFETY: `dma_fence_context_alloc()` merely works on a global atomic. + // Parameter `1` is the number of contexts we want to allocate. + nr: unsafe { bindings::dma_fence_context_alloc(1) }, + seqno: AtomicU64::new(0),
Do we really need to force a 0 here? i.e.: can’t we take the initial seqno as an argument?
We could. What would that be useful for?
+ driver_name, + timeline_name, + data <- data, + }) + }
[…]
+ /// Create a new fence, consuming `data`. + /// + /// The fence will increment the refcount of the fence context associated with this + /// [`FenceCtx`]. + pub fn new_fence(&self, memory: DriverFenceAllocation<'a, T>) -> DriverFence<'a, T> {
Do we ever check if the allocation was really made by “self” ? Apparently not :/
Hm, no, we don't.
For the most part that's irrelevant, since all critical components then only get set in new_fence(). Correct typization is enforced through T.
The notable exception is the fence_ctx reference itself.
What should we do about it?
We could keep the fctx field as a MaybeUninit and set it later. Or we check through the fctx identifier number whether it's the correct one in new_fence(), but then new_fence() could fail with some error, and it's probably better to have it be completely fail-free.
[…]
+pub trait FenceCb: Send + 'static {
IMHO these acronyms make the code harder to read for no gain. I don’t think we have to carry that over from C.
FenceCb -> FenceCallback
No principle objections from me. But do you believe *all* of them are bad? I really like `let fctx = …` for instance.
+ /// Called when the fence is signaled. + /// + /// This is called from the fence signaling path, which may be in interrupt + /// context or with locks held, which is why `self` is only borrowed, so that + /// it cannot drop. Implementations must not sleep or perform + /// long-running operations. + /// + /// An implementation likely wants to inform itself (e.g., through a work item) + /// within this callback that the associated [`FenceCbRegistration`] can now be + /// dropped. + fn called(&mut self);
I think “signaled” is more descriptive than “called”.
Here I disagree. A callback does not get signaled. It gets called once the fence gets signaled.
+}
[…]
+/// The receiving counterpart of a [`DriverFence`], designed to register callbacks +/// on, check the signalled state etc. A [`Fence`] cannot be signalled. +/// A [`Fence`] is always refcounted.
I would explain this a tad better.
What exactly? The refcounting? The dualism between DriverFence and Fence? :)
+#[repr(transparent)] +pub struct Fence { + /// The actual dma_fence passed to C. + inner: Opaquebindings::dma_fence, +}
+// SAFETY: Fences are literally designed to be shared between threads. +unsafe impl Send for Fence {} +// SAFETY: Fences are literally designed to be shared between threads. +unsafe impl Sync for Fence {}
+impl Fence { + /// Check whether the fence was signalled at the moment of the function call. + /// + /// Note that this can return `true` for a [`Fence`] whose [`DriverFence`] + /// has not yet been dropped. The reason is that the fence ops callbacks can + /// cause the fence to get signaled by the C backend. + pub fn is_signaled(&self) -> bool { + let fence = self.as_raw(); + let mut fence_flags: usize = 0; + let flag_ptr = &raw mut fence_flags;
+ // We shouuld not use `dma_fence_is_signaled_locked()` here, because
typo
ACK.
+ // according to the C backend's recommendations, that function is problematic + // and we should avoid calling that function with a lock held.
+ // SAFETY: `self` is valid by definition. We take the spinlock above.
Where?
The safety comment is wrong / outdated. See below.
+ let ret = unsafe { bindings::dma_fence_is_signaled(fence) };
+ // To guarantee that an API caller can 100% rely on the signalling being + // completed (i.e., all fence callbacks ran), we have to take the lock. + // + // The reason is that the C dma_fence backend currently does not carefully + // synchronize the `dma_fence_is_signaled()` function with the proper + // spinlock. This can lead to the function returning `true` while fence + // callbacks are still being executed. This can be mitigated by guarding + // the entire function with the spinlock. + // + // See commit c8a5d5ea3ba6a.
+ // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is + // merely a pointer to an integer, which lives as long as this function. + unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };
Shouldn’t this be before the “is_signaled” ffi call? Or is this only about ensuring all callbacks have run? i.e.: is “ret” valid even though it was computed before taking the lock?
OK, this is where it gets ugly.
So during the last weeks I've been struggling to get the C backend into better shape. One issue from my POV is that the C dma_fence spinlock does not protect the fence state; there is insistence that the lock shall only protect the callback list.
The function dma_fence_is_signaled() has an unlocked fast path check:
https://elixir.bootlin.com/linux/v7.2-rc3/source/include/linux/dma-fence.h#L...
whereas setting of that bit is done under lock-protection:
https://elixir.bootlin.com/linux/v7.2-rc3/source/drivers/dma-buf/dma-fence.c...
This can lead to funny races like in the commit mentioned in the comment block above (c8a5d5ea3ba6a).
And it also leads to weird hacks like this:
https://elixir.bootlin.com/linux/v7.2-rc3/source/drivers/gpu/drm/amd/amdgpu/...
Now, in principle I agree with you that a pattern like this:
dma_fence_lock_irqsave(…); let signaled = dma_fence_is_signaled_locked(…); dma_fence_unlock_irqrestore(…);
would be better.
However, lengthy discussions with Christian seem to settle at the point where Christian sees the very strict requirement of never calling fence callbacks under lock protection, and where he views dma_fence_is_signaled_locked() as a broken function that should be removed.
He's currently working on removing all bits where fence callbacks are invoked under lock protection:
https://lore.kernel.org/dri-devel/20260624122917.2483-1-christian.koenig@amd...
There's been a ton of discussions and proposals about that in recent weeks
https://lore.kernel.org/dri-devel/20260608142436.265820-2-phasta@kernel.org/ https://lore.kernel.org/dri-devel/20260612104251.2264707-2-phasta@kernel.org...
So tl;dr: The weird code you're commenting on above ensures that
a) the fence->ops->is_signaled() callback is not called under lock protection and b) taking and releasing the lock guarantees that all callbacks are really finished, i.e. they have run.
(I continue to believe that setting the bit under lock protection and reading it without lock is fundamentally broken and needs to be fixed, but fixes are being rejected because of claimed performance regressions years ago when this was tried, because checking the bit is some sort of fast path check for.. parties that spin on dma_fence_is_signaled() ??)
[…]
+} +// Necessary to guarantee that `inner` always comes first and can be freed by C. +// Also useful for using casts instead of container_of(). +#[repr(C)] +#[pin_data] +struct DriverFenceData<'a, T: Send + Sync + FenceCtxOps> { + #[pin] + /// The inner fence. + // Must always be the first member so that unsafe casting works; but also + // necessary so that the C backend can free the allocation (coming from our + // Rust code) with kfree_rcu(). + inner: Fence, + /// Callback head for dropping this in a deferred manner through RCU. + rcu_head: bindings::callback_head, + /// Reference to access the FenceCtx. Useful for obtaining name parameters. + fctx: &'a FenceCtx<T>,
This creates a self-referential borrow in the JobQueue, since it holds both (a) the context to mint new seqnos from and (b) the xarray with fences which borrow from that same context. This issue does not exist with the Arc and we should really fix it before moving with this series.
Can you detail that a bit more. So your JobQueue version has a `data: T` where T holds both the xarray and a jobqueue. The xarray contains fences that hold references to the FenceCtx. But shouldn't that be fine since the life time ensures that the drop order is correct?
btw we are recently coming up with a proposal on how JobQueue could store DriverFences for the driver. I want to present some RFC for that soonish.
+ /// The API user's data. This must either not need drop, or must delay its + /// drop by a grace period. It is essential that the data only performs + /// operations legal in atomic context in its [`Drop`] implementation. + #[pin] + data: T::FenceDataType, +}
[…]
+ // DriverFenceData is repr(C) and a Fence is its first member.
+ let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>;
Without a “CAST:” keyword, I think this will trigger the linter?
Didn't see a complaint from clippy nor compiler.
[…]
lifetime is a single word.
ACK.
+}
+impl<'a, T: Send + Sync + FenceCtxOps> Deref for DriverFenceBorrow<'a, T> { + type Target = DriverFence<'a, T>;
+ fn deref(&self) -> &Self::Target { + self.driver_fence.deref() + } +}
+// SAFETY: The Rust dma_fence abstractions are already designed around the inner +// C `dma_fence`, which can serve safely as the identification point when being +// owned by C. Moreover, safety is ensured by not dropping `DriverFence` and by +// only allowing operations without side effects on the Borrowed type. +unsafe impl<'b, T: Send + Sync + FenceCtxOps + 'static> ForeignOwnable for DriverFence<'b, T> { + type Borrowed<'a> + = DriverFenceBorrow<'a, T> + where + Self: 'a; + type BorrowedMut<'a> + = DriverFenceBorrow<'a, T>
We should have a separate type for mutable borrows, IMHO.
No hard objections. But because it's convention, or because you see other advantages?
Thanks
P.