[Mesa-dev] [PATCH] glsl: Add locking to builtin_builder singleton

[Kenneth Graunke]

On 02/16/2014 10:33 PM, Daniel Kurtz wrote:
> On Tue, Feb 11, 2014 at 4:27 PM, Kenneth Graunke <kenneth at 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 <kenneth at whitecape.org>
>>> Signed-off-by: Daniel Kurtz <djkurtz at 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 <djkurtz at chromium.org>
AuthorDate: Fri Feb 7 19:29:06 2014 +0800
Commit:     Kenneth Graunke <kenneth at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140216/9e8db725/attachment.pgp>