Re: [PATCH] dma-fence: Bypass signaling annotation from dma_fence_is_signaled

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 09/06/2023 13:52, Christian König wrote:
> Am 09.06.23 um 14:09 schrieb Tvrtko Ursulin:
>>
>> On 09/06/2023 07:32, Christian König wrote:
>>> Am 08.06.23 um 16:30 schrieb Tvrtko Ursulin:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> For dma_fence_is_signaled signaling critical path annotations are an
>>>> annoying cause of false positives when using dma_fence_is_signaled and
>>>> indirectly higher level helpers such as dma_resv_test_signaled etc.
>>>>
>>>> Drop the critical path annotation since the "is signaled" API does not
>>>> guarantee to ever change the signaled status anyway.
>>>>
>>>> We do that by adding a low level _dma_fence_signal helper and use it 
>>>> from
>>>> dma_fence_is_signaled.
>>>
>>> I have been considering dropping the signaling from the 
>>> dma_fence_is_signaled() function altogether.
>>>
>>> Doing this together with the spin locking we have in the dma_fence is 
>>> just utterly nonsense since the purpose of the external spinlock is 
>>> to keep the signaling in order while this here defeats that.
>>>
>>> The quick check is ok I think, but signaling the dma_fence and 
>>> issuing the callbacks should always come from the interrupt handler.
>>
>> What do you think is broken with regards to ordering with the current 
>> code? The unlocked fast check?
> 
> Well it's not broken, the complexity just doesn't make sense.
> 
> The dma_fence->lock is a pointer to a spinlock_t instead of a spinlock_t 
> itself. That was done to make sure that all dma_fence objects from a 
> single context (or in other words hardware device) signal in the order 
> of their sequence number, e.g. 1 first, then 2, then 3 etc...
> 
> But when somebody uses the dma_fence_is_signaled() function it's 
> perfectly possible that this races with an interrupt handler which wants 
> to signal fences on another CPU.
> 
> In other words we get:
> CPU A:
> dma_fence_is_signaled(fence with seqno=3)
> fence->ops->signaled() returns true
> dma_fence_signal()
> spin_lock_irqsave(fence->lock)
> signal seqno=3
> ...
> 
> CPU B:
> device_driver_interrupt_handler()
> spin_lock_irqsave(driver->lock) <- busy waits for CPU A to complete
> signal seqno=1
> signal seqno=2
> seqno=3 is already signaled.
> signal seqno=4
> ...

Right I see. However hm.. would the order of be guaranteed anyway, if 
someone would be observing what CPU B is doing via the 
dma_fence_is_signaled->test_bit? And in which scenarios would it matter 
if out of order signaled status could be observed?

> Either fence->lock should not be a pointer or we should not signal the 
> fence from dma_fence_is_signaled().
> 
> I strongly think that later should be the way to go.

Despite having wrote the above, I don't have any objections to removing 
this either. I don't see anything in the contract that requires it, but 
it was probably a bit before my time to know why it was added so I don't 
know if it could cause any subtle issues. It should be okay to try and see.

Regards,

Tvrtko