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

[Philipp Stanner <phasta@mailbox.org>]

On Wed, 2026-10-28 at 14:38 +0100, Christian König wrote:
> On 10/28/26 10:47, Philipp Stanner wrote:
> > > > 	commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> > > 
> > > This bug is exactly what I'm trying to prevent here.
> > 
> > What is your proposed strategy for preventing it?
> 
> nouveau_fence_signal() is consistently used by all signaling path to
> both signal and cleanup a fence.

No, you miss the point. Look at the bug. It exists because nouveau
thinks it can drop a reference now because the fence is signaled.

I grant the point, though, that this is actually a refcounting issue.
The path that is signaling does not have its own extra reference.

> When we stop calling callbacks with locks held and signaling fences
> from dma_fence_is_signaled() and dma_fence_enable_signaling() nouveau
> can call nouveau_fence_signal() consistently from all cleanup paths
> with the proper handling.
> 
> This makes it completely unecessary to have those cleanup hacks as
> well as the nouveau_fence_release() callback.
> 
> Background is that you can then put the fence context directly after
> the fence is signaled 
> 
> > > This problem only exists because we call the backend operations with
> > > the lock held!
> > 
> > Nope, this particular bug exists because Nouveau thought that it can
> > drop its reference when dma_fence_is_signaled() returns true, which is
> > not the case, because the fence backend has a race condition there.
> 
> No it doesn't, the fence backend works exactly like intended.

You mean the lockless checking of a bit that is set under lock-
protection works as intended?

> 
> I agree that we made tons of mistakes here but that is actually one
> of the few things which we got right on dma_fences from the very
> beginning.
> > A race condition that was allowed for performance reasons.
> 
> Well it is at least partially for correctness as well.
> 
> It is mandatory for quite a number of use cases that the signaling
> status becomes visible before the callbacks finish because otherwise
> you run into race conditions with the callbacks again.

That's not what I mean. There are two things here:

a) signaled-bit is read without lock-protection
b) signaled-bit is set before callbacks execute

fixing a) makes b) irrelevant.

But that's not even my point in this mail.


I'm still interested btw in who is busy-polling on a fence, waiting
until the signaled-bit gets true (that would be the "performance"
argument).

If you want to know when a fence changes from unsignaled to signaled,
use this function:

dma_fence_add_callback()

problem solved.

> > What signaling handler are you referring to?
> 
> Take a look at the hack nouveau_fence_is_signaled() does for example.
> The cleanup part the other signaling call path do is completely
> missing here because the function can't take the fctx->lock.

Again, ops->signaled() merely has to read an atomic64_t and return a
bool. Driver locks are not required here.

> 
> Yes, but if we want to get away from the external lock concept in
> dma_fence we must allow drivers to take their own fctx->lock inside
> the enable_signaling callback.
> 
> Just take a look at what nouveau_fence_enable_signaling() does, that
> is only possible because the dma_fence framework has grabbed fctx-
> >lock before calling this function.
> 
> The alternative is forcing drivers to use lockless approaches for
> their internal handling, and that is much much worse than that a
> dma_fence signaling status is visible without taking the lock.

[…]

> Just take nouveau_fence_enable_signaling() as an example, if we don't
> use fctx->lock as external dma_fence lock then the whole approach
> used here doesn't work any more.

My entire point is that the driver does not need any driver-lock
conflicting with the fence-lock-order at all.

I tried to point this out several times, but you keep iterating back to
"this is what nouveau does".

That nouveau does it does not proof that it's necessary. It just proofs
that nouveau does it.

> > > 
> > My impression was more that central design criteria for dma_fence (like
> > module unload of the driver who issued your fence, which is designed to
> > live arbitrarily long) were ignored, there was no clear guidance for
> > drivers how they should use what callback, and then driver problems
> > were solved in dma_fence, for instance with this shared lock. So same
> > story as with drm_sched.
> 
> Yes, exactly that.

Good!

So we agree that the dma_fence backend should give clear guidance about
what a driver must, must not and should do in its backend_ops
implementations.

Right?

Because so far the conversation sounded to me like "the driver must be
able to do whatever it wants and therefore needs to be able to grab
whatever locks".

> 
> 
> And that is strong evidence. Nouveau is actually the perfect example
> of why this approach causes so much trouble for driver
> implementations.

Calling Nouveau a "perfect example" is like calling Ronald McDonald a
role model in dietary choices.

Nova is being written right now, for reasons.

> 
> And such guidance should not exist in the first place.
> 
> It is up to the driver if they want to use locks for their internal
> state protection and the dma_fence framework should absolutely not
> enforce anything it doesn't need to deal with.
> 
> That the dma_fence lock should protect the state of the dma_fence is
> something I can perfectly agree on, but that does not include the ops
> call chain because that isn't dma_fence state in the first place.
> 
> > The only evidence I have ever seen is the fence-list-lock, and it's
> > just not clear at all why any callback would need access to that.
> 
> Because they need to clean things up after signaling.
> 
> > Let's walk it through together, shall we?
> > 
> > ops->release() and ->wait(): deprecated
> > 
> > ops->get_XXX_name() -> never needs driver locks
> > 
> > ops->signaled(): "Driver, here you read an atomic integer, the sequence
> > number, and tell the fence backend whether this fence shall be regarded
> > as completed"
> > No need for any driver lock.
> 
> Incorrect, you can only work without driver locks here when you delegate the cleanup.
> 
> In other words either to a worker or the ->release callback, both are things we try to avoid.
> 
> Again see nouveau as a perfect example of the problem.
> 
> > If this particular driver also signals fences through an interrupt
> > handler, it can take a list lock, then signal the fence. The ops must
> > not do that.
> > Why would a driver need both modes, sw *and* hw signaling? And why then
> > is it supposedly the case that both paths need to keep a list updated?
> 
> Because the signaling through the interrupt is mandatory for correct,
> the signaling through the is_signaled callback is then on top for
> performance optimizations.

is_signaled() never conflicts with any lock, see my list above.

I think at this point you and I should really first get in sync
regarding what the backend_ops are supposed to do, because we seem to
turn in circles a bit in our discussion.

And the only ops that seems to be controversial and that informs the
nouveau argument is ops->enable_signaling()

What should enable_signaling() do, in your mind? What should we tell
the driver to do in there?

And why does nouveau even implement it, when its interrupt handler will
ultimately signal the fence anyways. Since, as usual, nothing is
documented, and one enable_signaling function carries literally the
name "no_signaling", I don't give it the benefit of the doubt and just
assume that this is broken, bad design.


My impression is that ops->enable_signaling() and having an interrupt
handler signaling fences is mutually exclusive design-wise.


P.
>