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@xxxxxxxxx>
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
