dma-buf: keep only not signaled fence in reservation_object_add_shared_replace

From: Christian König <[email protected]>

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 <[email protected]>

---
  drivers/dma-buf/reservation.c | 31 +++++++++++++++++++++----------
  1 file changed, 21 insertions(+), 10 deletions(-)

drivers/dma-buf/reservation.c +2 -7

index b44d9d7db347..4ede77d1bb31 100644
@@ -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);

drivers/dma-buf/reservation.c +5 -3

  		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) {

Quoted 47 lines

From: Christian König <[email protected]>

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 <[email protected]>

---
  drivers/dma-buf/reservation.c | 31 +++++++++++++++++++++----------

  1 file changed, 21 insertions(+), 10 deletions(-)

drivers/dma-buf/reservation.c +1 -1

index b44d9d7db347..4ede77d1bb31 100644
@@ -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],

drivers/dma-buf/reservation.c +4 -3

@@ -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 {

drivers/dma-buf/reservation.c reservation_object_add_shared_replace(struct reservation_object *obj, · line 182

Quoted 6 lines

drivers/dma-buf/reservation.c +3 -0

+            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

Quoted 15 lines

drivers/dma-buf/reservation.c +6 -2

           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;

drivers/dma-buf/reservation.c reservation_object_add_shared_replace(struct reservation_object *obj, · line 212

Quoted 8 lines

drivers/dma-buf/reservation.c +6 -0

+
+        f = rcu_dereference_protected(fobj->shared[i],
+                          reservation_object_held(obj));
+        dma_fence_put(f);
+    }
+    kfree_rcu(old, rcu);
   }
     /**

Am 25.10.2017 um 08:42 schrieb Chunming Zhou:

Quoted 50 lines · Christian König

Quoted 47 lines

From: Christian König <[email protected]>

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 <[email protected]>

---
  drivers/dma-buf/reservation.c | 31 +++++++++++++++++++++----------

  1 file changed, 21 insertions(+), 10 deletions(-)

drivers/dma-buf/reservation.c +1 -1

index b44d9d7db347..4ede77d1bb31 100644
@@ -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],

drivers/dma-buf/reservation.c +4 -3

@@ -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 {

Quoted 8 lines

drivers/dma-buf/reservation.c reservation_object_add_shared_replace(struct reservation_object *obj, · line 182

Quoted 6 lines

drivers/dma-buf/reservation.c +3 -0

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

Quoted 27 lines

Regards,
David Zhou

Quoted 23 lines

drivers/dma-buf/reservation.c +12 -2

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

Any update?

Quoted 60 lines · Christian König

Am 25.10.2017 um 08:42 schrieb Chunming Zhou:

Quoted 50 lines · Christian König

Quoted 47 lines

From: Christian König <[email protected]>

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 <[email protected]>

---
  drivers/dma-buf/reservation.c | 31 +++++++++++++++++++++----------

  1 file changed, 21 insertions(+), 10 deletions(-)

drivers/dma-buf/reservation.c +1 -1

index b44d9d7db347..4ede77d1bb31 100644
@@ -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],

drivers/dma-buf/reservation.c +4 -3

@@ -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 {

Quoted 8 lines

drivers/dma-buf/reservation.c reservation_object_add_shared_replace(struct reservation_object *obj, · line 182

Quoted 6 lines

drivers/dma-buf/reservation.c +3 -0

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

Quoted 33 lines

Good point, thanks. Going to respin.

Regards,
Christian.

Quoted 27 lines

Regards,
David Zhou

Quoted 23 lines

drivers/dma-buf/reservation.c +12 -2

           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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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