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 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 > > --Ken > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
