Am 08.11.24 um 16:52 schrieb Tvrtko Ursulin:
On 08/11/2024 14:18, Christian König wrote:
Am 08.11.24 um 14:01 schrieb Tvrtko Ursulin:
On 08/11/2024 09:42, Christian König wrote:
The function silently assumed that signaling was already enabled
for the
dma_fence_array. This meant that without enabling signaling first
we would
never see forward progress.
Fix that by falling back to testing each individual fence when
signaling
isn't enabled yet.
Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
drivers/dma-buf/dma-fence-array.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-fence-array.c
b/drivers/dma-buf/dma-fence-array.c
index 46ac42bcfac0..01203796827a 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -103,10 +103,22 @@ static bool
dma_fence_array_enable_signaling(struct dma_fence *fence)
static bool dma_fence_array_signaled(struct dma_fence *fence)
{
struct dma_fence_array *array = to_dma_fence_array(fence);
+ unsigned int i, num_pending;
- if (atomic_read(&array->num_pending) > 0)
+ num_pending = atomic_read(&array->num_pending);
+ if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&array->base.flags)) {
+ if (!num_pending)
+ goto signal;
return false;
+ }
+
+ for (i = 0; i < array->num_fences; ++i) {
+ if (dma_fence_is_signaled(array->fences[i]) &&
!--num_pending)
+ goto signal;
+ }
+ return false;
Sampling num_pending (and decrementing) and test_bit from an
unlocked path makes one need to think if there are consequences,
false negatives, positives or something. Would it be fine to
simplify like the below?
Yeah I've played around with those ideas as well but came to the
conclusion that neither of them are correct.
static bool dma_fence_array_signaled(struct dma_fence *fence)
{
struct dma_fence_array *array = to_dma_fence_array(fence);
unsigned int i;
if (atomic_read(&array->num_pending)) {
for (i = 0; i < array->num_fences; i++) {
if (!dma_fence_is_signaled(array->fences[i]))
return false;
That's not correct. num_pending is not necessary equal to the number
of fences in the array.
E.g. we have cases where num_pending is just 1 so that the
dma_fence_array signals when *any* fence in it signals.
I forgot about that mode.
}
}
dma_fence_array_clear_pending_error(array);
return true;
}
Or if the optimisation to not walk the array when signalling is
already enabled is deemed important, perhaps a less thinking
inducing way would be this:
...
Decrementing locally cached num_pending in the loop I think does not
bring anything since when signalling is not enabled it will be stuck
at num_fences. So the loop walks the whole array versus bail on
first unsignalled, so latter even more efficient.
That is not for optimization but for correctness.
What the patch basically does is the following:
1. Grab the current value of num_pending.
2. Test if num_pending was potentially already modified because
signaling is already enabled, if yes just test it and return the result.
3. If it wasn't modified go over the fences and see if we already
have at least num_pending signaled.
I should probably add a code comment explaining that.
It would be good yes.
DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT can appear any time, even after the
check, hence I am not sure the absence of the bit can be used to
guarantee num_pending is stable. But I think this one is safe, since
the loop will in any case find the desired number of signalled fences
even if num_pending is stale (too high).
Also, can num_pending underflow in signal-on-any mode and if it can
what can happen in unsigned int num_pending and the below loop.
Potentially just one false negative with the following query returning
signalled from the top level dma-fence code.
Oh, good point. Yeah num_pending can indeed underflow when signaling is
enabled. I've fixed the check accordingly.
In summary I think patch works. I am just unsure if the above race can
silently happen and cause one extra round trip through the query. If
it can it still works, but definitely needs a big fat comment to
explain it.
I've added the summary from Boris as comment.
Thanks,
Christian.
Regards,
Tvrtko
In which case, should dma-fence-chain also be aligned to have the
fast path bail out?
Good point need to double check that code as well.
Thanks,
Christian.
Regards,
Tvrtko
+signal:
dma_fence_array_clear_pending_error(array);
return true;
}
