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

[Brian Paul]

On 03/15/2019 09:54 AM, Jose Fonseca wrote:
> 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.

Yeah, the test for refcount==1 kind of worried me too.


> 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.

I think that would work, but I should update the comments to indicate 
that a zombie sampler view may not necessarily be dead and I'll have to 
remove a few assertions.

I'll post a v2 of this patch after more testing.

-Brian


> 
>> +         }
>> +      }
>> +   }
>> +   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