[Mesa-dev] [PATCH 1/8] st/mesa: implement "zombie" sampler views
Stéphane Marchesin
stephane.marchesin at gmail.com
Fri Mar 15 21:55:21 UTC 2019
On Fri, Mar 15, 2019 at 8:55 AM Jose Fonseca <jfonseca at vmware.com> 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.
>
> 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!
>
+1 this thing has been a long standing pain of ours as well. Thanks
for tackling it!
Stéphane
> Reviewed-by: Jose Fonseca <jfonseca at vmware.com>
>
> Jose
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list