[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