Re: [PATCH v5 4/5] rust: Add dma_fence abstractions

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.