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

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

On 10/28/26 10:47, Philipp Stanner wrote:
> On Tue, 2026-10-27 at 15:59 +0100, Christian König wrote:
>> On 10/21/26 14:49, Philipp Stanner wrote:
>>> On Wed, 2026-10-21 at 13:53 +0200, Christian König wrote:
>>>> On 10/20/26 17:36, Philipp Stanner wrote:
>>> […]
>>>
>>>>>
>>>>> Nope, I want to protect shared data, which is the signaled-state. Which
>>>>> is currently a race-condition worked around with RCU (and sometimes
>>>>> with manually doing the right thing (locking!)) [1], stemming from the
>>>>> fact that the fence's spinlock is not used consistently.
>>>>
>>>> The fence spinlock is consistently used for protecting the fence
>>>> signaling, in other words it always happens with the spinlock held,
>>>> same for adding callbacks.
>>>>
>>>> The only thing which happens without the spinlock held is the fast
>>>> path checking of the signaled and enable_signaling bits and as far as
>>>> I can see that is ok as well.
>>>
>>> No, it isn't. It makes the API fragile.
>>>
>>> 	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.

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.

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.

And even if that wouldn't be mandatory for correctness the performance requirements just make it mandatory. Just take a look at how much people complain when we took the signaling callback away from radeon because of the locking issues.
>> What nouveau does is to delegate the cleanup since it can't do that
>> in the signaling handler because of the lock inversion.
> 
> 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.

>> This problem doesn't happen any more if you just completely drop the
>> approach of calling the backend operations with the fence lock held
>> and then re-structure the cleanup handling in Nouveau.
> 
> ops->signaled() is typically not called with the lock held. You might
> be referring to ops->enable_signaling(), which is consistently locked.

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.

>>> 	commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free
>>> of VM")
>>
>> I already discussed that with Simona back then. Simona actually
>> explicitly rejected my approach to take the spinlock to check for
>> signaling. Pointing out that amdgpu is using the API in a wrong way.
>>
>> The problem is that amdgpu doesn't track the callback structure and
>> so can't remove the callback. It's on my TODO list every since to fix
>> that miss use of the API.
> 
> You mean the issue is that amdgpu cannot deregister callbacks?
> 
> I don't see how that's related. The lock-unlock-cycle clearly ensures
> that amdgpu cannot continue while the fence is signaling.
> 
> To clarify: our conversation circles around two partially independent
> topics:
> 
> a) the signaled-bit is read without lock-protection as a fast path in
> dma_fence_is_signaled() and
> 
> b) the ops are not consistently called with the lock held.

Completely agree.

I mean regarding a) if we add the spinlock to dma_fence_is_signaled() I can live with that.

It's just that Simona explicitely rejected it when I tried that and we will have quite a crowd complaining about the performance and a handful of people complaining that this broke their timing.

I know that relying on the timing is also questionable to begin with but yeah some drivers really do that.

Regarding b) I honestly don't see another option as to stop calling the ops with the lock held, except for forcing all drivers to use a lockless approach.

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.

>>> Tell me one thing:
>>> If locking is the wrong approach, then how come it was added to
>>> literally all callbacks except for set_deadline()? Seems to me that it
>>> must have solved some issues.
>>
>> Well, it's because concerns with the lock inversion brought up by
>> Alex and me was ignored and the patches merged anyway.
>>
>> This is just another one of those DMA-buf design problems which would
>> have been fully preventable if I would have clearly NAKed that stuff
>> from the very beginning.
> 
> 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.

> If *everything* would have been locked from the get-go, that would have
> forced drivers to design their locking correctly, and decide wisely
> what tasks they want to perform where.

That's what I disagree on. Locking everything doesn't help, it just solve a problem in one place but complicates things in another place.

It's like shifting the hot potato from one side of the mouth to the other.

>> Back then I made the mistake to believe the argumentation you come up
>> here as well and so I don't explicitly rejected that approach, but by
>> now we have strong evidence that this was a mistake and I'm trying to
>> fix that mistake ever since.
> 
> Well, I am a different person with my own background that informs my
> thinking. Please keep that in mind.
> 
> The "strong evidence" you claim to exist has still not been shown to
> me. I show you concrete bugs that we could have prevented, but the only
> piece of code you ever showed me where something would break is the
> list_del() in nouveau.

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

>> We should have never taken the lock for the callbacks in the first
>> place, this just made the implementation much more complicated and
>> caused way more problems than the few raised by putting the ops
>> pointer under RCU.
> 
> Lock-inversions are not being solved by just not taking the lock, they
> are addressed by having good lock design. We recently solved exactly
> that in the Rust JobQueue, where drivers also reported lock-inversions,
> but we could eliminate them with just a few discussions, discovering
> that the driver does not need these locks when interacting with the
> JobQueue.
> 
> Often enough btw the only object the driver really needs is the
> sequence number anyways, which can trivially be passed and shared
> around.
> 
> Now with dma_fence, I strongly suspect that the problem came to be
> because of no clear guidance by dma_fence as to what the driver must
> and must not do in its ops implementation.

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.

> ops->enable_signaling(): "Make sure that this fence will get signaled.
> Register an interrupt or schedule a work item". These locks also don't
> interfere with the fence-lock.

Yes they do, see nouveau_fence_enable_signaling().

> ops->set_deadline(): clock up the CPU frequency. The only implementor
> trivially does that through a work item, too – after voluntarily taking
> the fence lock over the entire section.
> 
> 
> As you saw from our recent amdgpu discussions, the confusion of what a
> ops is even good for caused drivers to mess around with the fence
> directly in their callbacks, checking for the signaled state etc.

Yes and that is exactly what drivers should do.

> It was too much freedom that has caused the trouble. There are no
> canonical solutions now.

Yes there are. Use RCU protection for the ops and the per dma_fence spinlock for the dma_fence state, problem solved.

As far as I can see that approach fulfills all requirements we have and there is absolutely no reason any more to take the lock while calling any of the ops pointers.

> So except for updating the linked list in ops, can you show me any code
> example where a lock inversion would occur? I'm quite confident that it
> will turn out that the driver has no good justification for performing
> the related acts in the ops in the first place.

Again, drivers don't need a justification! The dma_fence framework needs a justification to restrict the drivers!

And that justification is not given any more when we drop the concept of a shared lock.

Christian.

> 
> 
> P.