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
> 1. 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#L551
> >
> > 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#L362
> >
> >
> > 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/amdgpu_vm.c#L2775
> >
> >
> > 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/[email protected]/
> >
> > There's been a ton of discussions and proposals about that in recent
> > weeks
> >
> > https://lore.kernel.org/dri-devel/[email protected]/
> > https://lore.kernel.org/dri-devel/[email protected]/
> >
> >
> > 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.
