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

[Daniel Almeida]

> 
>>> +        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.

> 
>> 
>>> +            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.

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.

> 
>> 
>>> 
> 
> […]
> 
>>> +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.

No, variable names are fine IMHO. It’s only types where I think we should be a bit
more verbose.

> 
> 
>> 
>>> +    /// 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.

Is the information that a callback was called more important than the fact that
it was called because a fence was signaled? I think that a driver author would
rather implement “what happens when the fence is signaled” versus
“what happens when the callback is called”. But this is pretty minor.
If you still think that called is better, I don’t object either :)

> 
>> 
>>> +}
>>> +
>>> 
> 
> […]
> 
>>> +
>>> +/// 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.


> 
>> 
>>> +#[repr(transparent)]
>>> +pub struct Fence {
>>> +    /// The actual dma_fence passed to C.
>>> +    inner: Opaque<bindings::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#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/20260624122917.2483-1-christian.koenig@xxxxxxx/
> 
> There's been a ton of discussions and proposals about that in recent
> weeks
> 
> https://lore.kernel.org/dri-devel/20260608142436.265820-2-phasta@xxxxxxxxxx/
> https://lore.kernel.org/dri-devel/20260612104251.2264707-2-phasta@xxxxxxxxxx/
> 
> 
> 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...

> 
>> 
>>> 
> 
> […]
>> 
>> 
>>> +    /// 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.

> 
>> 
>>> +
>>> 
> 
> […]
> 
>> 
>> 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?

Actually, when I first made this comment, I think pointers were being used.
Back then I wanted one type to deref to *const and the other one to deref to
*mut. This does not seem to be the case anymore, so please forget what I just
said :)

> 
> 
> 
> Thanks
> 
> P.
>>