[Mesa-dev] [PATCH 2/2] Make shader refcounting atomic.

[Timothy Arceri]

On Sat, 2016-11-05 at 15:37 +0100, Steinar H. Gunderson wrote:
> These were racy when using the same shaders (seemingly even from
> different
> program objects) on multiple theads sharing the same objects, leading
> to
> issues such as (excerpts from an apitrace dump from a real
> application):
> 
>   1097 @0 glCreateProgram() = 9
>   1099 @0 glAttachShader(program = 9, shader = 7)
>   1101 @0 glAttachShader(program = 9, shader = 8)
>   [...]
>   18122 @2 glCreateProgram() = 137
>   18128 @2 glAttachShader(program = 137, shader = 7)
>   18130 @2 glAttachShader(program = 137, shader = 8)
>   [...]
>   437559 @0 glUseProgram(program = 9)
>   437582 @2 glUseProgram(program = 137)
>   437613 @2 glUseProgram(program = 9)
>   437614 @2 glGetError() = GL_INVALID_VALUE
> 
> with nothing deleting the shaders or programs in-between; just racy
> refcounting, as confirmed by Helgrind:
> 
>   ==13727== Possible data race during read of size 4 at 0x2B3B2648 by
> thread #1
>   ==13727== Locks held: none
>   ==13727==    at 0x1EEBF613: _mesa_reference_shader_program_
> (shaderobj.c:247)
>   ==13727==    by 0x1EEBDFB2: _mesa_use_program (shaderapi.c:1259)
>   ==13727==    by 0x60FA618:
> movit::EffectChain::execute_phase(movit::Phase*, bool, std::set<int,
> std::less<int>, std::allocator<int> >*, std::map<movit::Phase*,
> unsigned int, std::less<movit::Phase*>,
> std::allocator<std::pair<movit::Phase* const, unsigned int> > >*,
> std::set<movit::Phase*, std::less<movit::Phase*>,
> std::allocator<movit::Phase*> >*) (effect_chain.cpp:1885)
>   [...]
>   ==13727== This conflicts with a previous write of size 4 by thread
> #20
>   ==13727== Locks held: none
>   ==13727==    at 0x1EEBF600: _mesa_reference_shader_program_
> (shaderobj.c:236)
>   ==13727==    by 0x1EEBDFB2: _mesa_use_program (shaderapi.c:1259)
>   ==13727==    by 0x60FA618:
> movit::EffectChain::execute_phase(movit::Phase*, bool, std::set<int,
> std::less<int>, std::allocator<int> >*, std::map<movit::Phase*,
> unsigned int, std::less<movit::Phase*>,
> std::allocator<std::pair<movit::Phase* const, unsigned int> > >*,
> std::set<movit::Phase*, std::less<movit::Phase*>,
> std::allocator<movit::Phase*> >*) (effect_chain.cpp:1885)
> 
> Cc: 11.2 12.0 13.0 <mesa-stable at lists.freedesktop.org>
> Signed-off-by: Steinar H. Gunderson <steinar+mesa at gunderson.no>
> ---
>  src/mesa/main/shaderobj.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
> index 8fd574e..08e4379 100644
> --- a/src/mesa/main/shaderobj.c
> +++ b/src/mesa/main/shaderobj.c
> @@ -41,6 +41,7 @@
>  #include "program/prog_parameter.h"
>  #include "util/ralloc.h"
>  #include "util/string_to_uint_map.h"
> +#include "util/u_atomic.h"
>  
>  /*******************************************************************
> ***/
>  /*** Shader object
> functions                                        ***/
> @@ -64,14 +65,11 @@ _mesa_reference_shader(struct gl_context *ctx,
> struct gl_shader **ptr,
>     }
>     if (*ptr) {
>        /* Unreference the old shader */
> -      GLboolean deleteFlag = GL_FALSE;
>        struct gl_shader *old = *ptr;
> +      int old_refcount = p_atomic_dec_return(&old->RefCount);
>  
> -      assert(old->RefCount > 0);
> -      old->RefCount--;
> -      deleteFlag = (old->RefCount == 0);
> -
> -      if (deleteFlag) {
> +      assert(old_refcount >= 0);
> +      if (old_refcount == 0) {

I think you could just use p_atomic_dec_zero(&old->RefCount) here like
Matt did here:

https://lists.freedesktop.org/archives/mesa-dev/2015-August/090979.html

>  	 if (old->Name != 0)
>  	    _mesa_HashRemove(ctx->Shared->ShaderObjects, old->Name);
>           _mesa_delete_shader(ctx, old);
> @@ -83,7 +81,7 @@ _mesa_reference_shader(struct gl_context *ctx,
> struct gl_shader **ptr,
>  
>     if (sh) {
>        /* reference new */
> -      sh->RefCount++;
> +      p_atomic_inc(&sh->RefCount);
>        *ptr = sh;
>     }
>  }
> @@ -226,14 +224,11 @@ _mesa_reference_shader_program_(struct
> gl_context *ctx,
>     }
>     if (*ptr) {
>        /* Unreference the old shader program */
> -      GLboolean deleteFlag = GL_FALSE;
>        struct gl_shader_program *old = *ptr;
> +      int old_refcount = p_atomic_dec_return(&old->RefCount);
>  
> -      assert(old->RefCount > 0);
> -      old->RefCount--;
> -      deleteFlag = (old->RefCount == 0);
> -
> -      if (deleteFlag) {
> +      assert(old_refcount >= 0);
> +      if (old_refcount == 0) {
>  	 if (old->Name != 0)
>  	    _mesa_HashRemove(ctx->Shared->ShaderObjects, old->Name);
>           _mesa_delete_shader_program(ctx, old);
> @@ -244,7 +239,7 @@ _mesa_reference_shader_program_(struct gl_context
> *ctx,
>     assert(!*ptr);
>  
>     if (shProg) {
> -      shProg->RefCount++;
> +      p_atomic_inc(&shProg->RefCount);
>        *ptr = shProg;
>     }
>  }