On 16.05.2017 01:32, Timothy Arceri wrote:
On 15/05/17 19:27, Nicolai Hähnle wrote:
From: Nicolai Hähnle <nicolai.haeh...@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 <tarc...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to