> On 5 Jun 2026, at 04:56, Philipp Stanner <[email protected]> wrote:
>
> On Thu, 2026-06-04 at 10:15 +0200, Boris Brezillon wrote:
>> On Wed, 3 Jun 2026 21:43:05 -0300
>> Daniel Almeida <[email protected]> wrote:
>>
>>>> On 3 Jun 2026, at 14:14, Boris Brezillon <[email protected]>
>>>> wrote:
>>>>
>>>> On Wed, 3 Jun 2026 13:41:02 -0300
>>>> Daniel Almeida <[email protected]> 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.
