[PATCH 2/4] glx: Fix memory leak in context garbage collection (v2)

Ian Romanick idr at freedesktop.org
Tue Oct 8 10:00:02 PDT 2013


On 10/02/2013 02:51 PM, Adam Jackson wrote:
> I broke this, back in:
> 
>     commit a48dadc98a28c969741979b70b7a639f24f4cbbd
>     Author: Adam Jackson <ajax at redhat.com>
>     Date:   Mon Mar 21 11:59:29 2011 -0400
> 
> 	glx: Reimplement context tags
> 
> In that, I changed the glx client state to not explicitly track the list
> of current contexts for the client (since that was what we were deriving
> tags from).  The bug was that I removed the code for same from
> glxClientCallback without noticing that it had the side effect of
> effectively de-currenting those contexts, so that ContextGone could free
> them.  So, if you had a client exit with a context still current, the
> context's memory would leak.  Not a huge deal for direct clients, but
> viciously bad for indirect, since the swrast context state at the bottom
> of Mesa is like 15M.
> 
> Fix this by promoting Bool isCurrent to ClientPtr currentClient, so that
> we have a back-pointer to chase when walking the list of contexts when
> ClientStateGone happens.
> 
> v2: Explicitly call __glXFreeContext on the ClientStateGone path.  Our
> current context might be one we got from EXT_import_context and whose
> creating client has since died.  Without the explicit call, the creating
> client's FreeClientResources would not free the context because it's
> still current, and the using client's FreeClientResources would not free
> the context because it's not an XID it created.  This matches the logic
> from a48dadc.

This seems like 3 separate changes:

 - Rename isCurrent to currentClient

 - Reorder __glXRemoveFromContextList wrt free.

 - Release all contexts owned by a client on ClientGone.

*If* you have to do a v3, you should split it.  It doesn't feel worth
the effort to do a v3 just for a split.

That said, I believe this patch is correct.

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
>  glx/createcontext.c |  2 +-
>  glx/glxcmds.c       | 14 +++++++-------
>  glx/glxcontext.h    | 10 +++++-----
>  glx/glxext.c        | 33 ++++++++++++++++++++++-----------
>  4 files changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/glx/createcontext.c b/glx/createcontext.c
> index 13d21cc..78792da 100644
> --- a/glx/createcontext.c
> +++ b/glx/createcontext.c
> @@ -320,7 +320,7 @@ __glXDisp_CreateContextAttribsARB(__GLXclientState * cl, GLbyte * pc)
>      ctx->id = req->context;
>      ctx->share_id = req->shareList;
>      ctx->idExists = True;
> -    ctx->isCurrent = False;
> +    ctx->currentClient = False;
>      ctx->isDirect = req->isDirect;
>      ctx->hasUnflushedCommands = False;
>      ctx->renderMode = GL_RENDER;
> diff --git a/glx/glxcmds.c b/glx/glxcmds.c
> index 663448a..288b417 100644
> --- a/glx/glxcmds.c
> +++ b/glx/glxcmds.c
> @@ -290,7 +290,7 @@ DoCreateContext(__GLXclientState * cl, GLXContextID gcId,
>      glxc->id = gcId;
>      glxc->share_id = shareList;
>      glxc->idExists = GL_TRUE;
> -    glxc->isCurrent = GL_FALSE;
> +    glxc->currentClient = NULL;
>      glxc->isDirect = isDirect;
>      glxc->hasUnflushedCommands = GL_FALSE;
>      glxc->renderMode = GL_RENDER;
> @@ -398,7 +398,7 @@ __glXDisp_DestroyContext(__GLXclientState * cl, GLbyte * pc)
>          return err;
>  
>      glxc->idExists = GL_FALSE;
> -    if (!glxc->isCurrent)
> +    if (!glxc->currentClient)
>          FreeResourceByType(req->context, __glXContextRes, FALSE);
>  
>      return Success;
> @@ -431,7 +431,7 @@ static void
>  StopUsingContext(__GLXcontext * glxc)
>  {
>      if (glxc) {
> -        glxc->isCurrent = GL_FALSE;
> +        glxc->currentClient = NULL;
>          if (!glxc->idExists) {
>              FreeResourceByType(glxc->id, __glXContextRes, FALSE);
>          }
> @@ -441,7 +441,7 @@ StopUsingContext(__GLXcontext * glxc)
>  static void
>  StartUsingContext(__GLXclientState * cl, __GLXcontext * glxc)
>  {
> -    glxc->isCurrent = GL_TRUE;
> +    glxc->currentClient = cl->client;
>  }
>  
>  /**
> @@ -571,7 +571,7 @@ DoMakeCurrent(__GLXclientState * cl,
>  
>          if (!validGlxContext(client, contextId, DixUseAccess, &glxc, &error))
>              return error;
> -        if ((glxc != prevglxc) && glxc->isCurrent) {
> +        if ((glxc != prevglxc) && glxc->currentClient) {
>              /* Context is current to somebody else */
>              return BadAccess;
>          }
> @@ -633,7 +633,7 @@ DoMakeCurrent(__GLXclientState * cl,
>              return __glXError(GLXBadContext);
>          }
>  
> -        glxc->isCurrent = GL_TRUE;
> +        glxc->currentClient = client;
>      }
>  
>      StopUsingContext(prevglxc);
> @@ -854,7 +854,7 @@ __glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc)
>      /*
>       ** The destination context must not be current for any client.
>       */
> -    if (dst->isCurrent) {
> +    if (dst->currentClient) {
>          client->errorValue = dest;
>          return BadAccess;
>      }
> diff --git a/glx/glxcontext.h b/glx/glxcontext.h
> index 96a4dd2..fd018d0 100644
> --- a/glx/glxcontext.h
> +++ b/glx/glxcontext.h
> @@ -70,6 +70,11 @@ struct __GLXcontext {
>      __GLXscreen *pGlxScreen;
>  
>      /*
> +     ** If this context is current for a client, this will be that client
> +     */
> +    ClientPtr currentClient;
> +
> +    /*
>       ** The XID of this context.
>       */
>      XID id;
> @@ -85,11 +90,6 @@ struct __GLXcontext {
>      GLboolean idExists;
>  
>      /*
> -     ** Whether this context is current for some client.
> -     */
> -    GLboolean isCurrent;
> -
> -    /*
>       ** Whether this context is a direct rendering context.
>       */
>      GLboolean isDirect;
> diff --git a/glx/glxext.c b/glx/glxext.c
> index 1bb884f..ff2ceae 100644
> --- a/glx/glxext.c
> +++ b/glx/glxext.c
> @@ -88,16 +88,15 @@ __glXResetLargeCommandStatus(__GLXclientState * cl)
>  }
>  
>  /*
> -** This procedure is called when the client who created the context goes
> -** away OR when glXDestroyContext is called.  In either case, all we do is
> -** flag that the ID is no longer valid, and (maybe) free the context.
> -** use.
> -*/
> + * This procedure is called when the client who created the context goes away
> + * OR when glXDestroyContext is called.  In either case, all we do is flag that
> + * the ID is no longer valid, and (maybe) free the context.
> + */
>  static int
>  ContextGone(__GLXcontext * cx, XID id)
>  {
>      cx->idExists = GL_FALSE;
> -    if (!cx->isCurrent) {
> +    if (!cx->currentClient) {
>          __glXFreeContext(cx);
>      }
>  
> @@ -131,9 +130,10 @@ DrawableGone(__GLXdrawable * glxPriv, XID xid)
>  
>      for (c = glxAllContexts; c; c = next) {
>          next = c->next;
> -        if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
> +        if (c->currentClient &&
> +		(c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
>              (*c->loseCurrent) (c);
> -            c->isCurrent = GL_FALSE;
> +            c->currentClient = NULL;
>          }
>          if (c->drawPriv == glxPriv)
>              c->drawPriv = NULL;
> @@ -187,14 +187,14 @@ __glXRemoveFromContextList(__GLXcontext * cx)
>  GLboolean
>  __glXFreeContext(__GLXcontext * cx)
>  {
> -    if (cx->idExists || cx->isCurrent)
> +    if (cx->idExists || cx->currentClient)
>          return GL_FALSE;
>  
> +    __glXRemoveFromContextList(cx);
> +
>      free(cx->feedbackBuf);
>      free(cx->selectBuf);
>  
> -    __glXRemoveFromContextList(cx);
> -
>      /* We can get here through both regular dispatching from
>       * __glXDispatch() or as a callback from the resource manager.  In
>       * the latter case we need to lift the DRI lock manually. */
> @@ -271,6 +271,7 @@ glxClientCallback(CallbackListPtr *list, pointer closure, pointer data)
>      NewClientInfoRec *clientinfo = (NewClientInfoRec *) data;
>      ClientPtr pClient = clientinfo->client;
>      __GLXclientState *cl = glxGetClient(pClient);
> +    __GLXcontext *c, *next;
>  
>      switch (pClient->clientState) {
>      case ClientStateRunning:
> @@ -282,6 +283,16 @@ glxClientCallback(CallbackListPtr *list, pointer closure, pointer data)
>          break;
>  
>      case ClientStateGone:
> +        /* detach from all current contexts */
> +        for (c = glxAllContexts; c; c = next) {
> +            next = c->next;
> +            if (c->currentClient == pClient) {
> +                c->loseCurrent(c);
> +                c->currentClient = NULL;
> +                __glXFreeContext(c);
> +            }
> +        }
> +
>          free(cl->returnBuf);
>          free(cl->largeCmdBuf);
>          free(cl->GLClientextensions);
> 



More information about the xorg-devel mailing list