[PATCH] drm/syncobj: Don't leak fences when WAIT_FOR_SUBMIT is set

[PATCH] drm/syncobj: Don't leak fences when WAIT_FOR_SUBMIT is set

[Jason Ekstrand]

We attempt to get fences earlier in the hopes that everything will
already have fences and no callbacks will be needed.  If we do succeed
in getting a fence, getting one a second time will result in a duplicate
ref with no unref.  This is causing memory leaks in Vulkan applications
that create a lot of fences; playing for a few hours can, apparently,
bring down the system.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107899
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_syncobj.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index adb3cb27d31e..759278fef35a 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -97,6 +97,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
 {
 	int ret;
 
+	WARN_ON(*fence);
+
 	*fence = drm_syncobj_fence_get(syncobj);
 	if (*fence)
 		return 1;
@@ -743,6 +745,9 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 
 	if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
 		for (i = 0; i < count; ++i) {
+			if (entries[i].fence)
+				continue;
+
 			drm_syncobj_fence_get_or_add_callback(syncobjs[i],
 							      &entries[i].fence,
 							      &entries[i].syncobj_cb,
-- 
2.17.1

Re: [PATCH] drm/syncobj: Don't leak fences when WAIT_FOR_SUBMIT is set

[Chris Wilson]

Quoting Jason Ekstrand (2018-09-26 08:17:03)
> We attempt to get fences earlier in the hopes that everything will
> already have fences and no callbacks will be needed.  If we do succeed
> in getting a fence, getting one a second time will result in a duplicate
> ref with no unref.  This is causing memory leaks in Vulkan applications
> that create a lot of fences; playing for a few hours can, apparently,
> bring down the system.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107899
> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_syncobj.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index adb3cb27d31e..759278fef35a 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -97,6 +97,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>  {
>         int ret;
>  
> +       WARN_ON(*fence);

I would have just put if (*fence) return; since the function is tied to
the array_wait implementation.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [PATCH] drm/syncobj: Don't leak fences when WAIT_FOR_SUBMIT is set

[Chris Wilson]

Quoting Jason Ekstrand (2018-09-26 08:17:03)
> We attempt to get fences earlier in the hopes that everything will
> already have fences and no callbacks will be needed.  If we do succeed
> in getting a fence, getting one a second time will result in a duplicate
> ref with no unref.  This is causing memory leaks in Vulkan applications
> that create a lot of fences; playing for a few hours can, apparently,
> bring down the system.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107899
> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_syncobj.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index adb3cb27d31e..759278fef35a 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -97,6 +97,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>  {
>         int ret;
>  
> +       WARN_ON(*fence);

I would have just put if (*fence) return; since the function is tied to
the array_wait implementation.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [PATCH] drm/syncobj: Don't leak fences when WAIT_FOR_SUBMIT is set

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 1446 bytes --]

On Wed, Sep 26, 2018 at 3:18 AM Chris Wilson <chris@chris-wilson.co.uk>
wrote:

> Quoting Jason Ekstrand (2018-09-26 08:17:03)
> > We attempt to get fences earlier in the hopes that everything will
> > already have fences and no callbacks will be needed.  If we do succeed
> > in getting a fence, getting one a second time will result in a duplicate
> > ref with no unref.  This is causing memory leaks in Vulkan applications
> > that create a lot of fences; playing for a few hours can, apparently,
> > bring down the system.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107899
> > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/drm_syncobj.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_syncobj.c
> b/drivers/gpu/drm/drm_syncobj.c
> > index adb3cb27d31e..759278fef35a 100644
> > --- a/drivers/gpu/drm/drm_syncobj.c
> > +++ b/drivers/gpu/drm/drm_syncobj.c
> > @@ -97,6 +97,8 @@ static int
> drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
> >  {
> >         int ret;
> >
> > +       WARN_ON(*fence);
>
> I would have just put if (*fence) return; since the function is tied to
> the array_wait implementation.
>

I considered doing that but marginally liked this better.  If you have a
preference, I'm happy to change itl.

--Jason


> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
>

[-- Attachment #1.2: Type: text/html, Size: 2332 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/syncobj: Don't leak fences when WAIT_FOR_SUBMIT is set

[Chris Wilson]

Quoting Jason Ekstrand (2018-09-26 10:43:59)
> On Wed, Sep 26, 2018 at 3:18 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
>     Quoting Jason Ekstrand (2018-09-26 08:17:03)
>     > We attempt to get fences earlier in the hopes that everything will
>     > already have fences and no callbacks will be needed.  If we do succeed
>     > in getting a fence, getting one a second time will result in a duplicate
>     > ref with no unref.  This is causing memory leaks in Vulkan applications
>     > that create a lot of fences; playing for a few hours can, apparently,
>     > bring down the system.
>     >
>     > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107899
>     > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
>     > Cc: stable@vger.kernel.org
>     > ---
>     >  drivers/gpu/drm/drm_syncobj.c | 5 +++++
>     >  1 file changed, 5 insertions(+)
>     >
>     > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/
>     drm_syncobj.c
>     > index adb3cb27d31e..759278fef35a 100644
>     > --- a/drivers/gpu/drm/drm_syncobj.c
>     > +++ b/drivers/gpu/drm/drm_syncobj.c
>     > @@ -97,6 +97,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct
>     drm_syncobj *syncobj,
>     >  {
>     >         int ret;
>     > 
>     > +       WARN_ON(*fence);
> 
>     I would have just put if (*fence) return; since the function is tied to
>     the array_wait implementation.
> 
> 
> I considered doing that but marginally liked this better.  If you have a
> preference, I'm happy to change itl.

Patch works, the difference is insignificant, land the patch and close
the bug.
-Chris

Re: [PATCH] drm/syncobj: Don't leak fences when WAIT_FOR_SUBMIT is set

[Sean Paul]

On Wed, Sep 26, 2018 at 02:17:03AM -0500, Jason Ekstrand wrote:
> We attempt to get fences earlier in the hopes that everything will
> already have fences and no callbacks will be needed.  If we do succeed
> in getting a fence, getting one a second time will result in a duplicate
> ref with no unref.  This is causing memory leaks in Vulkan applications
> that create a lot of fences; playing for a few hours can, apparently,
> bring down the system.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107899
> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>

Pushed to drm-misc-fixes, thanks!

Sean

> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_syncobj.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index adb3cb27d31e..759278fef35a 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -97,6 +97,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>  {
>  	int ret;
>  
> +	WARN_ON(*fence);
> +
>  	*fence = drm_syncobj_fence_get(syncobj);
>  	if (*fence)
>  		return 1;
> @@ -743,6 +745,9 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>  
>  	if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>  		for (i = 0; i < count; ++i) {
> +			if (entries[i].fence)
> +				continue;
> +
>  			drm_syncobj_fence_get_or_add_callback(syncobjs[i],
>  							      &entries[i].fence,
>  							      &entries[i].syncobj_cb,
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS