Re: [RFC] glx: fix DRI2 memory leak
On Wed, Mar 25, 2009 at 10:21:51AM -0700, Jesse Barnes wrote:
In trying to track down the memory leak in 20704, I found that the DRI2
drawable destroy routine doesn't seem to get called when new drawables
are created and old ones destroyed (as in the glViewport case in the
bug).
Ok, sorry for taking so long on this. Here's the patch I'd like to push
to fix the problem - let me know if it works alright.
thanks,
Kristian
From 2f2538d20291ead2187087228a391dade1a0434f Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Kristian=20H=C3=B8gsberg?= k...@redhat.com
Date: Thu, 9 Apr 2009 13:16:37 -0400
Subject: [PATCH] glx: Fix drawable private leak on destroy
When a drawable goes away, we don't destroy the GLX drawable in full,
since it may be current for a context. This means that when the drawable
is destroyed in full later, the backend doesn't get a chance to
destroy resources associated with the drawable (the DRI2Drawable).
With this patch, we destroy the GLX drawable in full when it goes away
and then track down all contexts that reference it and NULL their
pointers.
---
glx/Makefile.am|1 -
glx/glxcmds.c | 44 --
glx/glxdrawable.h |5 ---
glx/glxdri.c |2 +
glx/glxdri2.c |2 +
glx/glxdriswrast.c |2 +
glx/glxext.c | 42 +
glx/glxext.h |1 +
glx/glxutil.c | 74
glx/glxutil.h |9 +-
10 files changed, 67 insertions(+), 115 deletions(-)
delete mode 100644 glx/glxutil.c
diff --git a/glx/Makefile.am b/glx/Makefile.am
index 2537db8..a23ae0a 100644
--- a/glx/Makefile.am
+++ b/glx/Makefile.am
@@ -80,7 +80,6 @@ libglx_la_SOURCES = \
glxscreens.c \
glxscreens.h \
glxserver.h \
-glxutil.c \
glxutil.h \
render2.c \
render2swap.c \
diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index e21f0f0..86e8dd8 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -193,18 +193,9 @@ validGlxDrawable(ClientPtr client, XID id, int type, int
access_mode,
void
__glXContextDestroy(__GLXcontext *context)
{
-if (!context-isDirect) {
- if (context-drawPriv)
- __glXUnrefDrawable(context-drawPriv);
- if (context-readPriv)
- __glXUnrefDrawable(context-readPriv);
- context-drawPriv = NULL;
- context-readPriv = NULL;
-}
__glXFlushContextCache();
}
-
static void __glXdirectContextDestroy(__GLXcontext *context)
{
__glXContextDestroy(context);
@@ -320,6 +311,8 @@ DoCreateContext(__GLXclientState *cl, GLXContextID gcId,
glxc-isDirect = isDirect;
glxc-renderMode = GL_RENDER;
+__glXAddToContextList(glxc);
+
return Success;
}
@@ -639,10 +632,6 @@ DoMakeCurrent(__GLXclientState *cl,
}
__glXFlushContextCache();
if (!prevglxc-isDirect) {
- if (prevglxc-drawPriv)
- __glXUnrefDrawable(prevglxc-drawPriv);
- if (prevglxc-readPriv)
- __glXUnrefDrawable(prevglxc-readPriv);
prevglxc-drawPriv = NULL;
prevglxc-readPriv = NULL;
}
@@ -662,8 +651,6 @@ DoMakeCurrent(__GLXclientState *cl,
}
glxc-isCurrent = GL_TRUE;
- __glXRefDrawable(glxc-drawPriv);
- __glXRefDrawable(glxc-readPriv);
}
if (prevglxc) {
@@ -1090,6 +1077,33 @@ int __glXDisp_GetFBConfigsSGIX(__GLXclientState *cl,
GLbyte *pc)
return DoGetFBConfigs(cl, req-screen);
}
+GLboolean
+__glXDrawableInit(__GLXdrawable *drawable,
+ __GLXscreen *screen, DrawablePtr pDraw, int type,
+ XID drawId, __GLXconfig *config)
+{
+drawable-pDraw = pDraw;
+drawable-type = type;
+drawable-drawId = drawId;
+drawable-config = config;
+drawable-eventMask = 0;
+
+return GL_TRUE;
+}
+
+void
+__glXDrawableRelease(__GLXdrawable *drawable)
+{
+ScreenPtr pScreen = drawable-pDraw-pScreen;
+
+switch (drawable-type) {
+case GLX_DRAWABLE_PIXMAP:
+case GLX_DRAWABLE_PBUFFER:
+ (*pScreen-DestroyPixmap)((PixmapPtr) drawable-pDraw);
+ break;
+}
+}
+
static int
DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig
*config,
DrawablePtr pDraw, XID glxDrawableId, int type)
diff --git a/glx/glxdrawable.h b/glx/glxdrawable.h
index b64ff35..0215b3b 100644
--- a/glx/glxdrawable.h
+++ b/glx/glxdrawable.h
@@ -67,11 +67,6 @@ struct __GLXdrawable {
*/
__GLXconfig *config;
-/*
-** reference count
-*/
-int refCount;
-
GLenum target;
GLenum format;
diff --git a/glx/glxdri.c b/glx/glxdri.c
index cc6d939..eeac7fc 100644
--- a/glx/glxdri.c
+++ b/glx/glxdri.c
@@ -238,6 +238,8 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
__glXleaveServer(GL_FALSE);
}
+__glXDrawableRelease(drawable);
+
xfree(private);
}
diff --git a/glx/glxdri2.c b/glx/glxdri2.c
index c896536..44c2337 100644
Re: [RFC] glx: fix DRI2 memory leak
On Mon, 2009-04-06 at 14:57 -0700, Jesse Barnes wrote: On Wed, 01 Apr 2009 17:46:08 +0200 Michel Dänzer mic...@daenzer.net wrote: The real problem was pointed out by Pierre Willenbrock: If glxPriv-pDraw is a pixmap, DrawableGone() destroys it, then later DRI2DestroyDrawable dereferences it... The patch below seems to work here - at least it hasn't crashed in a couple of hours, not sure about the leak yet. Note that unless I'm missing something, it might still be possible for this to occur with windows if the underlying DrawableRec is freed before this code is reached. Also, I don't really understand the logic behind clearing glxPriv-pDraw and -drawId here. In particular, I'm not sure DrawableGone will ever be called with glxPriv-refCount 1. If it won't, this change makes the assignments unreachable. And if it will, we'll again leak because the cleanup code won't be able to get to the underlying DrawableRec? So are you happy enough with this fix to push it, Michel? Memory usage is still high on Intel after the fix, but that may be due to bo caching, and this is an important fix... Feel free to push it. It's not really my fix, I just combined the fixes and suggestions from others. :) -- Earthling Michel Dänzer |http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Wed, 01 Apr 2009 17:46:08 +0200 Michel Dänzer mic...@daenzer.net wrote: The real problem was pointed out by Pierre Willenbrock: If glxPriv-pDraw is a pixmap, DrawableGone() destroys it, then later DRI2DestroyDrawable dereferences it... The patch below seems to work here - at least it hasn't crashed in a couple of hours, not sure about the leak yet. Note that unless I'm missing something, it might still be possible for this to occur with windows if the underlying DrawableRec is freed before this code is reached. Also, I don't really understand the logic behind clearing glxPriv-pDraw and -drawId here. In particular, I'm not sure DrawableGone will ever be called with glxPriv-refCount 1. If it won't, this change makes the assignments unreachable. And if it will, we'll again leak because the cleanup code won't be able to get to the underlying DrawableRec? So are you happy enough with this fix to push it, Michel? Memory usage is still high on Intel after the fix, but that may be due to bo caching, and this is an important fix... Thanks, -- Jesse Barnes, Intel Open Source Technology Center ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
2009/4/1 Michel Dänzer mic...@daenzer.net:
On Mon, 2009-03-30 at 13:04 +0800, Shuang He wrote:
Shuang He wrote:
Jesse Barnes wrote:
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Memory leak problem seems resolved, but I still see X crash with:
(gdb) bt
#0 0xb7fd5424 in __kernel_vsyscall ()
#1 0x03155660 in raise () from /lib/libc.so.6
#2 0x03157028 in abort () from /lib/libc.so.6
#3 0x031925bd in __libc_message () from /lib/libc.so.6
#4 0x031987e4 in malloc_printerr () from /lib/libc.so.6
#5 0x0319c441 in _int_realloc () from /lib/libc.so.6
#6 0x0319d176 in realloc () from /lib/libc.so.6
#7 0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
#8 0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
at privates.c:129
#9 0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c,
val=0x0)
at privates.c:193
#10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
#11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at
glxdri2.c:108
#12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at glxutil.c:58
#13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0, xid=12583326)
at glxext.c:131
#14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
at resource.c:561
#15 0xb7edffa6 in DoDestroyDrawable (cl=value optimized out,
glxdrawable=12583326, type=1) at glxcmds.c:1225
#16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
#17 0x080874cf in Dispatch () at dispatch.c:437
---Type return to continue, or q return to quit---
#18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access memory
at
address 0xbde
Just debug a bit, check out this series of calls in DRI2DestroyDrawable when
X
crashed:
in (*ds-DestroyBuffers)(pDraw, pPriv-buffers, pPriv-bufferCount);
Xfree: free(0x9eef330)
Xfree: free(0x9eeef20)
Xfree: free(0x9efdde0)
Xfree: free(0x9efce08)
Xfree: free(0x9eee8b0)
Xrealloc: ptr = 0x9efaf20
Xrealloc: amount = 384
Xfree: free(0x9efcd18)
Xfree: free(0x9ef8468)
Xrealloc: ptr = 0x9efa278
Xrealloc: amount = 384
Xfree: free(0x9efcd18)
Xfree: free(0x9ef9808)
Xfree: free(0x9eeef38)
Xfree: free(0x9efa648)
Xfree: free(0x9efd788)
in dixSetPrivate(pPixmap-devPrivates, dri2PixmapPrivateKey, NULL);
Xrealloc: ptr = 0x9efce08
Xrealloc: amount = 512
So dixSetPrivate is trying to realloc memory at 0x9efce08, which is alreay
freed in DetroyBuffers. So maybe we should also do this: [...]
The real problem was pointed out by Pierre Willenbrock: If
glxPriv-pDraw is a pixmap, DrawableGone() destroys it, then later
DRI2DestroyDrawable dereferences it... The patch below seems to work
here - at least it hasn't crashed in a couple of hours, not sure about
the leak yet.
Note that unless I'm missing something, it might still be possible for
this to occur with windows if the underlying DrawableRec is freed before
this code is reached.
Also, I don't really understand the logic behind clearing glxPriv-pDraw
and -drawId here. In particular, I'm not sure DrawableGone will ever be
called with glxPriv-refCount 1. If it won't, this change makes the
assignments unreachable. And if it will, we'll again leak because the
cleanup code won't be able to get to the underlying DrawableRec?
We reference the glxPriv when we make it current for a context. If
the drawable is destroyed while it's current for a context,
DrawableGone will be called with refcount at least 2. Clearing
glxPriv-pDraw is done in this case so that when the context the
drawable is current for is destroyed, we don't try to dereference the
pointer to the destroyed drawable. But yes, as you mention, then
there's a leak when the glxdri2.c destroy function can't get at the
pDraw to destroy the DRI2Drawable.
diff --git a/glx/glxext.c b/glx/glxext.c
index c882372..e74d00e 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -119,17 +119,25 @@ static int ContextGone(__GLXcontext* cx, XID id)
static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
{
ScreenPtr pScreen = glxPriv-pDraw-pScreen;
+ PixmapPtr pPixmap = NULL;
+ int refcount;
switch (glxPriv-type) {
case GLX_DRAWABLE_PIXMAP:
case GLX_DRAWABLE_PBUFFER:
-
Re: [RFC] glx: fix DRI2 memory leak
On Mon, 2009-03-30 at 13:04 +0800, Shuang He wrote:
Shuang He wrote:
Jesse Barnes wrote:
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Memory leak problem seems resolved, but I still see X crash with:
(gdb) bt
#0 0xb7fd5424 in __kernel_vsyscall ()
#1 0x03155660 in raise () from /lib/libc.so.6
#2 0x03157028 in abort () from /lib/libc.so.6
#3 0x031925bd in __libc_message () from /lib/libc.so.6
#4 0x031987e4 in malloc_printerr () from /lib/libc.so.6
#5 0x0319c441 in _int_realloc () from /lib/libc.so.6
#6 0x0319d176 in realloc () from /lib/libc.so.6
#7 0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
#8 0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
at privates.c:129
#9 0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c,
val=0x0)
at privates.c:193
#10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
#11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at
glxdri2.c:108
#12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at glxutil.c:58
#13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0, xid=12583326)
at glxext.c:131
#14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
at resource.c:561
#15 0xb7edffa6 in DoDestroyDrawable (cl=value optimized out,
glxdrawable=12583326, type=1) at glxcmds.c:1225
#16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
#17 0x080874cf in Dispatch () at dispatch.c:437
---Type return to continue, or q return to quit---
#18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access memory
at
address 0xbde
Just debug a bit, check out this series of calls in DRI2DestroyDrawable when X
crashed:
in (*ds-DestroyBuffers)(pDraw, pPriv-buffers, pPriv-bufferCount);
Xfree: free(0x9eef330)
Xfree: free(0x9eeef20)
Xfree: free(0x9efdde0)
Xfree: free(0x9efce08)
Xfree: free(0x9eee8b0)
Xrealloc: ptr = 0x9efaf20
Xrealloc: amount = 384
Xfree: free(0x9efcd18)
Xfree: free(0x9ef8468)
Xrealloc: ptr = 0x9efa278
Xrealloc: amount = 384
Xfree: free(0x9efcd18)
Xfree: free(0x9ef9808)
Xfree: free(0x9eeef38)
Xfree: free(0x9efa648)
Xfree: free(0x9efd788)
in dixSetPrivate(pPixmap-devPrivates, dri2PixmapPrivateKey, NULL);
Xrealloc: ptr = 0x9efce08
Xrealloc: amount = 512
So dixSetPrivate is trying to realloc memory at 0x9efce08, which is alreay
freed in DetroyBuffers. So maybe we should also do this: [...]
The real problem was pointed out by Pierre Willenbrock: If
glxPriv-pDraw is a pixmap, DrawableGone() destroys it, then later
DRI2DestroyDrawable dereferences it... The patch below seems to work
here - at least it hasn't crashed in a couple of hours, not sure about
the leak yet.
Note that unless I'm missing something, it might still be possible for
this to occur with windows if the underlying DrawableRec is freed before
this code is reached.
Also, I don't really understand the logic behind clearing glxPriv-pDraw
and -drawId here. In particular, I'm not sure DrawableGone will ever be
called with glxPriv-refCount 1. If it won't, this change makes the
assignments unreachable. And if it will, we'll again leak because the
cleanup code won't be able to get to the underlying DrawableRec?
diff --git a/glx/glxext.c b/glx/glxext.c
index c882372..e74d00e 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -119,17 +119,25 @@ static int ContextGone(__GLXcontext* cx, XID id)
static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
{
ScreenPtr pScreen = glxPriv-pDraw-pScreen;
+PixmapPtr pPixmap = NULL;
+int refcount;
switch (glxPriv-type) {
case GLX_DRAWABLE_PIXMAP:
case GLX_DRAWABLE_PBUFFER:
- (*pScreen-DestroyPixmap)((PixmapPtr) glxPriv-pDraw);
+ pPixmap = (PixmapPtr) glxPriv-pDraw;
break;
}
-glxPriv-pDraw = NULL;
-glxPriv-drawId = 0;
+refcount = glxPriv-refCount;
__glXUnrefDrawable(glxPriv);
+if (refcount 1) {
+ glxPriv-pDraw = NULL;
+ glxPriv-drawId = 0;
+}
+
+if (pPixmap)
+ (*pScreen-DestroyPixmap)(pPixmap);
return True;
}
--
Earthling Michel Dänzer |http://www.vmware.com
Libre software enthusiast |
Re: [RFC] glx: fix DRI2 memory leak
Michel Dänzer wrote:
On Mon, 2009-03-30 at 13:04 +0800, Shuang He wrote:
Shuang He wrote:
Jesse Barnes wrote:
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Memory leak problem seems resolved, but I still see X crash with:
(gdb) bt
#0 0xb7fd5424 in __kernel_vsyscall ()
#1 0x03155660 in raise () from /lib/libc.so.6
#2 0x03157028 in abort () from /lib/libc.so.6
#3 0x031925bd in __libc_message () from /lib/libc.so.6
#4 0x031987e4 in malloc_printerr () from /lib/libc.so.6
#5 0x0319c441 in _int_realloc () from /lib/libc.so.6
#6 0x0319d176 in realloc () from /lib/libc.so.6
#7 0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
#8 0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
at privates.c:129
#9 0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c, val=0x0)
at privates.c:193
#10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
#11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at glxdri2.c:108
#12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at glxutil.c:58
#13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0, xid=12583326)
at glxext.c:131
#14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
at resource.c:561
#15 0xb7edffa6 in DoDestroyDrawable (cl=value optimized out,
glxdrawable=12583326, type=1) at glxcmds.c:1225
#16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
#17 0x080874cf in Dispatch () at dispatch.c:437
---Type return to continue, or q return to quit---
#18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access memory at
address 0xbde
Just debug a bit, check out this series of calls in DRI2DestroyDrawable when X
crashed:
in (*ds-DestroyBuffers)(pDraw, pPriv-buffers, pPriv-bufferCount);
Xfree: free(0x9eef330)
Xfree: free(0x9eeef20)
Xfree: free(0x9efdde0)
Xfree: free(0x9efce08)
Xfree: free(0x9eee8b0)
Xrealloc: ptr = 0x9efaf20
Xrealloc: amount = 384
Xfree: free(0x9efcd18)
Xfree: free(0x9ef8468)
Xrealloc: ptr = 0x9efa278
Xrealloc: amount = 384
Xfree: free(0x9efcd18)
Xfree: free(0x9ef9808)
Xfree: free(0x9eeef38)
Xfree: free(0x9efa648)
Xfree: free(0x9efd788)
in dixSetPrivate(pPixmap-devPrivates, dri2PixmapPrivateKey, NULL);
Xrealloc: ptr = 0x9efce08
Xrealloc: amount = 512
So dixSetPrivate is trying to realloc memory at 0x9efce08, which is alreay
freed in DetroyBuffers. So maybe we should also do this: [...]
The real problem was pointed out by Pierre Willenbrock: If
glxPriv-pDraw is a pixmap, DrawableGone() destroys it, then later
DRI2DestroyDrawable dereferences it... The patch below seems to work
here - at least it hasn't crashed in a couple of hours, not sure about
the leak yet.
Note that unless I'm missing something, it might still be possible for
this to occur with windows if the underlying DrawableRec is freed before
this code is reached.
Also, I don't really understand the logic behind clearing glxPriv-pDraw
and -drawId here. In particular, I'm not sure DrawableGone will ever be
called with glxPriv-refCount 1. If it won't, this change makes the
assignments unreachable. And if it will, we'll again leak because the
cleanup code won't be able to get to the underlying DrawableRec?
diff --git a/glx/glxext.c b/glx/glxext.c
index c882372..e74d00e 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -119,17 +119,25 @@ static int ContextGone(__GLXcontext* cx, XID id)
static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
{
ScreenPtr pScreen = glxPriv-pDraw-pScreen;
+PixmapPtr pPixmap = NULL;
+int refcount;
switch (glxPriv-type) {
case GLX_DRAWABLE_PIXMAP:
case GLX_DRAWABLE_PBUFFER:
- (*pScreen-DestroyPixmap)((PixmapPtr) glxPriv-pDraw);
+ pPixmap = (PixmapPtr) glxPriv-pDraw;
break;
}
-glxPriv-pDraw = NULL;
-glxPriv-drawId = 0;
+refcount = glxPriv-refCount;
__glXUnrefDrawable(glxPriv);
+if (refcount 1) {
+ glxPriv-pDraw = NULL;
+ glxPriv-drawId = 0;
+}
+
+if (pPixmap)
+ (*pScreen-DestroyPixmap)(pPixmap);
return True;
}
This works for me. I don't see the memory leaks or X crashes.
Thanks
--Shuang
___
xorg mailing list
xorg@lists.freedesktop.org
Re: [RFC] glx: fix DRI2 memory leak
On Monday 30 March 2009, Shuang He wrote:
Jesse Barnes wrote:
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
jbar...@virtuousgeek.org wrote:
Ok, I think this is where the leak was:
__glXUnrefDrawable-DrawableGone-__glXUnrefDrawable. This sequence
meant the glxPriv would stay around (since it was used), but
DrawableGone had generally already disposed of the pDrawable before
the call to Unref, so we never got into DRI2DestroyBuffers, thus
the leak.
The loop seems broken to me. It *looks* like DrawableGone should be
the real free routine, only called when a drawable really is free,
so I've fixed up the code to conform to that. This means removing
the GLX priv frees from DRI and DRI2 routines and putting them here
and using the GLX unref routine everwhere (since it will only free
the resource when the refcount reaches 0).
Any thoughts? I'd appreciate some more testers too... I'm not
quite sure about the generic DoDestroyDrawable change, but if the
refcount is always 1 there anyway it shouldn't make a difference
and seems more correct.
The __GLXdrawable is a reference counted object, and the glx code
references it in a couple of places: when there's an X resource
pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
the current drawable or readable for a context. The __GLXdrawable is
allocated by the backend in use (dri, dri2 or swrast) and must be
freed by the same backend (don't mix alloc and free across abstraction
barriers). We unref the __GLXdrawable when we either set a different
drawable/readable and when the X resource goes away.
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Thanks,
Memory leak problem seems resolved, but I still see X crash with:
(gdb) bt
#0 0xb7fd5424 in __kernel_vsyscall ()
#1 0x03155660 in raise () from /lib/libc.so.6
#2 0x03157028 in abort () from /lib/libc.so.6
#3 0x031925bd in __libc_message () from /lib/libc.so.6
#4 0x031987e4 in malloc_printerr () from /lib/libc.so.6
#5 0x0319c441 in _int_realloc () from /lib/libc.so.6
#6 0x0319d176 in realloc () from /lib/libc.so.6
#7 0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
#8 0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
at privates.c:129
#9 0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c,
val=0x0) at privates.c:193
#10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
#11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at
glxdri2.c:108 #12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at
glxutil.c:58 #13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0,
xid=12583326) at glxext.c:131
#14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
at resource.c:561
#15 0xb7edffa6 in DoDestroyDrawable (cl=value optimized out,
glxdrawable=12583326, type=1) at glxcmds.c:1225
#16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
#17 0x080874cf in Dispatch () at dispatch.c:437
---Type return to continue, or q return to quit---
#18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access
memory at address 0xbde
Thanks
--Shuang
) at main.c:397
I have observed this crash once as well, but haven't yet found a way to
reproduce it. Can you post a few steps that make this crash more likely? On
the bug report (http://bugs.freedesktop.org/show_bug.cgi?id=20704) you
mention it has something to do with resizing by small amounts.
Thanks
Magnus
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
Magnus Kessler wrote:
On Monday 30 March 2009, Shuang He wrote:
Jesse Barnes wrote:
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
jbar...@virtuousgeek.org wrote:
Ok, I think this is where the leak was:
__glXUnrefDrawable-DrawableGone-__glXUnrefDrawable. This sequence
meant the glxPriv would stay around (since it was used), but
DrawableGone had generally already disposed of the pDrawable before
the call to Unref, so we never got into DRI2DestroyBuffers, thus
the leak.
The loop seems broken to me. It *looks* like DrawableGone should be
the real free routine, only called when a drawable really is free,
so I've fixed up the code to conform to that. This means removing
the GLX priv frees from DRI and DRI2 routines and putting them here
and using the GLX unref routine everwhere (since it will only free
the resource when the refcount reaches 0).
Any thoughts? I'd appreciate some more testers too... I'm not
quite sure about the generic DoDestroyDrawable change, but if the
refcount is always 1 there anyway it shouldn't make a difference
and seems more correct.
The __GLXdrawable is a reference counted object, and the glx code
references it in a couple of places: when there's an X resource
pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
the current drawable or readable for a context. The __GLXdrawable is
allocated by the backend in use (dri, dri2 or swrast) and must be
freed by the same backend (don't mix alloc and free across abstraction
barriers). We unref the __GLXdrawable when we either set a different
drawable/readable and when the X resource goes away.
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Thanks,
Memory leak problem seems resolved, but I still see X crash with:
(gdb) bt
#0 0xb7fd5424 in __kernel_vsyscall ()
#1 0x03155660 in raise () from /lib/libc.so.6
#2 0x03157028 in abort () from /lib/libc.so.6
#3 0x031925bd in __libc_message () from /lib/libc.so.6
#4 0x031987e4 in malloc_printerr () from /lib/libc.so.6
#5 0x0319c441 in _int_realloc () from /lib/libc.so.6
#6 0x0319d176 in realloc () from /lib/libc.so.6
#7 0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
#8 0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
at privates.c:129
#9 0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c,
val=0x0) at privates.c:193
#10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
#11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at
glxdri2.c:108 #12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at
glxutil.c:58 #13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0,
xid=12583326) at glxext.c:131
#14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
at resource.c:561
#15 0xb7edffa6 in DoDestroyDrawable (cl=value optimized out,
glxdrawable=12583326, type=1) at glxcmds.c:1225
#16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
#17 0x080874cf in Dispatch () at dispatch.c:437
---Type return to continue, or q return to quit---
#18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access
memory at address 0xbde
Thanks
--Shuang
) at main.c:397
I have observed this crash once as well, but haven't yet found a way to
reproduce it. Can you post a few steps that make this crash more likely? On
the bug report (http://bugs.freedesktop.org/show_bug.cgi?id=20704) you
mention it has something to do with resizing by small amounts.
Thanks
Magnus
I could easily reproduce this by just resizing the window several
times. Most of times, it's just costing 1 or 2 minutes.
Thanks
--Shuang
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
Jesse Barnes wrote:
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
jbar...@virtuousgeek.org wrote:
Ok, I think this is where the leak was:
__glXUnrefDrawable-DrawableGone-__glXUnrefDrawable. This sequence
meant the glxPriv would stay around (since it was used), but
DrawableGone had generally already disposed of the pDrawable before
the call to Unref, so we never got into DRI2DestroyBuffers, thus
the leak.
The loop seems broken to me. It *looks* like DrawableGone should be
the real free routine, only called when a drawable really is free,
so I've fixed up the code to conform to that. This means removing
the GLX priv frees from DRI and DRI2 routines and putting them here
and using the GLX unref routine everwhere (since it will only free
the resource when the refcount reaches 0).
Any thoughts? I'd appreciate some more testers too... I'm not
quite sure about the generic DoDestroyDrawable change, but if the
refcount is always 1 there anyway it shouldn't make a difference
and seems more correct.
The __GLXdrawable is a reference counted object, and the glx code
references it in a couple of places: when there's an X resource
pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
the current drawable or readable for a context. The __GLXdrawable is
allocated by the backend in use (dri, dri2 or swrast) and must be
freed by the same backend (don't mix alloc and free across abstraction
barriers). We unref the __GLXdrawable when we either set a different
drawable/readable and when the X resource goes away.
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Thanks,
Memory leak problem seems resolved, but I still see X crash with:
(gdb) bt
#0 0xb7fd5424 in __kernel_vsyscall ()
#1 0x03155660 in raise () from /lib/libc.so.6
#2 0x03157028 in abort () from /lib/libc.so.6
#3 0x031925bd in __libc_message () from /lib/libc.so.6
#4 0x031987e4 in malloc_printerr () from /lib/libc.so.6
#5 0x0319c441 in _int_realloc () from /lib/libc.so.6
#6 0x0319d176 in realloc () from /lib/libc.so.6
#7 0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
#8 0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
at privates.c:129
#9 0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c, val=0x0)
at privates.c:193
#10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
#11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at glxdri2.c:108
#12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at glxutil.c:58
#13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0, xid=12583326)
at glxext.c:131
#14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
at resource.c:561
#15 0xb7edffa6 in DoDestroyDrawable (cl=value optimized out,
glxdrawable=12583326, type=1) at glxcmds.c:1225
#16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
#17 0x080874cf in Dispatch () at dispatch.c:437
---Type return to continue, or q return to quit---
#18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access memory at
address 0xbde
Thanks
--Shuang
) at main.c:397
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
Shuang He wrote:
Jesse Barnes wrote:
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
jbar...@virtuousgeek.org wrote:
Ok, I think this is where the leak was:
__glXUnrefDrawable-DrawableGone-__glXUnrefDrawable. This sequence
meant the glxPriv would stay around (since it was used), but
DrawableGone had generally already disposed of the pDrawable before
the call to Unref, so we never got into DRI2DestroyBuffers, thus
the leak.
The loop seems broken to me. It *looks* like DrawableGone should be
the real free routine, only called when a drawable really is free,
so I've fixed up the code to conform to that. This means removing
the GLX priv frees from DRI and DRI2 routines and putting them here
and using the GLX unref routine everwhere (since it will only free
the resource when the refcount reaches 0).
Any thoughts? I'd appreciate some more testers too... I'm not
quite sure about the generic DoDestroyDrawable change, but if the
refcount is always 1 there anyway it shouldn't make a difference
and seems more correct.
The __GLXdrawable is a reference counted object, and the glx code
references it in a couple of places: when there's an X resource
pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
the current drawable or readable for a context. The __GLXdrawable is
allocated by the backend in use (dri, dri2 or swrast) and must be
freed by the same backend (don't mix alloc and free across abstraction
barriers). We unref the __GLXdrawable when we either set a different
drawable/readable and when the X resource goes away.
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Thanks,
Memory leak problem seems resolved, but I still see X crash with:
(gdb) bt
#0 0xb7fd5424 in __kernel_vsyscall ()
#1 0x03155660 in raise () from /lib/libc.so.6
#2 0x03157028 in abort () from /lib/libc.so.6
#3 0x031925bd in __libc_message () from /lib/libc.so.6
#4 0x031987e4 in malloc_printerr () from /lib/libc.so.6
#5 0x0319c441 in _int_realloc () from /lib/libc.so.6
#6 0x0319d176 in realloc () from /lib/libc.so.6
#7 0x08131002 in Xrealloc (ptr=0x6, amount=0) at utils.c:1133
#8 0x0806d10b in dixAllocatePrivate (privates=0x91487c8, key=0xb7e90a3c)
at privates.c:129
#9 0x0806d1cc in dixSetPrivate (privates=0x91487c8, key=0xb7e90a3c, val=0x0)
at privates.c:193
#10 0xb7e8eca1 in DRI2DestroyDrawable (pDraw=0x91487b0) at dri2.c:218
#11 0xb7eee668 in __glXDRIdrawableDestroy (drawable=0x9205ff0) at glxdri2.c:108
#12 0xb7ee49bb in __glXUnrefDrawable (glxPriv=0x9205ff0) at glxutil.c:58
#13 0xb7ee3183 in DrawableGone (glxPriv=0x9205ff0, xid=12583326)
at glxext.c:131
#14 0x0806efdc in FreeResource (id=12583326, skipDeleteFuncType=0)
at resource.c:561
#15 0xb7edffa6 in DoDestroyDrawable (cl=value optimized out,
glxdrawable=12583326, type=1) at glxcmds.c:1225
#16 0xb7ee340a in __glXDispatch (client=0x8d79db8) at glxext.c:523
#17 0x080874cf in Dispatch () at dispatch.c:437
---Type return to continue, or q return to quit---
#18 0x0806c69d in main (argc=2, argv=0xbf9d2754, envp=Cannot access memory at
address 0xbde
Thanks
--Shuang
) at main.c:397
Just debug a bit, check out this series of calls in DRI2DestroyDrawable when X
crashed:
in (*ds-DestroyBuffers)(pDraw, pPriv-buffers, pPriv-bufferCount);
Xfree: free(0x9eef330)
Xfree: free(0x9eeef20)
Xfree: free(0x9efdde0)
Xfree: free(0x9efce08)
Xfree: free(0x9eee8b0)
Xrealloc: ptr = 0x9efaf20
Xrealloc: amount = 384
Xfree: free(0x9efcd18)
Xfree: free(0x9ef8468)
Xrealloc: ptr = 0x9efa278
Xrealloc: amount = 384
Xfree: free(0x9efcd18)
Xfree: free(0x9ef9808)
Xfree: free(0x9eeef38)
Xfree: free(0x9efa648)
Xfree: free(0x9efd788)
in dixSetPrivate(pPixmap-devPrivates, dri2PixmapPrivateKey, NULL);
Xrealloc: ptr = 0x9efce08
Xrealloc: amount = 512
So dixSetPrivate is trying to realloc memory at 0x9efce08, which is alreay
freed in DetroyBuffers. So maybe we should also do this:
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 0f2e24b..dddcfdc 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -204,9 +204,6 @@ DRI2DestroyDrawable(DrawablePtr pDraw)
if (pPriv-refCount 0)
return;
-(*ds-DestroyBuffers)(pDraw, pPriv-buffers, pPriv-bufferCount);
-xfree(pPriv);
-
if (pDraw-type == DRAWABLE_WINDOW)
{
pWin = (WindowPtr) pDraw;
@@ -217,6 +214,9 @@
Re: [RFC] glx: fix DRI2 memory leak
Jesse,
Thanks for the patch, I'm testing it now - it looks promising :)
X memory usage is more stable, but lsof | grep drm mm | wc -l still
increases - after 10 minutes it went up from 100 to 500... Is this
normal?
cheers,
Adam
2009/3/28 Jesse Barnes jbar...@virtuousgeek.org:
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
jbar...@virtuousgeek.org wrote:
Ok, I think this is where the leak was:
__glXUnrefDrawable-DrawableGone-__glXUnrefDrawable. This sequence
meant the glxPriv would stay around (since it was used), but
DrawableGone had generally already disposed of the pDrawable before
the call to Unref, so we never got into DRI2DestroyBuffers, thus
the leak.
The loop seems broken to me. It *looks* like DrawableGone should be
the real free routine, only called when a drawable really is free,
so I've fixed up the code to conform to that. This means removing
the GLX priv frees from DRI and DRI2 routines and putting them here
and using the GLX unref routine everwhere (since it will only free
the resource when the refcount reaches 0).
Any thoughts? I'd appreciate some more testers too... I'm not
quite sure about the generic DoDestroyDrawable change, but if the
refcount is always 1 there anyway it shouldn't make a difference
and seems more correct.
The __GLXdrawable is a reference counted object, and the glx code
references it in a couple of places: when there's an X resource
pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
the current drawable or readable for a context. The __GLXdrawable is
allocated by the backend in use (dri, dri2 or swrast) and must be
freed by the same backend (don't mix alloc and free across abstraction
barriers). We unref the __GLXdrawable when we either set a different
drawable/readable and when the X resource goes away.
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
diff --git a/glx/glxext.c b/glx/glxext.c
index c882372..fec45b7 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -119,6 +119,7 @@ static int ContextGone(__GLXcontext* cx, XID id)
static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
{
ScreenPtr pScreen = glxPriv-pDraw-pScreen;
+ int refcount;
switch (glxPriv-type) {
case GLX_DRAWABLE_PIXMAP:
@@ -127,9 +128,12 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
break;
}
- glxPriv-pDraw = NULL;
- glxPriv-drawId = 0;
+ refcount = glxPriv-refCount;
__glXUnrefDrawable(glxPriv);
+ if (refcount 1) {
+ glxPriv-pDraw = NULL;
+ glxPriv-drawId = 0;
+ }
return True;
}
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Saturday 28 March 2009 02:42:53 Jesse Barnes wrote: Yep, that seems to work too... Magnus or Vasily can you guys confirm? I've just got xorg-server crash. Here's trace (not very usefull I think): Backtrace: 0: /usr/bin/X(xorg_backtrace+0x3c) [0x813344c] 1: /usr/bin/X(xf86SigHandler+0x52) [0x80d47b2] 2: [0xb80c6400] 3: /usr/lib/xorg/modules/extensions//libdri2.so [0xb7b5f89f] 4: /usr/bin/X(Dispatch+0x33f) [0x808b48f] 5: /usr/bin/X(main+0x3bd) [0x80700fd] 6: /lib/libc.so.6(__libc_start_main+0xe5) [0xb7bd4725] 7: /usr/bin/X [0x806f581] Fatal server error: Caught signal 11. Server aborting signature.asc Description: This is a digitally signed message part. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Saturday 28 Mar 2009 00:42:53 Jesse Barnes wrote: Yep, that seems to work too... Magnus or Vasily can you guys confirm? Seems to work fine for me, although I'm going to have an eye open for bugs that might have previously been hidden in the now exposed code paths. Is there any good documentation about resource handling in the server available? I must admit that I get slightly nervous about the loops of function calls that occur when resources are freed. Magnus ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Sat, 28 Mar 2009 11:35:20 +0100 Adam Lantos h...@playma.org wrote: Jesse, Thanks for the patch, I'm testing it now - it looks promising :) X memory usage is more stable, but lsof | grep drm mm | wc -l still increases - after 10 minutes it went up from 100 to 500... Is this normal? Yeah that's probably due to our bo cache. We could probably make it smarter, but right now it'll continue to grow (theoretically more and more slowly over time). -- Jesse Barnes, Intel Open Source Technology Center ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
jbar...@virtuousgeek.org wrote:
Ok, I think this is where the leak was:
__glXUnrefDrawable-DrawableGone-__glXUnrefDrawable. This sequence
meant the glxPriv would stay around (since it was used), but
DrawableGone had generally already disposed of the pDrawable before
the call to Unref, so we never got into DRI2DestroyBuffers, thus
the leak.
The loop seems broken to me. It *looks* like DrawableGone should be
the real free routine, only called when a drawable really is free,
so I've fixed up the code to conform to that. This means removing
the GLX priv frees from DRI and DRI2 routines and putting them here
and using the GLX unref routine everwhere (since it will only free
the resource when the refcount reaches 0).
Any thoughts? I'd appreciate some more testers too... I'm not
quite sure about the generic DoDestroyDrawable change, but if the
refcount is always 1 there anyway it shouldn't make a difference
and seems more correct.
The __GLXdrawable is a reference counted object, and the glx code
references it in a couple of places: when there's an X resource
pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
the current drawable or readable for a context. The __GLXdrawable is
allocated by the backend in use (dri, dri2 or swrast) and must be
freed by the same backend (don't mix alloc and free across abstraction
barriers). We unref the __GLXdrawable when we either set a different
drawable/readable and when the X resource goes away.
Yeah it does violate the layering... but the uref-drawablegone-unref
is a bit convoluted too.
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Did you try that?
Ah no I think I had either = 1 or == 1 which clearly wouldn't work
since the glxPriv would be gone in the == 1 case. I'll try the 1 test.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Fri, 27 Mar 2009 13:20:25 -0400
Kristian Høgsberg k...@bitplanet.net wrote:
On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
jbar...@virtuousgeek.org wrote:
Ok, I think this is where the leak was:
__glXUnrefDrawable-DrawableGone-__glXUnrefDrawable. This sequence
meant the glxPriv would stay around (since it was used), but
DrawableGone had generally already disposed of the pDrawable before
the call to Unref, so we never got into DRI2DestroyBuffers, thus
the leak.
The loop seems broken to me. It *looks* like DrawableGone should be
the real free routine, only called when a drawable really is free,
so I've fixed up the code to conform to that. This means removing
the GLX priv frees from DRI and DRI2 routines and putting them here
and using the GLX unref routine everwhere (since it will only free
the resource when the refcount reaches 0).
Any thoughts? I'd appreciate some more testers too... I'm not
quite sure about the generic DoDestroyDrawable change, but if the
refcount is always 1 there anyway it shouldn't make a difference
and seems more correct.
The __GLXdrawable is a reference counted object, and the glx code
references it in a couple of places: when there's an X resource
pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's
the current drawable or readable for a context. The __GLXdrawable is
allocated by the backend in use (dri, dri2 or swrast) and must be
freed by the same backend (don't mix alloc and free across abstraction
barriers). We unref the __GLXdrawable when we either set a different
drawable/readable and when the X resource goes away.
The leak is (as you pointed out before) because we NULL the pDraw
pointer before calling the backend destructor, which then can't unref
the DRI2Drawable, which we then leak. You had the right fix in the
originial post, we just need to not touch glxPriv after
__glXUnrefDrawable if it got destroyed. I suggest this change to
DrawableGone in irc:
refcount = glxPriv-refcount;
__glXUnrefDrawable(glxPriv);
if (refcount 1) {
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
}
Yep, that seems to work too... Magnus or Vasily can you guys confirm?
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
diff --git a/glx/glxext.c b/glx/glxext.c
index c882372..fec45b7 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -119,6 +119,7 @@ static int ContextGone(__GLXcontext* cx, XID id)
static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
{
ScreenPtr pScreen = glxPriv-pDraw-pScreen;
+int refcount;
switch (glxPriv-type) {
case GLX_DRAWABLE_PIXMAP:
@@ -127,9 +128,12 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
break;
}
-glxPriv-pDraw = NULL;
-glxPriv-drawId = 0;
+refcount = glxPriv-refCount;
__glXUnrefDrawable(glxPriv);
+if (refcount 1) {
+ glxPriv-pDraw = NULL;
+ glxPriv-drawId = 0;
+}
return True;
}
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Wednesday 25 March 2009, Jesse Barnes wrote: In trying to track down the memory leak in 20704, I found that the DRI2 drawable destroy routine doesn't seem to get called when new drawables are created and old ones destroyed (as in the glViewport case in the bug). The GLX core code takes care of destroying drawables correctly though, and even calls DestroyPixmap at the right time. However, DRI2 drawables have more state associated with them than just a single pixmap, so we have a __glXDRIdrawableDestroy function that takes care of that. However, by the time we get there, the GLX drawable is gone, so I never saw the if (drawable-pDraw != NULL) DRI2DestroyDrawable(drawable-pDraw); case get triggered... The simple patch below fixed that, but apparently isn't correct (see the bug report) since it causes crashes after a time. My patch was missing another change to set the private-count correctly in dri2GetBuffers though, which may be part of the fix as well. Has anyone else seen this leak? Anyone care to educate me a bit more about GLX drawable lifetime rules? Thanks, Jesse Hi Jesse, This memory leak has troubled me for some time, as it forces me to log out of my X sessions on a regular basis. I have tested your patch and it seems to help tremendously. However, digging through the git commits for glxext.c it appears that the line setting glxPriv-pDraw to NULL was added as part of commit http://cgit.freedesktop.org/xorg/xserver/commit/?id=30bcaa966d6b00f1630609a78db18dee683cc43d which aims to Make glx destroy path handle cases where the X window goes away first. So it might be that the commenter on bug 20704 who observes a crash hits this condition. Maybe Kristian Høgsberg can comment some more. Thanks, Magnus diff --git a/glx/glxext.c b/glx/glxext.c index c882372..73e5a9b 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -127,9 +127,9 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) break; } -glxPriv-pDraw = NULL; glxPriv-drawId = 0; __glXUnrefDrawable(glxPriv); +glxPriv-pDraw = NULL; return True; } ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg signature.asc Description: This is a digitally signed message part. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Wednesday 25 March 2009 19:21:51 Jesse Barnes wrote: Has anyone else seen this leak? Anyone care to educate me a bit more about GLX drawable lifetime rules? Yep, my system suffer from this leak too. Thanks, Jesse diff --git a/glx/glxext.c b/glx/glxext.c index c882372..73e5a9b 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -127,9 +127,9 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) break; } -glxPriv-pDraw = NULL; glxPriv-drawId = 0; __glXUnrefDrawable(glxPriv); Maybe, we should somehow check if glxPriv is still valid after Unref before assigning something to the pDraw? (I'm not sure, I'm not familiar with X internals) +glxPriv-pDraw = NULL; return True; } ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg signature.asc Description: This is a digitally signed message part. ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Thu, 26 Mar 2009 22:02:52 +0200 Vasily Khoruzhick anars...@gmail.com wrote: On Wednesday 25 March 2009 19:21:51 Jesse Barnes wrote: Has anyone else seen this leak? Anyone care to educate me a bit more about GLX drawable lifetime rules? Yep, my system suffer from this leak too. Yeah sounds pretty widespread; it'll only get worse as the radeon DRI2 bits become more common. diff --git a/glx/glxext.c b/glx/glxext.c index c882372..73e5a9b 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -127,9 +127,9 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) break; } -glxPriv-pDraw = NULL; glxPriv-drawId = 0; __glXUnrefDrawable(glxPriv); Maybe, we should somehow check if glxPriv is still valid after Unref before assigning something to the pDraw? (I'm not sure, I'm not familiar with X internals) Yeah, I'm just learning this stuff too... Now that I've walked through the life of a GLX pixmap a bit things look even more convoluted. GLX pixmaps are created by __glXDRIscreenCreateDrawable at various times, either explicitly by something like __glXGetDrawable or implicitly by a glXCreateWindow call. All of these end up in DRI2CreateDrawable which either bumps the refcount of an existing pixmap or creates a new one (though with no buffers attached). The drawables get destroyed by their own destroy hook, which points at __glXDRIdrawableDestroy (yes this and the one above should probably have been called glXDRI2...), which is either explicitly called by a cleanup routine (error handling in drawable creation for example), free context time, free screen time, or when a drawable is deref'd and the count drops to 0. What's weird is that __glXUnrefDrawable might be called by DrawableGone (the top level X managed resource for glx drawables) due to __glXUnrefDrawable calling FreeResourceByType, but then DrawableGone will again call __glXUnrefDrawable... I think krh noticed this today and suggested we check the refcount in DrawableGone... But it seems like since __glXUnrefDrawable only frees the resource if the refcount reaches 0, we shouldn't need another unref in DrawableGone... So maybe that's what my fix was running into. Anyway back to more testing reading. -- Jesse Barnes, Intel Open Source Technology Center ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
Re: [RFC] glx: fix DRI2 memory leak
On Thu, 26 Mar 2009 14:20:07 -0700
Jesse Barnes jbar...@virtuousgeek.org wrote:
What's weird is that __glXUnrefDrawable might be called by
DrawableGone (the top level X managed resource for glx drawables) due
to __glXUnrefDrawable calling FreeResourceByType, but then
DrawableGone will again call __glXUnrefDrawable... I think krh
noticed this today and suggested we check the refcount in
DrawableGone... But it seems like since __glXUnrefDrawable only frees
the resource if the refcount reaches 0, we shouldn't need another
unref in DrawableGone... So maybe that's what my fix was running
into.
Ok, I think this is where the leak was:
__glXUnrefDrawable-DrawableGone-__glXUnrefDrawable. This sequence
meant the glxPriv would stay around (since it was used), but
DrawableGone had generally already disposed of the pDrawable before the
call to Unref, so we never got into DRI2DestroyBuffers, thus the leak.
The loop seems broken to me. It *looks* like DrawableGone should be
the real free routine, only called when a drawable really is free, so
I've fixed up the code to conform to that. This means removing the GLX
priv frees from DRI and DRI2 routines and putting them here and using
the GLX unref routine everwhere (since it will only free the resource
when the refcount reaches 0).
Any thoughts? I'd appreciate some more testers too... I'm not quite
sure about the generic DoDestroyDrawable change, but if the refcount is
always 1 there anyway it shouldn't make a difference and seems more
correct.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index ab2d91b..08b866d 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -1222,7 +1222,7 @@ static int DoDestroyDrawable(__GLXclientState *cl, XID
glxdrawable, int type)
}
}
-FreeResource(glxdrawable, FALSE);
+__glXUnrefDrawable(pGlxDraw);
return Success;
}
diff --git a/glx/glxdri.c b/glx/glxdri.c
index 223b06e..253d868 100644
--- a/glx/glxdri.c
+++ b/glx/glxdri.c
@@ -237,8 +237,6 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
serverClient, drawable-pDraw);
__glXleaveServer(GL_FALSE);
}
-
-xfree(private);
}
static GLboolean
diff --git a/glx/glxdri2.c b/glx/glxdri2.c
index 4e76c71..4fdc7df 100644
--- a/glx/glxdri2.c
+++ b/glx/glxdri2.c
@@ -106,8 +106,6 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
* aready have taken care of this, so only call if pDraw isn't NULL. */
if (drawable-pDraw != NULL)
DRI2DestroyDrawable(drawable-pDraw);
-
-xfree(private);
}
static void
diff --git a/glx/glxext.c b/glx/glxext.c
index c882372..0b6c752 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -120,6 +120,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
{
ScreenPtr pScreen = glxPriv-pDraw-pScreen;
+glxPriv-destroy(glxPriv);
+
switch (glxPriv-type) {
case GLX_DRAWABLE_PIXMAP:
case GLX_DRAWABLE_PBUFFER:
@@ -129,7 +131,7 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
glxPriv-pDraw = NULL;
glxPriv-drawId = 0;
-__glXUnrefDrawable(glxPriv);
+xfree(glxPriv);
return True;
}
diff --git a/glx/glxutil.c b/glx/glxutil.c
index bc71087..61323f5 100644
--- a/glx/glxutil.c
+++ b/glx/glxutil.c
@@ -52,11 +52,9 @@ void
__glXUnrefDrawable(__GLXdrawable *glxPriv)
{
glxPriv-refCount--;
-if (glxPriv-refCount == 0) {
+if (glxPriv-refCount == 0)
/* remove the drawable from the drawable list */
FreeResourceByType(glxPriv-drawId, __glXDrawableRes, FALSE);
- glxPriv-destroy(glxPriv);
-}
}
GLboolean
___
xorg mailing list
xorg@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/xorg
[RFC] glx: fix DRI2 memory leak
In trying to track down the memory leak in 20704, I found that the DRI2 drawable destroy routine doesn't seem to get called when new drawables are created and old ones destroyed (as in the glViewport case in the bug). The GLX core code takes care of destroying drawables correctly though, and even calls DestroyPixmap at the right time. However, DRI2 drawables have more state associated with them than just a single pixmap, so we have a __glXDRIdrawableDestroy function that takes care of that. However, by the time we get there, the GLX drawable is gone, so I never saw the if (drawable-pDraw != NULL) DRI2DestroyDrawable(drawable-pDraw); case get triggered... The simple patch below fixed that, but apparently isn't correct (see the bug report) since it causes crashes after a time. My patch was missing another change to set the private-count correctly in dri2GetBuffers though, which may be part of the fix as well. Has anyone else seen this leak? Anyone care to educate me a bit more about GLX drawable lifetime rules? Thanks, Jesse diff --git a/glx/glxext.c b/glx/glxext.c index c882372..73e5a9b 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -127,9 +127,9 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) break; } -glxPriv-pDraw = NULL; glxPriv-drawId = 0; __glXUnrefDrawable(glxPriv); +glxPriv-pDraw = NULL; return True; } ___ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg
