[RFC] glx: fix DRI2 memory leak

[Adam Lantos]

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 <jbarnes at virtuousgeek.org>:
> On Fri, 27 Mar 2009 13:20:25 -0400
> Kristian Høgsberg <krh at bitplanet.net> wrote:
>
>> On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes
>> <jbarnes at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xorg