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

On 10/20/26 16:41, Philipp Stanner wrote:

On Tue, 2026-10-20 at 14:02 +0200, Christian König wrote:

quoted

On 10/20/26 12:00, Philipp Stanner wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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:

quoted

quoted

Point №6 means that this:

dma_fence_lock_irqsave(…);

I even forgot that you also need a

if (!dma_fence_is_signaled_locked(…))

here

quoted

quoted

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);

:)