[PATCH] dma-buf: fix dma_fence_array_signaled

[Christian König]

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 at amd.com>
>>>> ---
>>>>   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;
>>>>   }
>>