[PATCH] dma-buf: fix dma_fence_array_signaled

[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.

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.

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;
>>>   }
>