Re: [RFC PATCH v2 0/3] Solve dma_fence's problems by locking

["Christian König" <christian.koenig@amd.com>]

On 10/20/26 16:41, Philipp Stanner wrote:
> On Tue, 2026-10-20 at 14:02 +0200, Christian König wrote:
>> On 10/20/26 12:00, Philipp Stanner wrote:
>>> In our Rust code bases, I'm currently experiencing what we also
>>> experience in C at many places: that the dma_fence contract's RCU
>>> requirements make the API more fragile and put an unnecessary burden on
>>> the API user. Most notably, in C the API behaving correctly now depends
>>> on the API *user* waiting before free()s, waiting before unloads – all
>>> because dma_fence is not properly synchronized.
>>>
>>> The patches commit messages give more details about where I want to go
>>> with this particular RFC.
>>>
>>> My rough "vision" for dma_fence, where I think it should develop to, can
>>> be described like this:
>>>
>>> 1. All users should use inline-lock.
>>> 2. The fence-signaled state should be our decoupling point.
>>> 3. The fence state must be consistently locked.
>>
>>
>>> 4. This means all callbacks are only invoked for unsignaled fences, and
>>>    the fence cannot signal while a callback runs.
>>
>> First of all that prevents concurrent execution of the callbacks
>> which for a lot of use cases is a must have.
> 
> Currently only set_deadline() can execute in parallel.

Especially the is_signaling() callback is executed in parallel by tons of use cases.

> 
> IIUC in our past discussions, your wish has been that the driver can
> take its own locks with free locking-order in the callbacks.
> 
> Thus, the callback is invoked under lock in any case, just a few
> instructions earlier or later. Thus, no callback ever truly executes
> concurrently.

And that is fine. Taking the look when you actually need it to protect data structures is perfectly correct.

But what you propose here is to abuse the look as a barrier, e.g. to find out if something has finished or not and that is a completely different story.

>>
>> Then as I pointed out before the second problem is that this again
>> makes the existing lock inversion between the signaling by polling
>> and signaling by interrupts even worse than it already is.
> 
> We asked for examples in our last discussions, and the only one ever
> presented was the list-access requiring the fctx-lock in
> nouveau_fence_no_signaling(). Nouveau is often not a good example.
> 
> Can you provide examples in the kernel where deadlock would occur?

Just exercise the simple example of a driver supporting both pull and interrupt:

Interrupt: Handler takes lock protecting a linked list, finds fence and signals it.

Polling: is_signaled is called with fence lock held, can't take list lock and so needs to work around that somehow.

This pattern repeats multiple times all over the different dma_fence implementation. Nouveu is just the tip of the iceberg, we have cases of this in amdgpu as well as i915 as well as XE.

> Note that I pointed out that all callbacks are already invoked with the
> lock taken, which proofs that it's doable.

Again, no it isn't. The shared lock was invented exactly because drivers ran into this problem.

The common pattern is the same, implementations just dance around the fact that the callbacks are called with the fence lock held by delegating cleanup work and/or using lockless containers etc...

A common component like dma_fence should never enforce users to do such workarounds just to solve it's own lifetime issues.

> Future users with inline-
> lock could employ various techniques for dealing with the lock-order.

And that's exactly what I'm trying to avoid.

> 
> Only exception currently is set_deadline(). The only user, MSM,
> acquires the lock manually and then nevertheless triggers work on a
> kthread, examplifying just one technique.
> 
>>
>> What we could use is an SRCU, but a spinlock or any kind of lock is
>> clearly the wrong approach.
>>
>> So as far as I can see that is a big no-go and we should really just
>> use RCU for everything and remove leaking the fence look into the
>> callbacks like I outlined before.
> 
> Look at patch 3. We might want to add some legacy support for shared-
> lock, but it is evident how much more simple the code gets.

To be honest I don't think that this is simpler at all.

Regards,
Christian.

> 
> Also:
> 
>>>
>>> Point №6 means that this:
>>>
>>> dma_fence_lock_irqsave(…);
> 
> I even forgot that you also need a
> 
> if (!dma_fence_is_signaled_locked(…))
> 
> here
> 
>>> dma_fence_set_error(f, err);
>>> dma_fence_signal_locked(f);
>>> dma_fence_unlock_irqrestore(…);
>>> synchronize_rcu();
>>> kfree(driver_foo);
>>>
>>> should simply become this:
>>>
>>> dma_fence_signal(f, err);
>>> kfree(driver_foo);
> 
> 
> :)