[Mesa-dev] [PATCH 1/8] st/mesa: implement "zombie" sampler views

Jose Fonseca jfonseca at vmware.com
Fri Mar 15 15:54:54 UTC 2019


On 14/03/2019 19:37, Brian Paul wrote:
> 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.
> ---
>   src/mesa/state_tracker/st_cb_flush.c     |  6 +++
>   src/mesa/state_tracker/st_context.c      | 81 ++++++++++++++++++++++++++++++++
>   src/mesa/state_tracker/st_context.h      | 25 ++++++++++
>   src/mesa/state_tracker/st_sampler_view.c | 27 +++++++++--
>   src/mesa/state_tracker/st_texture.h      |  3 ++
>   5 files changed, 138 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..bd919da 100644
> --- a/src/mesa/state_tracker/st_context.c
> +++ b/src/mesa/state_tracker/st_context.c
> @@ -261,6 +261,80 @@ 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 save the view for later deletion.  The context here is
> + * 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);
> +   assert(view->reference.count == 1);
> +
> +   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);
> +      assert(entry->view->reference.count == 1);
> +      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 +642,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 +845,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..d16d523 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,27 @@ 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 the refcount==1 here, it means stsv->view is the one and only
> +          * reference.
> +          */
> +         if (stsv->st != st && stsv->view->reference.count == 1) {
> +            /* This sampler belongs to a different context so we don't
> +             * want to free it with this pipe context.
> +             * Instead, put it on the other context's zombie list.
> +             */
> +            st_save_zombie_sampler_view(stsv->st, stsv->view);
> +            /* The refcount is transferred to the zombie list */
> +            stsv->view = NULL;
> +         } else {
> +            pipe_sampler_view_reference(&stsv->view, NULL);

I've just realized that there's a tiny race condition here.  Imagine:
- the stsv->view->reference.count is initially 2 and we take this branch
- then another thread reduces reference count to 1
- when we call pipe_sampler_view_reference it goes to zero and we end up 
freeing with the wrong context.

To fix this we'd probably need to do a compare-and-exchange instead of a 
decrement to ensure we wouldn't inadvertently free with the wrong 
context if this happened.

It's not a trivial fix.  So for now I'd just add a FIXME commment.

Alternatively, a simple way to avoid this race condition would be to 
always put the view on a zombie, when it's on a difference context, 
regardless of count.

> +         }
> +      }
> +   }
> +   views->count = 0;
>      simple_mtx_unlock(&stObj->validate_mutex);
>   }
>   
> @@ -241,6 +259,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 */
> 

Otherwise looks great.  It's nice to finally to have a proper solution 
for this long standing tricky issue!

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>

Jose


More information about the mesa-dev mailing list