[Mesa-dev] [PATCH 1/9] glsl: fix a race condition when inserting new types
Timothy Arceri
tarceri at itsqueeze.com
Mon May 15 23:32:07 UTC 2017
On 15/05/17 19:27, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> By splitting glsl_type::mutex into two, we can avoid dropping the hash
> mutex while creating the new type instance (e.g. struct/record,
> interface).
>
> This fixes a time-of-check/time-of-use race where two threads would
> simultaneously attempt to create the same type but end up with different
> instances of glsl_type.
> ---
> src/compiler/glsl_types.cpp | 61 +++++++++++++++++++--------------------------
> src/compiler/glsl_types.h | 11 ++++----
> 2 files changed, 32 insertions(+), 40 deletions(-)
>
> diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp
> index 0fd1230..063d3f1 100644
> --- a/src/compiler/glsl_types.cpp
> +++ b/src/compiler/glsl_types.cpp
> @@ -21,21 +21,22 @@
> * DEALINGS IN THE SOFTWARE.
> */
>
> #include <stdio.h>
> #include "main/macros.h"
> #include "compiler/glsl/glsl_parser_extras.h"
> #include "glsl_types.h"
> #include "util/hash_table.h"
>
>
> -mtx_t glsl_type::mutex = _MTX_INITIALIZER_NP;
> +mtx_t glsl_type::mem_mutex = _MTX_INITIALIZER_NP;
> +mtx_t glsl_type::hash_mutex = _MTX_INITIALIZER_NP;
Should we split this further? Since this is used for unrelated hash tables?
Either way this is better than the current code so:
Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
> hash_table *glsl_type::array_types = NULL;
> hash_table *glsl_type::record_types = NULL;
> hash_table *glsl_type::interface_types = NULL;
> hash_table *glsl_type::function_types = NULL;
> hash_table *glsl_type::subroutine_types = NULL;
> void *glsl_type::mem_ctx = NULL;
>
> void
> glsl_type::init_ralloc_type_ctx(void)
> {
> @@ -55,160 +56,160 @@ glsl_type::glsl_type(GLenum gl_type,
> vector_elements(vector_elements), matrix_columns(matrix_columns),
> length(0)
> {
> /* Values of these types must fit in the two bits of
> * glsl_type::sampled_type.
> */
> STATIC_ASSERT((unsigned(GLSL_TYPE_UINT) & 3) == unsigned(GLSL_TYPE_UINT));
> STATIC_ASSERT((unsigned(GLSL_TYPE_INT) & 3) == unsigned(GLSL_TYPE_INT));
> STATIC_ASSERT((unsigned(GLSL_TYPE_FLOAT) & 3) == unsigned(GLSL_TYPE_FLOAT));
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
>
> init_ralloc_type_ctx();
> assert(name != NULL);
> this->name = ralloc_strdup(this->mem_ctx, name);
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
>
> /* Neither dimension is zero or both dimensions are zero.
> */
> assert((vector_elements == 0) == (matrix_columns == 0));
> memset(& fields, 0, sizeof(fields));
> }
>
> glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type,
> enum glsl_sampler_dim dim, bool shadow, bool array,
> unsigned type, const char *name) :
> gl_type(gl_type),
> base_type(base_type),
> sampler_dimensionality(dim), sampler_shadow(shadow),
> sampler_array(array), sampled_type(type), interface_packing(0),
> interface_row_major(0), length(0)
> {
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
>
> init_ralloc_type_ctx();
> assert(name != NULL);
> this->name = ralloc_strdup(this->mem_ctx, name);
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
>
> memset(& fields, 0, sizeof(fields));
>
> matrix_columns = vector_elements = 1;
> }
>
> glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields,
> const char *name) :
> gl_type(0),
> base_type(GLSL_TYPE_STRUCT),
> sampler_dimensionality(0), sampler_shadow(0), sampler_array(0),
> sampled_type(0), interface_packing(0), interface_row_major(0),
> vector_elements(0), matrix_columns(0),
> length(num_fields)
> {
> unsigned int i;
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
>
> init_ralloc_type_ctx();
> assert(name != NULL);
> this->name = ralloc_strdup(this->mem_ctx, name);
> this->fields.structure = ralloc_array(this->mem_ctx,
> glsl_struct_field, length);
>
> for (i = 0; i < length; i++) {
> this->fields.structure[i] = fields[i];
> this->fields.structure[i].name = ralloc_strdup(this->fields.structure,
> fields[i].name);
> }
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
> }
>
> glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields,
> enum glsl_interface_packing packing,
> bool row_major, const char *name) :
> gl_type(0),
> base_type(GLSL_TYPE_INTERFACE),
> sampler_dimensionality(0), sampler_shadow(0), sampler_array(0),
> sampled_type(0), interface_packing((unsigned) packing),
> interface_row_major((unsigned) row_major),
> vector_elements(0), matrix_columns(0),
> length(num_fields)
> {
> unsigned int i;
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
>
> init_ralloc_type_ctx();
> assert(name != NULL);
> this->name = ralloc_strdup(this->mem_ctx, name);
> this->fields.structure = rzalloc_array(this->mem_ctx,
> glsl_struct_field, length);
> for (i = 0; i < length; i++) {
> this->fields.structure[i] = fields[i];
> this->fields.structure[i].name = ralloc_strdup(this->fields.structure,
> fields[i].name);
> }
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
> }
>
> glsl_type::glsl_type(const glsl_type *return_type,
> const glsl_function_param *params, unsigned num_params) :
> gl_type(0),
> base_type(GLSL_TYPE_FUNCTION),
> sampler_dimensionality(0), sampler_shadow(0), sampler_array(0),
> sampled_type(0), interface_packing(0), interface_row_major(0),
> vector_elements(0), matrix_columns(0),
> length(num_params)
> {
> unsigned int i;
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
>
> init_ralloc_type_ctx();
>
> this->fields.parameters = rzalloc_array(this->mem_ctx,
> glsl_function_param, num_params + 1);
>
> /* We store the return type as the first parameter */
> this->fields.parameters[0].type = return_type;
> this->fields.parameters[0].in = false;
> this->fields.parameters[0].out = true;
>
> /* We store the i'th parameter in slot i+1 */
> for (i = 0; i < length; i++) {
> this->fields.parameters[i + 1].type = params[i].type;
> this->fields.parameters[i + 1].in = params[i].in;
> this->fields.parameters[i + 1].out = params[i].out;
> }
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
> }
>
> glsl_type::glsl_type(const char *subroutine_name) :
> gl_type(0),
> base_type(GLSL_TYPE_SUBROUTINE),
> sampler_dimensionality(0), sampler_shadow(0), sampler_array(0),
> sampled_type(0), interface_packing(0), interface_row_major(0),
> vector_elements(1), matrix_columns(1),
> length(0)
> {
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
>
> init_ralloc_type_ctx();
> assert(subroutine_name != NULL);
> this->name = ralloc_strdup(this->mem_ctx, subroutine_name);
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
> }
>
> bool
> glsl_type::contains_sampler() const
> {
> if (this->is_array()) {
> return this->fields.array->contains_sampler();
> } else if (this->is_record() || this->is_interface()) {
> for (unsigned int i = 0; i < this->length; i++) {
> if (this->fields.structure[i].type->contains_sampler())
> @@ -453,23 +454,23 @@ glsl_type::glsl_type(const glsl_type *array, unsigned length) :
> * is represented by the size rather than the type.
> */
> this->gl_type = array->gl_type;
>
> /* Allow a maximum of 10 characters for the array size. This is enough
> * for 32-bits of ~0. The extra 3 are for the '[', ']', and terminating
> * NUL.
> */
> const unsigned name_length = strlen(array->name) + 10 + 3;
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
> char *const n = (char *) ralloc_size(this->mem_ctx, name_length);
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
>
> if (length == 0)
> snprintf(n, name_length, "%s[]", array->name);
> else {
> /* insert outermost dimensions in the correct spot
> * otherwise the dimension order will be backwards
> */
> const char *pos = strchr(array->name, '[');
> if (pos) {
> int idx = pos - array->name;
> @@ -875,43 +876,41 @@ const glsl_type *
> glsl_type::get_array_instance(const glsl_type *base, unsigned array_size)
> {
> /* Generate a name using the base type pointer in the key. This is
> * done because the name of the base type may not be unique across
> * shaders. For example, two shaders may have different record types
> * named 'foo'.
> */
> char key[128];
> snprintf(key, sizeof(key), "%p[%u]", (void *) base, array_size);
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::hash_mutex);
>
> if (array_types == NULL) {
> array_types = _mesa_hash_table_create(NULL, _mesa_key_hash_string,
> _mesa_key_string_equal);
> }
>
> const struct hash_entry *entry = _mesa_hash_table_search(array_types, key);
> if (entry == NULL) {
> - mtx_unlock(&glsl_type::mutex);
> const glsl_type *t = new glsl_type(base, array_size);
> - mtx_lock(&glsl_type::mutex);
>
> entry = _mesa_hash_table_insert(array_types,
> ralloc_strdup(mem_ctx, key),
> (void *) t);
> }
>
> assert(((glsl_type *) entry->data)->base_type == GLSL_TYPE_ARRAY);
> assert(((glsl_type *) entry->data)->length == array_size);
> assert(((glsl_type *) entry->data)->fields.array == base);
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::hash_mutex);
>
> return (glsl_type *) entry->data;
> }
>
>
> bool
> glsl_type::record_compare(const glsl_type *b, bool match_locations) const
> {
> if (this->length != b->length)
> return false;
> @@ -1033,109 +1032,103 @@ glsl_type::record_key_hash(const void *a)
> }
>
>
> const glsl_type *
> glsl_type::get_record_instance(const glsl_struct_field *fields,
> unsigned num_fields,
> const char *name)
> {
> const glsl_type key(fields, num_fields, name);
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::hash_mutex);
>
> if (record_types == NULL) {
> record_types = _mesa_hash_table_create(NULL, record_key_hash,
> record_key_compare);
> }
>
> const struct hash_entry *entry = _mesa_hash_table_search(record_types,
> &key);
> if (entry == NULL) {
> - mtx_unlock(&glsl_type::mutex);
> const glsl_type *t = new glsl_type(fields, num_fields, name);
> - mtx_lock(&glsl_type::mutex);
>
> entry = _mesa_hash_table_insert(record_types, t, (void *) t);
> }
>
> assert(((glsl_type *) entry->data)->base_type == GLSL_TYPE_STRUCT);
> assert(((glsl_type *) entry->data)->length == num_fields);
> assert(strcmp(((glsl_type *) entry->data)->name, name) == 0);
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::hash_mutex);
>
> return (glsl_type *) entry->data;
> }
>
>
> const glsl_type *
> glsl_type::get_interface_instance(const glsl_struct_field *fields,
> unsigned num_fields,
> enum glsl_interface_packing packing,
> bool row_major,
> const char *block_name)
> {
> const glsl_type key(fields, num_fields, packing, row_major, block_name);
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::hash_mutex);
>
> if (interface_types == NULL) {
> interface_types = _mesa_hash_table_create(NULL, record_key_hash,
> record_key_compare);
> }
>
> const struct hash_entry *entry = _mesa_hash_table_search(interface_types,
> &key);
> if (entry == NULL) {
> - mtx_unlock(&glsl_type::mutex);
> const glsl_type *t = new glsl_type(fields, num_fields,
> packing, row_major, block_name);
> - mtx_lock(&glsl_type::mutex);
>
> entry = _mesa_hash_table_insert(interface_types, t, (void *) t);
> }
>
> assert(((glsl_type *) entry->data)->base_type == GLSL_TYPE_INTERFACE);
> assert(((glsl_type *) entry->data)->length == num_fields);
> assert(strcmp(((glsl_type *) entry->data)->name, block_name) == 0);
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::hash_mutex);
>
> return (glsl_type *) entry->data;
> }
>
> const glsl_type *
> glsl_type::get_subroutine_instance(const char *subroutine_name)
> {
> const glsl_type key(subroutine_name);
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::hash_mutex);
>
> if (subroutine_types == NULL) {
> subroutine_types = _mesa_hash_table_create(NULL, record_key_hash,
> record_key_compare);
> }
>
> const struct hash_entry *entry = _mesa_hash_table_search(subroutine_types,
> &key);
> if (entry == NULL) {
> - mtx_unlock(&glsl_type::mutex);
> const glsl_type *t = new glsl_type(subroutine_name);
> - mtx_lock(&glsl_type::mutex);
>
> entry = _mesa_hash_table_insert(subroutine_types, t, (void *) t);
> }
>
> assert(((glsl_type *) entry->data)->base_type == GLSL_TYPE_SUBROUTINE);
> assert(strcmp(((glsl_type *) entry->data)->name, subroutine_name) == 0);
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::hash_mutex);
>
> return (glsl_type *) entry->data;
> }
>
>
> static bool
> function_key_compare(const void *a, const void *b)
> {
> const glsl_type *const key1 = (glsl_type *) a;
> const glsl_type *const key2 = (glsl_type *) b;
> @@ -1156,42 +1149,40 @@ function_key_hash(const void *a)
> (key->length + 1) * sizeof(*key->fields.parameters));
> }
>
> const glsl_type *
> glsl_type::get_function_instance(const glsl_type *return_type,
> const glsl_function_param *params,
> unsigned num_params)
> {
> const glsl_type key(return_type, params, num_params);
>
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::hash_mutex);
>
> if (function_types == NULL) {
> function_types = _mesa_hash_table_create(NULL, function_key_hash,
> function_key_compare);
> }
>
> struct hash_entry *entry = _mesa_hash_table_search(function_types, &key);
> if (entry == NULL) {
> - mtx_unlock(&glsl_type::mutex);
> const glsl_type *t = new glsl_type(return_type, params, num_params);
> - mtx_lock(&glsl_type::mutex);
>
> entry = _mesa_hash_table_insert(function_types, t, (void *) t);
> }
>
> const glsl_type *t = (const glsl_type *)entry->data;
>
> assert(t->base_type == GLSL_TYPE_FUNCTION);
> assert(t->length == num_params);
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::hash_mutex);
>
> return t;
> }
>
>
> const glsl_type *
> glsl_type::get_mul_type(const glsl_type *type_a, const glsl_type *type_b)
> {
> if (type_a == type_b) {
> return type_a;
> diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
> index 9da0cbc..55faac2 100644
> --- a/src/compiler/glsl_types.h
> +++ b/src/compiler/glsl_types.h
> @@ -142,42 +142,42 @@ struct glsl_type {
> * GLSL_TYPE_FLOAT, \c GLSL_TYPE_INT,
> * and \c GLSL_TYPE_UINT are valid.
> */
> unsigned interface_packing:2;
> unsigned interface_row_major:1;
>
> /* Callers of this ralloc-based new need not call delete. It's
> * easier to just ralloc_free 'mem_ctx' (or any of its ancestors). */
> static void* operator new(size_t size)
> {
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
>
> /* mem_ctx should have been created by the static members */
> assert(glsl_type::mem_ctx != NULL);
>
> void *type;
>
> type = ralloc_size(glsl_type::mem_ctx, size);
> assert(type != NULL);
>
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
>
> return type;
> }
>
> /* If the user *does* call delete, that's OK, we will just
> * ralloc_free in that case. */
> static void operator delete(void *type)
> {
> - mtx_lock(&glsl_type::mutex);
> + mtx_lock(&glsl_type::mem_mutex);
> ralloc_free(type);
> - mtx_unlock(&glsl_type::mutex);
> + mtx_unlock(&glsl_type::mem_mutex);
> }
>
> /**
> * \name Vector and matrix element counts
> *
> * For scalars, each of these values will be 1. For non-numeric types
> * these will be 0.
> */
> /*@{*/
> uint8_t vector_elements; /**< 1, 2, 3, or 4 vector elements. */
> @@ -813,21 +813,22 @@ struct glsl_type {
> /**
> * Check if the type interface is row major
> */
> bool get_interface_row_major() const
> {
> return (bool) interface_row_major;
> }
>
> private:
>
> - static mtx_t mutex;
> + static mtx_t mem_mutex;
> + static mtx_t hash_mutex;
>
> /**
> * ralloc context for all glsl_type allocations
> *
> * Set on the first call to \c glsl_type::new.
> */
> static void *mem_ctx;
>
> void init_ralloc_type_ctx(void);
>
>
More information about the mesa-dev
mailing list