Skip to contentSkip to conversation

android: fix reference leak in sync_fence_create

PATCH In review Maarten Lankhorst 4 participants 11 messages Aug 14, 2014 – Sep 1, 2014 Staging

lore
Share & tools
lore

Maarten Lankhorst

author
#0 lore raw

PATCH 1/2 android: fix reference leak in sync_fence_create

According to the documentation sync_fence_create takes ownership of the point, not a reference on the point.

Signed-off-by Maarten Lankhorst <[email protected]>
Cc Colin Cross <[email protected]>
---
 drivers/staging/android/sync.c | 1 -
 1 file changed, 1 deletion(-)
drivers/staging/android/sync.c +0 -1
index e7b2e0234196..69139ce7420d 100644
@@ -199,7 +199,6 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
 	fence->num_fences = 1;
 	atomic_set(&fence->status, 1);
 
-	fence_get(&pt->base);
 	fence->cbs[0].sync_pt = &pt->base;
 	fence->cbs[0].fence = fence;
 	if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
Signature · 1 line
2.0.4
author
#1 lore raw

PATCH 2/2 android: add sync_fence_create_dma

This allows users of dma fences to create a android fence.

Signed-off-by Maarten Lankhorst <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Jesse Barnes <[email protected]>
---
 drivers/staging/android/sync.c | 24 ++++++++++++++++++++----
 drivers/staging/android/sync.h | 11 +++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)
drivers/staging/android/sync.c +20 -4
index 69139ce7420d..c9331250ac26 100644
@@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb)
 }
 
 /* TODO: implement a create which takes more that one sync_pt */
-struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+static struct sync_fence *sync_fence_create_noref(const char *name, struct fence *pt)
 {
 	struct sync_fence *fence;
 

@@ -199,16 +199,32 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
 	fence->num_fences = 1;
 	atomic_set(&fence->status, 1);
 
-	fence->cbs[0].sync_pt = &pt->base;
+	fence->cbs[0].sync_pt = pt;
 	fence->cbs[0].fence = fence;
-	if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
-			       fence_check_cb_func))
+	if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func))
 		atomic_dec(&fence->status);
 
 	sync_fence_debug_add(fence);
 
 	return fence;
 }
+
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt)
+{
+	struct sync_fence *fence;
+
+	fence = sync_fence_create_noref(name, fence_get(pt));
+	if (!fence)
+		fence_put(pt);
+
+	return fence;
+}
+EXPORT_SYMBOL(sync_fence_create_dma);
+
+struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+{
+	return sync_fence_create_noref(name, &pt->base);
+}
 EXPORT_SYMBOL(sync_fence_create);
 
 struct sync_fence *sync_fence_fdget(int fd)
drivers/staging/android/sync.h +11 -0
index 66b0f431f63e..7b3bf560790c 100644
@@ -254,6 +254,17 @@ void sync_pt_free(struct sync_pt *pt);
  */
 struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt);
 
+/**
+ * sync_fence_create_dma() - creates a sync fence from a dma fence
+ * @name:	name of fence to create
+ * @pt:		dma fence to add to the sync fence
+ *
+ * Creates a fence containg @pt.  Once this is called, the fence takes
+ * a reference on @pt, unlike sync_fence_create which doesn't add one.
+ */
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt);
+
+
 /*
  * API for sync_fence consumers
  */
Signature · 1 line
2.0.4
Reviewed-by on v1 2/2
#2 lore raw

Re: [PATCH 2/2] android: add sync_fence_create_dma

Quoted 33 lines · On Thu, 14 Aug 2014 11:54:52 +0200 Maarten Lankhorst…

This allows users of dma fences to create a android fence.

Signed-off-by Maarten Lankhorst <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Jesse Barnes <[email protected]>
---
 drivers/staging/android/sync.c | 24 ++++++++++++++++++++----
 drivers/staging/android/sync.h | 11 +++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)
drivers/staging/android/sync.c +3 -4
index 69139ce7420d..c9331250ac26 100644
@@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb)
 }
 
 /* TODO: implement a create which takes more that one sync_pt */
-struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+static struct sync_fence *sync_fence_create_noref(const char *name, struct fence *pt)
 {
 	struct sync_fence *fence;
 

@@ -199,16 +199,32 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
 	fence->num_fences = 1;
 	atomic_set(&fence->status, 1);
 
-	fence->cbs[0].sync_pt = &pt->base;
+	fence->cbs[0].sync_pt = pt;
 	fence->cbs[0].fence = fence;
-	if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
-			       fence_check_cb_func))
+	if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func))

drivers/staging/android/sync.c struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt) · line 214

Quoted 10 lines
drivers/staging/android/sync.c +4 -0
 		atomic_dec(&fence->status);
 
 	sync_fence_debug_add(fence);
 
 	return fence;
 }
+
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt)
+{
+	struct sync_fence *fence;

I ran into the same naming trouble in my implementation; I think I ended up with sfence for sync fence declarations.

Quoted 28 lines
drivers/staging/android/sync.c +13 -0
+
+	fence = sync_fence_create_noref(name, fence_get(pt));
+	if (!fence)
+		fence_put(pt);
+
+	return fence;
+}
+EXPORT_SYMBOL(sync_fence_create_dma);
+
+struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+{
+	return sync_fence_create_noref(name, &pt->base);
+}
 EXPORT_SYMBOL(sync_fence_create);
 
 struct sync_fence *sync_fence_fdget(int fd)
drivers/staging/android/sync.h +4 -0
index 66b0f431f63e..7b3bf560790c 100644
@@ -254,6 +254,17 @@ void sync_pt_free(struct sync_pt *pt);
  */
 struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt);
 
+/**
+ * sync_fence_create_dma() - creates a sync fence from a dma fence
+ * @name:	name of fence to create
+ * @pt:		dma fence to add to the sync fence

drivers/staging/android/sync.h void sync_pt_free(struct sync_pt *pt); · line 270

Quoted 10 lines
drivers/staging/android/sync.h +7 -0
+ *
+ * Creates a fence containg @pt.  Once this is called, the fence takes
+ * a reference on @pt, unlike sync_fence_create which doesn't add one.
+ */
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt);
+
+
 /*
  * API for sync_fence consumers
  */

Yeah, I've been using this, looks good.

Reviewed-by Jesse Barnes <[email protected]>
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
maintainer
#3 lore raw

Re: [PATCH 2/2] android: add sync_fence_create_dma

In reply to Maarten Lankhorst
Quoted 1 line · Maarten Lankhorst

This allows users of dma fences to create a android fence.

Who is going to use these functions? I need an in-kernel user before I can add new api calls.

thanks,

greg k-h

#4 lore raw

Re: [PATCH 2/2] android: add sync_fence_create_dma

Quoted 5 lines · On Fri, 15 Aug 2014 14:46:56 +0800 Greg Kroah-Hartman…
Quoted 1 line · Maarten Lankhorst

This allows users of dma fences to create a android fence.

Who is going to use these functions? I need an in-kernel user before I can add new api calls.

I have some code that uses them, but I need a userspace user to push my stuff. :)

I'm working on the latter bit too in Mesa, and we have demand from Android, so we should get some users shortly.

Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
author
#5 lore raw

Re: [PATCH 2/2] android: add sync_fence_create_dma

In reply to Greg Kroah-Hartman

Hey,

Quoted 5 lines · Greg Kroah-Hartman
Quoted 1 line · Maarten Lankhorst

This allows users of dma fences to create a android fence.

Who is going to use these functions? I need an in-kernel user before I can add new api calls.

So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync. Will you apply these patches?

~Maarten

#6 lore raw

Re: [PATCH 2/2] android: add sync_fence_create_dma

Quoted 11 lines · Maarten Lankhorst

Hey,

Quoted 5 lines · Greg Kroah-Hartman
Quoted 1 line · Maarten Lankhorst

This allows users of dma fences to create a android fence.

Who is going to use these functions? I need an in-kernel user before I can add new api calls.

So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync. Will you apply these patches?

Can you resend the patches? Fix the changelog of the first one to mention that it is a bugfix. Send a [patch 3/3] which uses the new functions in [patch 2/3].

regards,
dan carpenter

author
#7 lore raw

Re: [PATCH 2/2] android: add sync_fence_create_dma

Hey,

Op 28-08-14 om 13:57 schreef Dan Carpenter:

Quoted 5 lines · Maarten Lankhorst
Quoted 4 lines

Hey,

Quoted 1 line · Greg Kroah-Hartman

On Thu, Aug 14, 2014 at 11:54:52AM +0200, Maarten Lankhorst wrote:

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

This allows users of dma fences to create a android fence.

Who is going to use these functions? I need an in-kernel user before I can add new api calls.

So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync. Will you apply these patches?

Can you resend the patches? Fix the changelog of the first one to mention that it is a bugfix. Send a [patch 3/3] which uses the new functions in [patch 2/3].

The second patch will have to be applied without an in-kernel user because it will be used in the drm subsystem, by someone other than me. Their code is not ready yet, but will likely will be for the 3.18 merge window.

~Maarten

#8 lore raw

Re: [PATCH 2/2] android: add sync_fence_create_dma

Quoted 11 lines · Maarten Lankhorst

Hey,

Op 28-08-14 om 13:57 schreef Dan Carpenter:

Quoted 8 lines · Maarten Lankhorst
Quoted 7 lines

Hey,

Quoted 4 lines · Greg Kroah-Hartman
Quoted 1 line · Maarten Lankhorst

This allows users of dma fences to create a android fence.

Who is going to use these functions? I need an in-kernel user before I can add new api calls.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

So I found a in-kernel user and PATCH 1/2 fixes a mem-leak with android out of tree drivers, and android's in-kernel sw-sync. Will you apply these patches?

Can you resend the patches? Fix the changelog of the first one to mention that it is a bugfix. Send a [patch 3/3] which uses the new functions in [patch 2/3].

The second patch will have to be applied without an in-kernel user because it will be used in the drm subsystem, by someone other than me. Their code is not ready yet, but will likely will be for the 3.18 merge window.

Let's just wait until the user is ready. It might be easiest if they push your patch?

Anyway, please resend the first patch so we can apply that right away.

regards,
dan carpenter

#9 lore raw
In reply to Maarten Lankhorst
Quoted 3 lines · Maarten Lankhorst

According to the documentation sync_fence_create takes ownership of the point, not a reference on the point.

What are the user visible effects of this bug? I assume this is a real bug but judging solely based on your patch description, it sounds like you could just update the documentation instead of changing the code.

regards,
dan carpenter

author
#10 lore raw

Hey,

Op 18-08-14 om 14:57 schreef Dan Carpenter:

Quoted 8 lines · Maarten Lankhorst
Quoted 3 lines

According to the documentation sync_fence_create takes ownership of the point, not a reference on the point.

What are the user visible effects of this bug? I assume this is a real bug but judging solely based on your patch description, it sounds like you could just update the documentation instead of changing the code.

Small memory leak on every created android fence when you run out of tree android drivers.

But because it happens every frame (or possibly even more often) it's worth fixing.

~Maarten