[Mesa-dev] [PATCH 1/9] glsl: fix a race condition when inserting new types
Nicolai Hähnle
nhaehnle at gmail.com
Mon May 15 23:41:21 UTC 2017
On 16.05.2017 01:32, Timothy Arceri wrote:
> 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?
I thought I'd err on the side of keeping things simple for now. We can
always split it further if we get a full implementation of
ARB_parallel_shader_compile and it turns out to be a source of lock
contention.
> Either way this is better than the current code so:
>
> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
Thanks!
Cheers,
Nicolai
>
>> 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);
>>
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
More information about the mesa-dev
mailing list