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-sta...@lists.freedesktop.org>
> Signed-off-by: Steinar H. Gunderson <steinar+m...@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;
> }
> }
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
