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

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

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

v2: temporary put the signaled fences at the end of the new container

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

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index b44d9d7db347..6fc794576997 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 = fobj->shared_max; i < old->shared_count; ++i) {
 		struct dma_fence *check;
 
 		check = rcu_dereference_protected(old->shared[i],
@@ -172,10 +170,18 @@ 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 {
+			/*
+			 * Temporary save the signaled fences at the end of the
+			 * new container
+			 */
+			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 +198,20 @@ 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;
+
+	/* Drop the references to the signaled fences */
+	for (i = k; i < fobj->shared_max; ++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 1/2] dma-buf: keep only not signaled fence in reservation_object_add_shared_replace v2

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

v2: temporary put the signaled fences at the end of the new container

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

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index b44d9d7db347..6fc794576997 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 = fobj->shared_max; i < old->shared_count; ++i) {
 		struct dma_fence *check;
 
 		check = rcu_dereference_protected(old->shared[i],
@@ -172,10 +170,18 @@ 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 {
+			/*
+			 * Temporary save the signaled fences at the end of the
+			 * new container
+			 */
+			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 +198,20 @@ 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;
+
+	/* Drop the references to the signaled fences */
+	for (i = k; i < fobj->shared_max; ++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

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

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

[Christian König]

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.

v2: temporary put the signaled fences at the end of the new container

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

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index b44d9d7db347..6fc794576997 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 = fobj->shared_max; i < old->shared_count; ++i) {
 		struct dma_fence *check;
 
 		check = rcu_dereference_protected(old->shared[i],
@@ -172,10 +170,18 @@ 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 {
+			/*
+			 * Temporary save the signaled fences at the end of the
+			 * new container
+			 */
+			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 +198,20 @@ 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;
+
+	/* Drop the references to the signaled fences */
+	for (i = k; i < fobj->shared_max; ++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]

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 6fc794576997..a3928ce9f311 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

_______________________________________________
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 v2

[Chunming Zhou]

On 2017年10月31日 16:43, Christian König wrote:
> 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.
>
> v2: temporary put the signaled fences at the end of the new container
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/dma-buf/reservation.c | 36 ++++++++++++++++++++++++++----------
>   1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index b44d9d7db347..6fc794576997 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 = fobj->shared_max; i < old->shared_count; ++i) {
>   		struct dma_fence *check;
>   
>   		check = rcu_dereference_protected(old->shared[i],
> @@ -172,10 +170,18 @@ 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 {
> +			/*
> +			 * Temporary save the signaled fences at the end of the
> +			 * new container
> +			 */
> +			RCU_INIT_POINTER(fobj->shared[--k], check);
> +		}
>   	}
> +	fobj->shared_count = j;
>   	if (!old_fence) {
>   		RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
Seems you still don't address here, how do you sure 
fobj->shared[fobj->shared_count] is null? if not NULL, the old one will 
be leak.

Regards,
David Zhou
>   		fobj->shared_count++;
> @@ -192,10 +198,20 @@ 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;
> +
> +	/* Drop the references to the signaled fences */
> +	for (i = k; i < fobj->shared_max; ++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 v2

[Christian König]

Am 31.10.2017 um 09:51 schrieb Chunming Zhou:
>
>
> On 2017年10月31日 16:43, Christian König wrote:
>> 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.
>>
>> v2: temporary put the signaled fences at the end of the new container
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/reservation.c | 36 
>> ++++++++++++++++++++++++++----------
>>   1 file changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/dma-buf/reservation.c 
>> b/drivers/dma-buf/reservation.c
>> index b44d9d7db347..6fc794576997 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 = fobj->shared_max; i < old->shared_count; 
>> ++i) {
>>           struct dma_fence *check;
>>             check = rcu_dereference_protected(old->shared[i],
>> @@ -172,10 +170,18 @@ 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 {
>> +            /*
>> +             * Temporary save the signaled fences at the end of the
>> +             * new container
>> +             */
>> +            RCU_INIT_POINTER(fobj->shared[--k], check);
>> +        }
>>       }
>> +    fobj->shared_count = j;
>>       if (!old_fence) {
>> RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
> Seems you still don't address here, how do you sure 
> fobj->shared[fobj->shared_count] is null? if not NULL, the old one 
> will be leak.

I've put them at the end of the container, see above "k = 
fobj->shared_max". Since shared_max >> shared_count (at least twice as 
large in this situation) we should be on the save side.

Regards,
Christian.

>
> Regards,
> David Zhou
>>           fobj->shared_count++;
>> @@ -192,10 +198,20 @@ 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;
>> +
>> +    /* Drop the references to the signaled fences */
>> +    for (i = k; i < fobj->shared_max; ++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 v2

[Chunming Zhou]

On 2017年10月31日 16:55, Christian König wrote:
> Am 31.10.2017 um 09:51 schrieb Chunming Zhou:
>>
>>
>> On 2017年10月31日 16:43, Christian König wrote:
>>> 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.
>>>
>>> v2: temporary put the signaled fences at the end of the new container
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/dma-buf/reservation.c | 36 
>>> ++++++++++++++++++++++++++----------
>>>   1 file changed, 26 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/reservation.c 
>>> b/drivers/dma-buf/reservation.c
>>> index b44d9d7db347..6fc794576997 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 = fobj->shared_max; i < old->shared_count; 
>>> ++i) {
>>>           struct dma_fence *check;
>>>             check = rcu_dereference_protected(old->shared[i],
>>> @@ -172,10 +170,18 @@ 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 {
>>> +            /*
>>> +             * Temporary save the signaled fences at the end of the
>>> +             * new container
>>> +             */
>>> +            RCU_INIT_POINTER(fobj->shared[--k], check);
>>> +        }
>>>       }
>>> +    fobj->shared_count = j;
>>>       if (!old_fence) {
>>> RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
>> Seems you still don't address here, how do you sure 
>> fobj->shared[fobj->shared_count] is null? if not NULL, the old one 
>> will be leak.
>
> I've put them at the end of the container, see above "k = 
> fobj->shared_max". Since shared_max >> shared_count (at least twice as 
> large in this situation) we should be on the save side.
Ah, oops, Reviewed-by: Chunming Zhou <david1.zhou@amd.com> for the series.

Thanks,
David Zhou
>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>>           fobj->shared_count++;
>>> @@ -192,10 +198,20 @@ 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;
>>> +
>>> +    /* Drop the references to the signaled fences */
>>> +    for (i = k; i < fobj->shared_max; ++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 v2

[Sumit Semwal]

Hi Christian,

On 30 October 2017 at 20:29, Christian König
<ckoenig.leichtzumerken@gmail.com> 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.

I think this warrants some more context in the commit log. Could you
please do the same for the other patch as well?
>
> v2: temporary put the signaled fences at the end of the new container
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/reservation.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
>

Best,
Sumit.
_______________________________________________
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 v2

[Chris Wilson]

Quoting Christian König (2017-10-30 14:59:03)
> 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.
> 
> v2: temporary put the signaled fences at the end of the new container
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/reservation.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index b44d9d7db347..6fc794576997 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 = fobj->shared_max; i < old->shared_count; ++i) {
>                 struct dma_fence *check;
>  
>                 check = rcu_dereference_protected(old->shared[i],
> @@ -172,10 +170,18 @@ 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 {
> +                       /*
> +                        * Temporary save the signaled fences at the end of the
> +                        * new container
> +                        */
> +                       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 +198,20 @@ 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);

We don't need to keep old_fence (local var) around any more in this
scheme. By construction, the new shared_list is large enough to hold all
the old_fences+1, so you can use a loop like

j = 0;
k = fobj->shared_max;
RCU_INIT_POINTER(fobj->shared[j++], fence);
for (i = 0; i < old->shared_count; ++i) {
	struct dma_fence *check;

	check = rcu_dereference_protected(old->shared[i],
					  reservation_object_held(obj));

	if (check->context == fence->context ||
	    dma_fence_is_signaled(check))
		RCU_INIT_POINTER(fobj->shared[--k], check);
	else
		RCU_INIT_POINTER(fobj->shared[j++], check);
}
fobj->shared_count = j;

> +
> +       if (!old)
> +               return;
> +
> +       /* Drop the references to the signaled fences */
> +       for (i = k; i < fobj->shared_max; ++i) {
> +               struct dma_fence *f;
> +
> +               f = rcu_dereference_protected(fobj->shared[i],
> +                                             reservation_object_held(obj));
> +               dma_fence_put(f);
> +       }
> +       kfree_rcu(old, rcu);
>  }

Fwiw, this fixes a big problem with the lack of pruning of the
reservation_object, so \o/
-Chris
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx