Re: [PATCH 3/4] rust: Add dma_fence abstractions

[Daniel Almeida <dwlsalmeida@gmail.com>]

> On 5 Jun 2026, at 04:56, Philipp Stanner <phasta@mailbox.org> wrote:
> 
> On Thu, 2026-06-04 at 10:15 +0200, Boris Brezillon wrote:
>> On Wed, 3 Jun 2026 21:43:05 -0300
>> Daniel Almeida <dwlsalmeida@gmail.com> wrote:
>> 
>>>> On 3 Jun 2026, at 14:14, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>>> 
>>>> On Wed, 3 Jun 2026 13:41:02 -0300
>>>> Daniel Almeida <dwlsalmeida@gmail.com> wrote:
>>>>   
>>>>>> +    /// 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);    
>>>>> 
>>>>> This is a central point. We ideally would want this to consume self, because we
>>>>> may want to move things out of the callback.    
>>>> 
>>>> This one comes from me. The rationale being that ::called() is called
>>>> from an atomic context, and the resources attached to the callback data
>>>> might require acquiring other sleeping locks to be released, and
>>>> sometimes you don't even notice immediately because said resources are
>>>> refcounted, and the lock is only acquired when you happen to be the
>>>> last owner. Yes, those can be caught at runtime if the C side is
>>>> properly annotated with might_sleep(), but that's not always the case.
>>>> 
>>>> If we defer the drop of the data only when the FenceCb is
>>>> dropped/recycled, we're at least not constrained by this "runs in
>>>> atomic context" thing.
>>>>   
>>> 
>>> This design does not solve it, because one can quite trivially get around this
>>> restriction using Option<T> as I said. If your point is “don’t run any drop() here”,
>>> then &mut self doesn’t do it.
>> 
>> My bad, I thought you were talking about some Option<T> in
>> FenceCbRegistration<T> (there was one at some point, but it's gone now),
>> but you're talking about having an Option<X> inside the T. Yes, there's
>> indeed nothing preventing a drop on X in that path, and it's just as
>> bad as passing the fence back as value to the callback in that case.
> 
> Then maybe we should just pass it by value and require implementation
> of an unsafe trait on `T`, whose safety-requirements demand that this
> must be save to drop from atomic context.
> 
>> 
>>> 
>>>>> 
>>>>> Consider a fence design where signal() consumes self. Now consider this:
>>>>> 
>>>>> ```
>>>>> impl FenceCb for MyCallback {
>>>>> fn called(&mut self) {
>>>>>   // Can't move the fence out, so we have to put an Option<T> just to be able
>>>>>   // to move.
>>>>>   if let Some(f) = self.some_fence.take() {
>>>>>     f.signal();
>>>>>   }
>>>>> }
>>>>> ```
>>>>> 
>>>>> This used to be the case when our version of the job queue used the "proxy
>>>>> fence" design:
>>>>> 
>>>>> 
>>>>> ```
>>>>> // Callback on the hw fence
>>>>> impl FenceCb for MyCallback {
>>>>> fn called(&mut self) {
>>>>>   if let Some(f) = self.submit_fence.take() {
>>>>>     f.signal();
>>>>>   }  
>>>> 
>>>> I'm pretty sure lockdep won't like it anyway, because this is nested
>>>> locking of the same lock class. For such proxies, we'll need to teach
>>>> lockdep about the nesting like has been recently done on
>>>> dma_fence_array & co. But I'm digressing.  
>>> 
>>> Yeah, but this is more about resource transfer in general, not
>>> this pattern specifically.
>>> 
>>> I agree that this has issues, and yes, lockdep complained back
>>> then :)
>> 
>> The thing is, there's so many aspects that could go wrong because of the
>> context this callback is called in. Nested locking is one of them,
>> the fact we can't sleep is another. And with rust it's even worse,
>> because of the implicit drops that will happen when you take ownership
>> of resources (taking sleeping locks to remove resources from a dataset
>> for instance).
> 
> Doesn't that have to be a problem for much of Rust infrastructure? How
> do other parties solve that?
> 
>> 
>> So, by passing self by value to the ::callback(), you're basically
>> telling users "hey, BTW, don't forget to defer the drop to some
>> workqueue if you think it's not atomic-safe". And how can users know
>> that the thing they're about to drop can be dropped in atomic context?
>> They basically have to audit the ::drop() of all the resources they
>> embed in their type implementing FenceCb. Not only that, but they also
>> have to design the thing so the deferral of this ::drop() doesn't
>> allocate, because, obviously, allocating in atomic context is
>> tricky/fallible. AFAIK, none of this can be spot at compile-time (I
>> remember Gary/Danilo mentioning that we could teach the klint about
>> some of these rules). This would leave us with runtime checks like
>> might_sleep(), but most of the C putters (xxx_put(object)) don't have
>> might_sleep() in the path where the decref doesn't lead to a refcnt=0
>> situation.
>> 
>> TLDR; Call this PTSD if you want, but this is the sort of bugs I
>> struggled with on the C side, and I can predict that the exact same
>> will happen in rust drivers if we expose the FenceCb as it is designed
>> here and we don't have a way to check the soundness of the FenceCb
>> implementations at compile time.
> 
> My guess would be that the existence of unsafe-traits is the admission
> of Rust that this just cannot be guaranteed by design.
> 
> If a driver cannot know whether this or that is safe to drop, then it
> would have to defer it's dropping. Or would there be cases where this
> also doesn't work?


Although I totally understand where Boris is coming from here, and I agree with
him, the reality is that the current &mut self design doesn’t solve this. An
unsafe trait could work as a pinky-promise by drivers, which is half-way there.

What we ideally would like to have is a bound though, something like:

T: !Drop

If I recall correctly there were people working to get support for that on
Rust? I think there are two things here: !Trait, which is not supported except
for !Sized IIRC, and having an auto trait that represents types that implement
Drop, similar to Send and Sync.


> 
>> 
>> The other option (the one I've been advocating for from the start), is
>> to not let drivers implement FenceCb (make it private), but instead
>> have a bunch of implementations that we know are safe. Here's a list of
>> implementations that I think would unblock most of the drivers use
>> cases:
>> 
>> - wakeup a thread
>> - complete a completion object
>> - schedule a WorkItem
>> - schedule a kthread_worker (once we get a proper rust abstraction for
>>   that)
>> 
>> It doesn't mean we can't have optimized FenceCb implementations that do
>> a lot more in the callback() path instead of deferring to a
>> workqueue/thread, but at least those would have to be implemented in
>> dma_fence.rs, and the dma_fence.rs maintainers can then carefully audit
>> the code as part of the review process, which we know is not really the
>> case when changes touch drivers code only.
> 
> Pragmatically speaking, if the common cases are trivial, then the
> drivers will get them right, because those critical primitives are
> already atomic-safe.

No, you can still fumble trivial code, specially when it has to be cargo-culted
from somewhere else.  

I am not saying that having things in dma_fence . rs is the solution, although
it is definitely one solution. But the argument above isn’t very strong
IMHO.

> 
> And a non-common case will have to be implemented in the driver
> anyways, so we'd have to allow for that.
> 
> 
> 
> P.