[Mesa-dev] [PATCH] st/mesa/radeonsi: fix race between destruction of types and shader compilation
Marek Olšák
maraeo at gmail.com
Tue Apr 23 15:18:12 UTC 2019
Reviewed-by: Marek Olšák <marek.olsak at amd.com>
Marek
On Tue, Apr 23, 2019 at 2:04 AM Timothy Arceri <tarceri at itsqueeze.com>
wrote:
> Commit 624789e3708c moved the destruction of types out of atexit() and
> made use of a ref count instead. This is useful for avoiding a crash
> where drivers such as radeonsi are still compiling in a thread when the app
> exits and has not called MakeCurrent to change from the current context.
>
> While the above scenario is technically an app bug we shouldn't crash.
> However that change caused another race condition between the shader
> compilation tread in radeonsi and context teardown functions.
>
> This patch makes two changes to fix this new problem:
>
> First we explicitly call _mesa_destroy_shader_compiler_types() when
> destroying
> the st context rather than calling it indirectly via
> _mesa_free_context_data().
> We do this as we must call it after st_destroy_context_priv() so that we
> don't
> destory the glsl types before the compilation threads finish.
>
> Next wait for the shader threads to finish in si_destroy_context() this
> also means we need to call context destroy before destroying the queues
> in si_destroy_screen().
>
> Fixes: 624789e3708c ("compiler/glsl: handle case where we have multiple
> users for types")
> ---
> src/compiler/glsl/glsl_parser_extras.h | 1 +
> src/gallium/drivers/radeonsi/si_pipe.c | 8 ++++++--
> src/mesa/drivers/dri/i915/intel_context.c | 2 +-
> src/mesa/drivers/dri/i965/brw_context.c | 2 +-
> src/mesa/drivers/dri/nouveau/nouveau_context.c | 2 +-
> src/mesa/drivers/dri/radeon/radeon_common_context.c | 2 +-
> src/mesa/drivers/osmesa/osmesa.c | 8 ++++----
> src/mesa/drivers/x11/xm_api.c | 4 ++--
> src/mesa/main/context.c | 7 ++++---
> src/mesa/main/context.h | 2 +-
> src/mesa/state_tracker/st_context.c | 8 +++++++-
> 11 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/src/compiler/glsl/glsl_parser_extras.h
> b/src/compiler/glsl/glsl_parser_extras.h
> index f92d2160aac..edc6fc06c77 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -997,6 +997,7 @@ extern "C" {
> #endif
>
> struct glcpp_parser;
> +struct _mesa_glsl_parse_state;
>
> typedef void (*glcpp_extension_iterator)(
> struct _mesa_glsl_parse_state *state,
> diff --git a/src/gallium/drivers/radeonsi/si_pipe.c
> b/src/gallium/drivers/radeonsi/si_pipe.c
> index fa96ce34224..e9e1bd0aa38 100644
> --- a/src/gallium/drivers/radeonsi/si_pipe.c
> +++ b/src/gallium/drivers/radeonsi/si_pipe.c
> @@ -150,6 +150,9 @@ static void si_destroy_context(struct pipe_context
> *context)
> struct si_context *sctx = (struct si_context *)context;
> int i;
>
> + util_queue_finish(&sctx->screen->shader_compiler_queue);
> +
> util_queue_finish(&sctx->screen->shader_compiler_queue_low_priority);
> +
> /* Unreference the framebuffer normally to disable related logic
> * properly.
> */
> @@ -702,6 +705,9 @@ static void si_destroy_screen(struct pipe_screen*
> pscreen)
> if (!sscreen->ws->unref(sscreen->ws))
> return;
>
> + mtx_destroy(&sscreen->aux_context_lock);
> + sscreen->aux_context->destroy(sscreen->aux_context);
> +
> util_queue_destroy(&sscreen->shader_compiler_queue);
> util_queue_destroy(&sscreen->shader_compiler_queue_low_priority);
>
> @@ -728,8 +734,6 @@ static void si_destroy_screen(struct pipe_screen*
> pscreen)
> si_gpu_load_kill_thread(sscreen);
>
> mtx_destroy(&sscreen->gpu_load_mutex);
> - mtx_destroy(&sscreen->aux_context_lock);
> - sscreen->aux_context->destroy(sscreen->aux_context);
>
> slab_destroy_parent(&sscreen->pool_transfers);
>
> diff --git a/src/mesa/drivers/dri/i915/intel_context.c
> b/src/mesa/drivers/dri/i915/intel_context.c
> index c23e5ffb26e..aa3175816cf 100644
> --- a/src/mesa/drivers/dri/i915/intel_context.c
> +++ b/src/mesa/drivers/dri/i915/intel_context.c
> @@ -599,7 +599,7 @@ intelDestroyContext(__DRIcontext * driContextPriv)
> driDestroyOptionCache(&intel->optionCache);
>
> /* free the Mesa context */
> - _mesa_free_context_data(&intel->ctx);
> + _mesa_free_context_data(&intel->ctx, true);
>
> _math_matrix_dtr(&intel->ViewportMatrix);
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> index ab637ddf721..df12f373f22 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -1226,7 +1226,7 @@ intelDestroyContext(__DRIcontext * driContextPriv)
> driDestroyOptionCache(&brw->optionCache);
>
> /* free the Mesa context */
> - _mesa_free_context_data(&brw->ctx);
> + _mesa_free_context_data(&brw->ctx, true);
>
> ralloc_free(brw);
> driContextPriv->driverPrivate = NULL;
> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c
> b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> index 93f6ce473a1..8fec35237c0 100644
> --- a/src/mesa/drivers/dri/nouveau/nouveau_context.c
> +++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> @@ -217,7 +217,7 @@ nouveau_context_deinit(struct gl_context *ctx)
> nouveau_object_del(&nctx->hw.chan);
>
> nouveau_scratch_destroy(ctx);
> - _mesa_free_context_data(ctx);
> + _mesa_free_context_data(ctx, true);
> }
>
> void
> diff --git a/src/mesa/drivers/dri/radeon/radeon_common_context.c
> b/src/mesa/drivers/dri/radeon/radeon_common_context.c
> index 47719baa575..689304aa4fc 100644
> --- a/src/mesa/drivers/dri/radeon/radeon_common_context.c
> +++ b/src/mesa/drivers/dri/radeon/radeon_common_context.c
> @@ -270,7 +270,7 @@ void radeonDestroyContext(__DRIcontext *driContextPriv
> )
>
> /* free atom list */
> /* free the Mesa context data */
> - _mesa_free_context_data(&radeon->glCtx);
> + _mesa_free_context_data(&radeon->glCtx, true);
>
> /* free the option cache */
> driDestroyOptionCache(&radeon->optionCache);
> diff --git a/src/mesa/drivers/osmesa/osmesa.c
> b/src/mesa/drivers/osmesa/osmesa.c
> index 44374a2e917..a065161b89e 100644
> --- a/src/mesa/drivers/osmesa/osmesa.c
> +++ b/src/mesa/drivers/osmesa/osmesa.c
> @@ -854,7 +854,7 @@ OSMesaCreateContextAttribs(const int *attribList,
> OSMesaContext sharelist)
> osmesa->gl_buffer = _mesa_create_framebuffer(osmesa->gl_visual);
> if (!osmesa->gl_buffer) {
> _mesa_destroy_visual( osmesa->gl_visual );
> - _mesa_free_context_data( &osmesa->mesa );
> + _mesa_free_context_data(&osmesa->mesa, true);
> free(osmesa);
> return NULL;
> }
> @@ -891,7 +891,7 @@ OSMesaCreateContextAttribs(const int *attribList,
> OSMesaContext sharelist)
> !_tnl_CreateContext( ctx ) ||
> !_swsetup_CreateContext( ctx )) {
> _mesa_destroy_visual(osmesa->gl_visual);
> - _mesa_free_context_data(ctx);
> + _mesa_free_context_data(ctx, true);
> free(osmesa);
> return NULL;
> }
> @@ -919,7 +919,7 @@ OSMesaCreateContextAttribs(const int *attribList,
> OSMesaContext sharelist)
>
> if (ctx->Version < version_major * 10 + version_minor) {
> _mesa_destroy_visual(osmesa->gl_visual);
> - _mesa_free_context_data(ctx);
> + _mesa_free_context_data(ctx, true);
> free(osmesa);
> return NULL;
> }
> @@ -955,7 +955,7 @@ OSMesaDestroyContext( OSMesaContext osmesa )
> _mesa_destroy_visual( osmesa->gl_visual );
> _mesa_reference_framebuffer( &osmesa->gl_buffer, NULL );
>
> - _mesa_free_context_data( &osmesa->mesa );
> + _mesa_free_context_data(&osmesa->mesa, true);
> free( osmesa );
> }
> }
> diff --git a/src/mesa/drivers/x11/xm_api.c b/src/mesa/drivers/x11/xm_api.c
> index e8405950656..90f89a36221 100644
> --- a/src/mesa/drivers/x11/xm_api.c
> +++ b/src/mesa/drivers/x11/xm_api.c
> @@ -945,7 +945,7 @@ XMesaContext XMesaCreateContext( XMesaVisual v,
> XMesaContext share_list )
> !_vbo_CreateContext( mesaCtx ) ||
> !_tnl_CreateContext( mesaCtx ) ||
> !_swsetup_CreateContext( mesaCtx )) {
> - _mesa_free_context_data(&c->mesa);
> + _mesa_free_context_data(&c->mesa, true);
> free(c);
> return NULL;
> }
> @@ -982,7 +982,7 @@ void XMesaDestroyContext( XMesaContext c )
> _swrast_DestroyContext( mesaCtx );
> _tnl_DestroyContext( mesaCtx );
> _vbo_DestroyContext( mesaCtx );
> - _mesa_free_context_data( mesaCtx );
> + _mesa_free_context_data(mesaCtx, true);
> free( c );
> }
>
> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> index 7300d2f3c46..2c3d9a11ce3 100644
> --- a/src/mesa/main/context.c
> +++ b/src/mesa/main/context.c
> @@ -1310,7 +1310,7 @@ fail:
> * \sa _mesa_initialize_context() and init_attrib_groups().
> */
> void
> -_mesa_free_context_data( struct gl_context *ctx )
> +_mesa_free_context_data(struct gl_context *ctx, bool
> destroy_compiler_types)
> {
> if (!_mesa_get_current_context()){
> /* No current context, but we may need one in order to delete
> @@ -1385,7 +1385,8 @@ _mesa_free_context_data( struct gl_context *ctx )
>
> free(ctx->VersionString);
>
> - _mesa_destroy_shader_compiler_types();
> + if (destroy_compiler_types)
> + _mesa_destroy_shader_compiler_types();
>
> /* unbind the context if it's currently bound */
> if (ctx == _mesa_get_current_context()) {
> @@ -1405,7 +1406,7 @@ void
> _mesa_destroy_context( struct gl_context *ctx )
> {
> if (ctx) {
> - _mesa_free_context_data(ctx);
> + _mesa_free_context_data(ctx, true);
> free( (void *) ctx );
> }
> }
> diff --git a/src/mesa/main/context.h b/src/mesa/main/context.h
> index 7de10e9924b..e9b270df14c 100644
> --- a/src/mesa/main/context.h
> +++ b/src/mesa/main/context.h
> @@ -115,7 +115,7 @@ _mesa_initialize_context( struct gl_context *ctx,
> const struct dd_function_table
> *driverFunctions);
>
> extern void
> -_mesa_free_context_data( struct gl_context *ctx );
> +_mesa_free_context_data(struct gl_context *ctx, bool
> destroy_compiler_types);
>
> extern void
> _mesa_destroy_context( struct gl_context *ctx );
> diff --git a/src/mesa/state_tracker/st_context.c
> b/src/mesa/state_tracker/st_context.c
> index 09d467aa360..067d47ef5e9 100644
> --- a/src/mesa/state_tracker/st_context.c
> +++ b/src/mesa/state_tracker/st_context.c
> @@ -85,6 +85,7 @@
> #include "util/u_upload_mgr.h"
> #include "util/u_vbuf.h"
> #include "cso_cache/cso_context.h"
> +#include "compiler/glsl/glsl_parser_extras.h"
>
>
> DEBUG_GET_ONCE_BOOL_OPTION(mesa_mvp_dp4, "MESA_MVP_DP4", FALSE)
> @@ -976,13 +977,18 @@ st_destroy_context(struct st_context *st)
>
> st_destroy_program_variants(st);
>
> - _mesa_free_context_data(ctx);
> + _mesa_free_context_data(ctx, false);
>
> /* This will free the st_context too, so 'st' must not be accessed
> * afterwards. */
> st_destroy_context_priv(st, true);
> st = NULL;
>
> + /* This must be called after st_destroy_context_priv() to avoid a race
> + * condition between any shader compiler threads and context
> destruction.
> + */
> + _mesa_destroy_shader_compiler_types();
> +
> free(ctx);
>
> if (save_ctx == ctx) {
> --
> 2.20.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190423/11458ae1/attachment-0001.html>
More information about the mesa-dev
mailing list