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

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

On 11/2/26 10:35, Philipp Stanner wrote:
> 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.

Well yes and no.

It's true that the path which needs the extra reference should have one and drop it as soon as it doesn't need it any more. But the real question is why does that path exists in the first place?

What nouveau seems to do here is to install a callback on it's own fence to do some cleanup when the fence signals.

But that is nouveaus own fence, why doesn't it do the cleanup after it has explicitly called dma_fence_signal_*()?
>> 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?

Yes, exactly that and it is not even a performance improvement in the sense that it makes things execute faster.

In other words we have use cases where the latency needs to be as low as possible and the state change that a fence has signaled visible to other CPU cores *before* the callbacks have finished executing.

Take the "gang submit" use case as an example, here we have two submissions to two different HW engines which depend on each other. After filling the common ring buffer submission A busy waits on submission B to start and drain the ring buffer again. This busy waiting inside the HW consumes power and so drivers try to avoid it as much as possible.

Another example is scheduling dependent page flips etc...
>> 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.

Not it doesn't. It prevents intentional execution of multiple things in parallel.

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

Well that is the point I'm trying to explain. Nobody is busy polling the signaled bit! That is not why we do that.
> If you want to know when a fence changes from unsignaled to signaled,
> use this function:
> 
> dma_fence_add_callback()
> 
> problem solved.

That would enable interrupts.

>>> 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 they are. Take a look why nouveau installs the callback.

>>
>> 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.

And that is what I strongly disagree on.
> 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.

But nouveau is a really really good example because it never installed the same workaround to the problem modern drivers amdgpu or XE have.

Yes it is possible to work around the problem in drivers, it's just not a good idea because you are moving the problem from the framework into the drivers.

>>>>
>>> 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?

Correct, I completely agree on that.

> 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".

Yes, because that is something drivers should be able to do. Not allowing drivers that is a bad guidance from the framework.

It pushes the drivers into the wrong direction of using fragile approaches.

The much better guidance would be to have a single signaling path in which drivers call dma_fence_signal_*() so that drivers can start to cleanup after doing so.

It is still possible that drivers mess things up with both approaches, it's just that I think it is less likely with that one.
>> 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.

Oh that is a good one. Thanks for brightening up my day :D

> Nova is being written right now, for reasons.

My point is that nouveau is a good example just because it is so broken.

Take a look at amdgpu, it does the whole interrupt signaling with RCU protection and doesn't even implement the is_signaled callback for the same reason nouveau has bugs regarding the fence signaling.
>> 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?

The enable_signaling callback let's the backend know that some code depends on the fence to signal.

The backend can do different things to enable that, e.g. enabling HW interrupts, start a worker to suspend HW queues, or start a worker to poll the status in certain intervals etc...

The point is the framework doesn't know what the backend needs to do and it also shouldn't limit what the backend can do here.

What the framework needs to do is to give the right guidance, e.g. check if enabled signaling is called multiple times and only forward the first one to the driver, check if the fence is already signaled, check if the backend is even still around...
> 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.

Well if a backend wants to implement both use cases then it is necessary for it to enable/disable interrupts based on calls to enable_signaling.

And when you have only one interrupt control for multiple fences then you need to common lock to protect that one interrupt control.

Regards,
Christian.

> 
> 
> P.
>>