On Fri, 2026-07-17 at 14:14 -0300, Daniel Almeida wrote:
+ 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?
On Mali, the hardware syncobj starts at 0. If you see a 0, is this the default state, or should you signal seqno 0?
This problem goes away if we can have seqnos starting at a custom value, like
- It seems like the C machinery also special-cases 0 in a few other places too.
ACK.
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.
Agree about the fail-free part.
The problem I see here is that new_fence() will use "seqno" and "nr" from whatever context called new_fence(), but DriverFenceAllocation has some other (possibly unrelated) context as its DriverFenceData::fctx.
The lifetimes are apparently broken too, because 'a is the lifetime of the context where new_fence_allocation was called, meaning that the context that actually called new_fence() can drop, even though it provided the state for dma_fence_init().
I guess this can be solved by moving new_fence() to impl DriverFenceAllocation? That already has a context, and most importantly, the right context.
Yes that is / was broken. Having the wrong fctx would cause wrong container_of() calls and therefore explode.
Fixed it locally following your suggestion of creating fences on the allocation object.
[…]
+/// 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? :)
For example, you say “a Fence cannot be signaled”. A person seeing this code for the first time might ask why. Specially if they start by reading the docs for Fence first.
I think explaining a bit more about the DriverFence/Fence/refcounting as you said is already enough to settle it.
OK, I flesh that out a bit.
[…]
+ 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() ??)
I see, there is a lot more context on this then. Can you merely add a comment saying it’s ok to call dma_fence_is_signaled() without the locks? Otherwise people might try to “fix” this down the line...
I tried to make it clear with the comment above:
// 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.
I can try to make it more explicit.
Actually, I suppose with our design in Rust we would not actually need this lock-unlock, because we ensure that it does not matter whether a callback already ran.
But since we don't know who will be working on this in 5 years, adding this or that exotic callback, or maybe registering callbacks on his own fence, I put this sequence there for robustness.
So if the function returns true, you really know that all callbacks are gone forever.
[…]
+ /// 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.
I recommend the CAST thing anyways. It’s being adopted in other parts of the kernel crate.
I can write a CAST comment, no problem.
Thx P.