[Mesa-dev] [PATCH] st/mesa: implement "zombie" sampler views (v2)
Roland Scheidegger
sroland at vmware.com
Mon Mar 18 18:32:33 UTC 2019
Reviewed-by: Roland Scheidegger <sroland at vmware.com>
Am 15.03.19 um 22:12 schrieb Brian Paul:
> When st_texture_release_all_sampler_views() is called the texture may
> have sampler views belonging to several contexts. If we unreference a
> sampler view and its refcount hits zero, we need to be sure to destroy
> the sampler view with the same context which created it.
>
> This was not the case with the previous code which used
> pipe_sampler_view_release(). That function could end up freeing a
> sampler view with a context different than the one which created it.
> In the case of the VMware svga driver, we detected this but leaked the
> sampler view. This led to a crash with google-chrome when the kernel
> module had too many sampler views. VMware bug 2274734.
>
> Alternately, if we try to delete a sampler view with the correct
> context, we may be "reaching into" a context which is active on
> another thread. That's not safe.
>
> To fix these issues this patch adds a per-context list of "zombie"
> sampler views. These are views which are to be freed at some point
> when the context is active. Other contexts may safely add sampler
> views to the zombie list at any time (it's mutex protected). This
> avoids the context/view ownership mix-ups we had before.
>
> Tested with: google-chrome, google earth, Redway3D Watch/Turbine demos
> a few Linux games. If anyone can recomment some other multi-threaded,
> multi-context GL apps to test, please let me know.
>
> v2: avoid potential race issue by always adding sampler views to the
> zombie list if the view's context doesn't match the current context,
> ignoring the refcount.
>
> Reviewed-by: Roland Scheidegger <sroland at vmware.com>
> Reviewed-by: Neha Bhende <bhenden at vmware.com>
> Reviewed-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> Reviewed-By: Jose Fonseca <jfonseca at vmware.com>
> ---
> src/mesa/state_tracker/st_cb_flush.c | 6 +++
> src/mesa/state_tracker/st_context.c | 80 ++++++++++++++++++++++++++++++++
> src/mesa/state_tracker/st_context.h | 25 ++++++++++
> src/mesa/state_tracker/st_sampler_view.c | 21 +++++++--
> src/mesa/state_tracker/st_texture.h | 3 ++
> 5 files changed, 131 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_flush.c b/src/mesa/state_tracker/st_cb_flush.c
> index 5b3188c..81e5338 100644
> --- a/src/mesa/state_tracker/st_cb_flush.c
> +++ b/src/mesa/state_tracker/st_cb_flush.c
> @@ -39,6 +39,7 @@
> #include "st_cb_flush.h"
> #include "st_cb_clear.h"
> #include "st_cb_fbo.h"
> +#include "st_context.h"
> #include "st_manager.h"
> #include "pipe/p_context.h"
> #include "pipe/p_defines.h"
> @@ -53,6 +54,11 @@ st_flush(struct st_context *st,
> {
> st_flush_bitmap_cache(st);
>
> + /* We want to call this function periodically.
> + * Typically, it has nothing to do so it shouldn't be expensive.
> + */
> + st_context_free_zombie_objects(st);
> +
> st->pipe->flush(st->pipe, fence, flags);
> }
>
> diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c
> index 2898279..c38f8e5 100644
> --- a/src/mesa/state_tracker/st_context.c
> +++ b/src/mesa/state_tracker/st_context.c
> @@ -261,6 +261,79 @@ st_invalidate_state(struct gl_context *ctx)
> }
>
>
> +/*
> + * In some circumstances (such as running google-chrome) the state
> + * tracker may try to delete a resource view from a context different
> + * than when it was created. We don't want to do that.
> + *
> + * In that situation, st_texture_release_all_sampler_views() calls this
> + * function to transfer the sampler view reference to this context (expected
> + * to be the context which created the view.)
> + */
> +void
> +st_save_zombie_sampler_view(struct st_context *st,
> + struct pipe_sampler_view *view)
> +{
> + struct st_zombie_sampler_view_node *entry;
> +
> + assert(view->context == st->pipe);
> +
> + entry = MALLOC_STRUCT(st_zombie_sampler_view_node);
> + if (!entry)
> + return;
> +
> + entry->view = view;
> +
> + /* We need a mutex since this function may be called from one thread
> + * while free_zombie_resource_views() is called from another.
> + */
> + mtx_lock(&st->zombie_sampler_views.mutex);
> + LIST_ADDTAIL(&entry->node, &st->zombie_sampler_views.list.node);
> + mtx_unlock(&st->zombie_sampler_views.mutex);
> +}
> +
> +
> +/*
> + * Free any zombie sampler views that may be attached to this context.
> + */
> +static void
> +free_zombie_sampler_views(struct st_context *st)
> +{
> + struct st_zombie_sampler_view_node *entry, *next;
> +
> + if (LIST_IS_EMPTY(&st->zombie_sampler_views.list.node)) {
> + return;
> + }
> +
> + mtx_lock(&st->zombie_sampler_views.mutex);
> +
> + LIST_FOR_EACH_ENTRY_SAFE(entry, next,
> + &st->zombie_sampler_views.list.node, node) {
> + LIST_DEL(&entry->node); // remove this entry from the list
> +
> + assert(entry->view->context == st->pipe);
> + pipe_sampler_view_reference(&entry->view, NULL);
> +
> + free(entry);
> + }
> +
> + assert(LIST_IS_EMPTY(&st->zombie_sampler_views.list.node));
> +
> + mtx_unlock(&st->zombie_sampler_views.mutex);
> +}
> +
> +
> +/*
> + * This function is called periodically to free any zombie objects
> + * which are attached to this context.
> + */
> +void
> +st_context_free_zombie_objects(struct st_context *st)
> +{
> + free_zombie_sampler_views(st);
> +}
> +
> +
> static void
> st_destroy_context_priv(struct st_context *st, bool destroy_pipe)
> {
> @@ -568,6 +641,9 @@ st_create_context_priv(struct gl_context *ctx, struct pipe_context *pipe,
> /* Initialize context's winsys buffers list */
> LIST_INITHEAD(&st->winsys_buffers);
>
> + LIST_INITHEAD(&st->zombie_sampler_views.list.node);
> + mtx_init(&st->zombie_sampler_views.mutex, mtx_plain);
> +
> return st;
> }
>
> @@ -768,6 +844,10 @@ st_destroy_context(struct st_context *st)
> _mesa_make_current(ctx, NULL, NULL);
> }
>
> + st_context_free_zombie_objects(st);
> +
> + mtx_destroy(&st->zombie_sampler_views.mutex);
> +
> /* This must be called first so that glthread has a chance to finish */
> _mesa_glthread_destroy(ctx);
>
> diff --git a/src/mesa/state_tracker/st_context.h b/src/mesa/state_tracker/st_context.h
> index a3f70ec..1106bb6 100644
> --- a/src/mesa/state_tracker/st_context.h
> +++ b/src/mesa/state_tracker/st_context.h
> @@ -37,6 +37,7 @@
> #include "util/u_inlines.h"
> #include "util/list.h"
> #include "vbo/vbo.h"
> +#include "util/list.h"
>
>
> #ifdef __cplusplus
> @@ -95,6 +96,16 @@ struct drawpix_cache_entry
> };
>
>
> +/*
> + * Node for a linked list of dead sampler views.
> + */
> +struct st_zombie_sampler_view_node
> +{
> + struct pipe_sampler_view *view;
> + struct list_head node;
> +};
> +
> +
> struct st_context
> {
> struct st_context_iface iface;
> @@ -306,6 +317,11 @@ struct st_context
> * the estimated allocated size needed to execute those operations.
> */
> struct util_throttle throttle;
> +
> + struct {
> + struct st_zombie_sampler_view_node list;
> + mtx_t mutex;
> + } zombie_sampler_views;
> };
>
>
> @@ -334,6 +350,15 @@ extern void
> st_invalidate_buffers(struct st_context *st);
>
>
> +extern void
> +st_save_zombie_sampler_view(struct st_context *st,
> + struct pipe_sampler_view *view);
> +
> +void
> +st_context_free_zombie_objects(struct st_context *st);
> +
> +
> +
> /**
> * Wrapper for struct gl_framebuffer.
> * This is an opaque type to the outside world.
> diff --git a/src/mesa/state_tracker/st_sampler_view.c b/src/mesa/state_tracker/st_sampler_view.c
> index e4eaf39..b20a00c 100644
> --- a/src/mesa/state_tracker/st_sampler_view.c
> +++ b/src/mesa/state_tracker/st_sampler_view.c
> @@ -150,6 +150,7 @@ found:
> sv->glsl130_or_later = glsl130_or_later;
> sv->srgb_skip_decode = srgb_skip_decode;
> sv->view = view;
> + sv->st = st;
>
> out:
> simple_mtx_unlock(&stObj->validate_mutex);
> @@ -213,8 +214,6 @@ void
> st_texture_release_all_sampler_views(struct st_context *st,
> struct st_texture_object *stObj)
> {
> - GLuint i;
> -
> /* TODO: This happens while a texture is deleted, because the Driver API
> * is asymmetric: the driver allocates the texture object memory, but
> * mesa/main frees it.
> @@ -224,8 +223,21 @@ st_texture_release_all_sampler_views(struct st_context *st,
>
> simple_mtx_lock(&stObj->validate_mutex);
> struct st_sampler_views *views = stObj->sampler_views;
> - for (i = 0; i < views->count; ++i)
> - pipe_sampler_view_release(st->pipe, &views->views[i].view);
> + for (unsigned i = 0; i < views->count; ++i) {
> + struct st_sampler_view *stsv = &views->views[i];
> + if (stsv->view) {
> + if (stsv->st != st) {
> + /* Transfer this reference to the zombie list. It will
> + * likely be freed when the zombie list is freed.
> + */
> + st_save_zombie_sampler_view(stsv->st, stsv->view);
> + stsv->view = NULL;
> + } else {
> + pipe_sampler_view_reference(&stsv->view, NULL);
> + }
> + }
> + }
> + views->count = 0;
> simple_mtx_unlock(&stObj->validate_mutex);
> }
>
> @@ -241,6 +253,7 @@ st_delete_texture_sampler_views(struct st_context *st,
> st_texture_release_all_sampler_views(st, stObj);
>
> /* Free the container of the current per-context sampler views */
> + assert(stObj->sampler_views->count == 0);
> free(stObj->sampler_views);
> stObj->sampler_views = NULL;
>
> diff --git a/src/mesa/state_tracker/st_texture.h b/src/mesa/state_tracker/st_texture.h
> index f71d5a0..c5fc30c 100644
> --- a/src/mesa/state_tracker/st_texture.h
> +++ b/src/mesa/state_tracker/st_texture.h
> @@ -57,6 +57,9 @@ struct st_sampler_view
> {
> struct pipe_sampler_view *view;
>
> + /** The context which created this view */
> + struct st_context *st;
> +
> /** The glsl version of the shader seen during validation */
> bool glsl130_or_later;
> /** Derived from the sampler's sRGBDecode state during validation */
>
More information about the mesa-dev
mailing list