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

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

[Christian König]

From: Christian König <christian.koenig@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@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);
 		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);
 }
 
 /**
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christian König]

From: Christian König <christian.koenig@amd.com>

The amdgpu issue to also need signaled fences in the reservation objects should
be fixed by now.

Optimize the handling by replacing a signaled fence when adding a new
shared one.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/reservation.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 4ede77d1bb31..702ef03a923a 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -104,7 +104,8 @@ reservation_object_add_shared_inplace(struct reservation_object *obj,
 				      struct reservation_object_list *fobj,
 				      struct dma_fence *fence)
 {
-	u32 i;
+	struct dma_fence *signaled = NULL;
+	u32 i, signaled_idx;
 
 	dma_fence_get(fence);
 
@@ -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;
+		}
 	}
 
 	/*
 	 * memory barrier is added by write_seqcount_begin,
 	 * fobj->shared_count is protected by this lock too
 	 */
-	RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
-	fobj->shared_count++;
+	if (signaled) {
+		RCU_INIT_POINTER(fobj->shared[signaled_idx], fence);
+	} else {
+		RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
+		fobj->shared_count++;
+	}
 
 	write_seqcount_end(&obj->seq);
 	preempt_enable();
+
+	dma_fence_put(signaled);
 }
 
 static void
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chunming Zhou]

On 2017年10月24日 21:55, Christian König wrote:
> From: Christian König <christian.koenig@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@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.

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);
>   }
>   
>   /**

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christian König]

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@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@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);
>>   }
>>     /**
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@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@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);
>>>   }
>>>     /**
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christian König]

Looks like v2 never made it to the list. I've just send the V2 patches 
once more.

Christian.

Am 31.10.2017 um 08:26 schrieb 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@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@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);
>>>>   }
>>>>     /**
>>>
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

On 31/10/17 09:42 AM, Christian König wrote:
> Looks like v2 never made it to the list. I've just send the V2 patches
> once more.

FWIW, I received your v2 patches yesterday, and now twice today.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel