On 02/16/2014 10:33 PM, Daniel Kurtz wrote: > On Tue, Feb 11, 2014 at 4:27 PM, Kenneth Graunke <kenn...@whitecape.org> > wrote: >> On 02/07/2014 03:29 AM, Daniel Kurtz wrote: >>> Consider a multithreaded program with two contexts A and B, and the >>> following scenario: >>> >>> 1. Context A calls initialize(), which allocates mem_ctx and starts >>> building built-ins. >>> 2. Context B calls initialize(), which sees mem_ctx != NULL and assumes >>> everything is already set up. It returns. >>> 3. Context B calls find(), which fails to find the built-in since it >>> hasn't been created yet. >>> 4. Context A finally finishes initializing the built-ins. >>> >>> This will break at step 3. Adding a lock ensures that subsequent >>> callers of initialize() will wait until initialization is actually >>> complete. >>> >>> Similarly, if any thread calls release while another thread is still >>> initializing, or calling find(), the mem_ctx/shader would get free'd while >>> from under it, leading to corruption or use-after-free crashes. >>> >>> Fixes sporadic failures in Piglit's glx-multithread-shader-compile as >>> reported in: >>> https://bugs.freedesktop.org/69200 >>> >>> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> >>> Signed-off-by: Daniel Kurtz <djku...@chromium.org> >>> --- >>> src/glsl/builtin_functions.cpp | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/glsl/builtin_functions.cpp b/src/glsl/builtin_functions.cpp >>> index aeb8e5d..2162baa 100644 >>> --- a/src/glsl/builtin_functions.cpp >>> +++ b/src/glsl/builtin_functions.cpp >>> @@ -4100,6 +4100,7 @@ builtin_builder::_mid3(const glsl_type *type) >>> >>> /* The singleton instance of builtin_builder. */ >>> static builtin_builder builtins; >>> +_glthread_DECLARE_STATIC_MUTEX(builtins_lock); >>> >>> /** >>> * External API (exposing the built-in module to the rest of the compiler): >>> @@ -4108,20 +4109,28 @@ static builtin_builder builtins; >>> void >>> _mesa_glsl_initialize_builtin_functions() >>> { >>> + _glthread_LOCK_MUTEX(builtins_lock); >>> builtins.initialize(); >>> + _glthread_UNLOCK_MUTEX(builtins_lock); >>> } >>> >>> void >>> _mesa_glsl_release_builtin_functions() >>> { >>> + _glthread_LOCK_MUTEX(builtins_lock); >>> builtins.release(); >>> + _glthread_UNLOCK_MUTEX(builtins_lock); >>> } >>> >>> ir_function_signature * >>> _mesa_glsl_find_builtin_function(_mesa_glsl_parse_state *state, >>> const char *name, exec_list >>> *actual_parameters) >>> { >>> - return builtins.find(state, name, actual_parameters); >>> + ir_function_signature * s; >>> + _glthread_LOCK_MUTEX(builtins_lock); >>> + s = builtins.find(state, name, actual_parameters); >>> + _glthread_UNLOCK_MUTEX(builtins_lock); >>> + return s; >>> } >>> >>> gl_shader * >>> >> >> This looks good to me. >> >> Locking around initialize() and release() is definitely necessary to >> prevent multiple threads from trying to create/tear down the data. >> >> Locking around find() prevents a second thread from calling release() >> and wrecking data that's in use. >> >> We definitely do need more locking. I believe the hash table usage in >> glsl_types.cpp for creating new array or record types needs a similar >> treatment. Would you like to cook up a patch for that, Daniel? >> >> In the meantime, I think we can push this. Even if it's not complete, I >> believe everything here is necessary. Thanks for the patch! > > Hi Ken, > > Thanks for taking a look at this patch. > I don't have rights to push to mesa, though, so hope someone with > rights can push.
I actually pushed it last Tuesday:
commit b47d231526821f5cff99546a984103a7222bc66c
Author: Daniel Kurtz <djku...@chromium.org>
AuthorDate: Fri Feb 7 19:29:06 2014 +0800
Commit: Kenneth Graunke <kenn...@whitecape.org>
CommitDate: Tue Feb 11 02:21:41 2014 -0800
glsl: Add locking to builtin_builder singleton
> I don't think I'll get a chance to write the follow on patch anytime soon.
> I did the first one to fix a crash while doing full testing piglit
> runs on some test hardware.
> It would take a bit of effort to context switch back and expand this.
>
> -Dan
Okay, no problem. Thanks for the patch.
--Ken
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
