[PATCH 2/2] dma-buf: try to replace a signaled fence in reservation_object_add_shared_inplace

[Christian König]

Am 15.11.2017 um 18:43 schrieb Chris Wilson:
> Quoting Christian König (2017-11-15 17:34:07)
>> Am 15.11.2017 um 17:55 schrieb Chris Wilson:
>>> Quoting Chris Wilson (2017-11-14 14:34:05)
>>>> Quoting Christian König (2017-11-14 14:24:44)
>>>>> Am 06.11.2017 um 17:22 schrieb Chris Wilson:
>>>>>> Quoting Christian König (2017-10-30 14:59:04)
>>>>>>> @@ -126,17 +127,28 @@ reservation_object_add_shared_inplace(struct reservation_object *obj,
>>>>>>>                            dma_fence_put(old_fence);
>>>>>>>                            return;
>>>>>>>                    }
>>>>>>> +
>>>>>>> +               if (!signaled && dma_fence_is_signaled(old_fence)) {
>>>>>>> +                       signaled = old_fence;
>>>>>>> +                       signaled_idx = i;
>>>>>>> +               }
>>>>>> How much do we care about only keeping one fence per-ctx here? You could
>>>>>> rearrange this to break on old_fence->context == fence->context ||
>>>>>> dma_fence_is_signaled(old_fence) then everyone can use the final block.
>>>>> Yeah, that is what David Zhou suggested as well.
>>>>>
>>>>> I've rejected this approach for now cause I think we still have cases
>>>>> where we rely on one fence per ctx (but I'm not 100% sure).
>>>>>
>>>>> I changed patch #1 in this series as you suggest and going to send that
>>>>> out once more in a minute.
>>>>>
>>>>> Can we get this upstream as is for now? I won't have much more time
>>>>> working on this.
>>>> Sure, we are only discussing how we might make it look tidier, pure
>>>> micro-optimisation with the caveat of losing the one-fence-per-ctx
>>>> guarantee.
>>> Ah, one thing to note is that extra checking pushed one of our corner
>>> case tests over its time limit.
>>>
>>> If we can completely forgo the one-fence-per-ctx here, what works really
>>> well in testing is
>>>
>>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
>>> index 5319ac478918..5755e95fab1b 100644
>>> --- a/drivers/dma-buf/reservation.c
>>> +++ b/drivers/dma-buf/reservation.c
>>> @@ -104,39 +104,19 @@ reservation_object_add_shared_inplace(struct reservation_object *obj,
>>>                                         struct reservation_object_list *fobj,
>>>                                         struct dma_fence *fence)
>>>    {
>>> -       struct dma_fence *replace = NULL;
>>> -       u32 ctx = fence->context;
>>> -       u32 i;
>>> -
>>>           dma_fence_get(fence);
>>>    
>>>           preempt_disable();
>>>           write_seqcount_begin(&obj->seq);
>>>    
>>> -       for (i = 0; i < fobj->shared_count; ++i) {
>>> -               struct dma_fence *check;
>>> -
>>> -               check = rcu_dereference_protected(fobj->shared[i],
>>> -                                                 reservation_object_held(obj));
>>> -
>>> -               if (check->context == ctx || dma_fence_is_signaled(check)) {
>>> -                       replace = old_fence;
>>> -                       break;
>>> -               }
>>> -       }
>>> -
>>>           /*
>>>            * memory barrier is added by write_seqcount_begin,
>>>            * fobj->shared_count is protected by this lock too
>>>            */
>>> -       RCU_INIT_POINTER(fobj->shared[i], fence);
>>> -       if (!replace)
>>> -               fobj->shared_count++;
>>> +       RCU_INIT_POINTER(fobj->shared[fobj->shared_count++], fence);
>>>    
>>>           write_seqcount_end(&obj->seq);
>>>           preempt_enable();
>>> -
>>> -       dma_fence_put(replace);
>>>    }
>>>    
>>>    static void
>>>
>>>    i.e. don't check when not replacing the shared[], on creating the new
>>>    buffer we then discard all the old fences.
>>>
>>> It should work for amdgpu as well since you do a ht to coalesce
>>> redundant fences before queuing.
>> That won't work for all cases. This way the reservation object would
>> keep growing without a chance to ever shrink.
> We only keep the active fences when it grows, which is effective enough
> to keep it in check on the workloads I can find in the hour since
> noticing the failure in CI ;)

Not sure what tests you run, but this change means that we basically 
just add the fence to an ever growing list of fences on every command 
submission which is only reaped when we double the list size.

That is a serious no-go as far as I can see. What we could do is improve 
reservation_object_reserve_shared() as well to figure out how many 
signaled fences are actually in the array when we reallocate it.

> And on the workloads where it is being
> flooded with live fences from many contexts, the order of magnitude
> throughput improvement is not easy to ignore.

Well not sure about the Intel driver, but since we started to mostly use 
a single reservation object per process the time spend in those 
functions on amdgpu are completely negligible.

Regards,
Christian.

> -Chris