Re: [Mesa-dev] [PATCH] util/u_queue: fix a deadlock in util_queue_finish

[Nicolai Hähnle]

Nice catch!

Reviewed-by: Nicolai Hähnle <nicolai.haeh...@amd.com>

On 24.04.2018 23:06, Marek Olšák wrote:

From: Marek Olšák <marek.ol...@amd.com>

Cc: 18.0 18.1 <mesa-sta...@lists.freedesktop.org>
---
  src/util/u_queue.c | 9 +++++++++
  src/util/u_queue.h | 1 +
  2 files changed, 10 insertions(+)

diff --git a/src/util/u_queue.c b/src/util/u_queue.c
index dba23f96456..da513fd9cc5 100644
--- a/src/util/u_queue.c
+++ b/src/util/u_queue.c
@@ -304,20 +304,21 @@ util_queue_init(struct util_queue *queue,
     queue->flags = flags;
     queue->num_threads = num_threads;
     queue->max_jobs = max_jobs;

queue->jobs = (struct util_queue_job*)

                   calloc(max_jobs, sizeof(struct util_queue_job));
     if (!queue->jobs)
        goto fail;

(void) mtx_init(&queue->lock, mtx_plain);

+   (void) mtx_init(&queue->finish_lock, mtx_plain);

queue->num_queued = 0;

     cnd_init(&queue->has_queued_cond);
     cnd_init(&queue->has_space_cond);

queue->threads = (thrd_t*) calloc(num_threads, sizeof(thrd_t));

     if (!queue->threads)
        goto fail;

/* start threads */

@@ -391,20 +392,21 @@ util_queue_killall_and_wait(struct util_queue *queue)
  }

void

  util_queue_destroy(struct util_queue *queue)
  {
     util_queue_killall_and_wait(queue);
     remove_from_atexit_list(queue);

cnd_destroy(&queue->has_space_cond);

     cnd_destroy(&queue->has_queued_cond);
+   mtx_destroy(&queue->finish_lock);
     mtx_destroy(&queue->lock);
     free(queue->jobs);
     free(queue->threads);
  }

void

  util_queue_add_job(struct util_queue *queue,
                     void *job,
                     struct util_queue_fence *fence,
                     util_queue_execute_func execute,
@@ -522,29 +524,36 @@ util_queue_finish_execute(void *data, int num_thread)
   * Wait until all previously added jobs have completed.
   */
  void
  util_queue_finish(struct util_queue *queue)
  {
     util_barrier barrier;
     struct util_queue_fence *fences = malloc(queue->num_threads * 
sizeof(*fences));

util_barrier_init(&barrier, queue->num_threads); + /* If 2 threads were adding jobs for 2 different barries at the same time,

+    * a deadlock would happen, because 1 barrier requires that all threads
+    * wait for it exclusively.
+    */
+   mtx_lock(&queue->finish_lock);
+
     for (unsigned i = 0; i < queue->num_threads; ++i) {
        util_queue_fence_init(&fences[i]);
        util_queue_add_job(queue, &barrier, &fences[i], 
util_queue_finish_execute, NULL);
     }

for (unsigned i = 0; i < queue->num_threads; ++i) {

        util_queue_fence_wait(&fences[i]);
        util_queue_fence_destroy(&fences[i]);
     }
+   mtx_unlock(&queue->finish_lock);

util_barrier_destroy(&barrier); free(fences);

  }

int64_t

  util_queue_get_thread_time_nano(struct util_queue *queue, unsigned 
thread_index)
  {
     /* Allow some flexibility by not raising an error. */
diff --git a/src/util/u_queue.h b/src/util/u_queue.h
index d49f713e6ad..d702c4bce8d 100644
--- a/src/util/u_queue.h
+++ b/src/util/u_queue.h
@@ -193,20 +193,21 @@ typedef void (*util_queue_execute_func)(void *job, int 
thread_index);
  struct util_queue_job {
     void *job;
     struct util_queue_fence *fence;
     util_queue_execute_func execute;
     util_queue_execute_func cleanup;
  };

/* Put this into your context. */

  struct util_queue {
     const char *name;
+   mtx_t finish_lock; /* only for util_queue_finish */
     mtx_t lock;
     cnd_t has_queued_cond;
     cnd_t has_space_cond;
     thrd_t *threads;
     unsigned flags;
     int num_queued;
     unsigned num_threads;
     int kill_threads;
     int max_jobs;
     int write_idx, read_idx; /* ring buffer pointers */

--
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