On Fri, Mar 15, 2019 at 8:55 AM Jose Fonseca <jfons...@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 <jfons...@vmware.com> > > Jose > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev