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

[Nicolai Hähnle]

Nice catch!

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

On 24.04.2018 23:06, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
> 
> Cc: 18.0 18.1 <mesa-stable at 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.