[PATCH 1/2] dma-buf: keep only not signaled fence in reservation_object_add_shared_replace

[Chunming Zhou]

Any update?


On 2017年10月25日 15:28, Christian König wrote:
> Am 25.10.2017 um 08:42 schrieb Chunming Zhou:
>>
>>
>> On 2017年10月24日 21:55, Christian König wrote:
>>> From: Christian König <christian.koenig at amd.com>
>>>
>>> The amdgpu issue to also need signaled fences in the reservation 
>>> objects
>>> should be fixed by now.
>>>
>>> Optimize the list by keeping only the not signaled yet fences around.
>>>
>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>> ---
>>>   drivers/dma-buf/reservation.c | 31 +++++++++++++++++++++----------
>>>   1 file changed, 21 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/reservation.c 
>>> b/drivers/dma-buf/reservation.c
>>> index b44d9d7db347..4ede77d1bb31 100644
>>> --- a/drivers/dma-buf/reservation.c
>>> +++ b/drivers/dma-buf/reservation.c
>>> @@ -145,8 +145,8 @@ reservation_object_add_shared_replace(struct 
>>> reservation_object *obj,
>>>                         struct reservation_object_list *fobj,
>>>                         struct dma_fence *fence)
>>>   {
>>> -    unsigned i;
>>>       struct dma_fence *old_fence = NULL;
>>> +    unsigned i, j, k;
>>>         dma_fence_get(fence);
>>>   @@ -162,9 +162,7 @@ reservation_object_add_shared_replace(struct 
>>> reservation_object *obj,
>>>        * references from the old struct are carried over to
>>>        * the new.
>>>        */
>>> -    fobj->shared_count = old->shared_count;
>>> -
>>> -    for (i = 0; i < old->shared_count; ++i) {
>>> +    for (i = 0, j = 0, k = old->shared_count; i < 
>>> old->shared_count; ++i) {
>>>           struct dma_fence *check;
>>>             check = rcu_dereference_protected(old->shared[i],
>>> @@ -172,10 +170,14 @@ reservation_object_add_shared_replace(struct 
>>> reservation_object *obj,
>>>             if (!old_fence && check->context == fence->context) {
>>>               old_fence = check;
>>> -            RCU_INIT_POINTER(fobj->shared[i], fence);
>>> -        } else
>>> -            RCU_INIT_POINTER(fobj->shared[i], check);
>>> +            RCU_INIT_POINTER(fobj->shared[j++], fence);
>>> +        } else if (!dma_fence_is_signaled(check)) {
>>> +            RCU_INIT_POINTER(fobj->shared[j++], check);
>>> +        } else {
>>> +            RCU_INIT_POINTER(fobj->shared[--k], check);
>>> +        }
>>>       }
>>> +    fobj->shared_count = j;
>>>       if (!old_fence) {
>>> RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
>> Here there is a memory leak for signaled fence slots, since you 
>> re-order the slots, the j'th slot is storing signaled fence, there is 
>> no place to put it when you assign to new one.
>> you cam move it to end or put it here first.
>
> Good point, thanks. Going to respin.
>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>>           fobj->shared_count++;
>>> @@ -192,10 +194,19 @@ reservation_object_add_shared_replace(struct 
>>> reservation_object *obj,
>>>       write_seqcount_end(&obj->seq);
>>>       preempt_enable();
>>>   -    if (old)
>>> -        kfree_rcu(old, rcu);
>>> -
>>>       dma_fence_put(old_fence);
>>> +
>>> +    if (!old)
>>> +        return;
>>> +
>>> +    for (i = fobj->shared_count; i < old->shared_count; ++i) {
>>> +        struct dma_fence *f;
>>> +
>>> +        f = rcu_dereference_protected(fobj->shared[i],
>>> +                          reservation_object_held(obj));
>>> +        dma_fence_put(f);
>>> +    }
>>> +    kfree_rcu(old, rcu);
>>>   }
>>>     /**
>>
>