[Mesa-dev] [PATCH 1/4] gallium/u_queue: fix random crashes when the app calls exit()

[Nicolai Hähnle]

On 20.02.2017 18:44, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> This fixes:
>     vdpauinfo: ../lib/CodeGen/TargetPassConfig.cpp:579: virtual void
>     llvm::TargetPassConfig::addMachinePasses(): Assertion `TPI && IPI &&
>     "Pass ID not registered!"' failed.
>
> Cc: 13.0 17.0 <mesa-stable at lists.freedesktop.org>
> ---
>  src/gallium/auxiliary/util/u_queue.c | 88 +++++++++++++++++++++++++++++++++++-
>  src/gallium/auxiliary/util/u_queue.h |  3 ++
>  2 files changed, 89 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/auxiliary/util/u_queue.c b/src/gallium/auxiliary/util/u_queue.c
> index 4da5d8e..89fb235 100644
> --- a/src/gallium/auxiliary/util/u_queue.c
> +++ b/src/gallium/auxiliary/util/u_queue.c
> @@ -22,20 +22,94 @@
>   * The above copyright notice and this permission notice (including the
>   * next paragraph) shall be included in all copies or substantial portions
>   * of the Software.
>   */
>
>  #include "u_queue.h"
>  #include "u_memory.h"
>  #include "u_string.h"
>  #include "os/os_time.h"
>
> +static void util_queue_killall_and_wait(struct util_queue *queue);
> +
> +/****************************************************************************
> + * Wait for all queues to assert idle when exit() is called.
> + *
> + * Otherwise, C++ static variable destructors can be called while threads
> + * are using the static variables.
> + */
> +
> +static once_flag atexit_once_flag = ONCE_FLAG_INIT;
> +static struct util_queue *first_queue;
> +pipe_static_mutex(exit_mutex);
> +
> +static void
> +atexit_handler(void)
> +{
> +   struct util_queue *queue;
> +
> +   pipe_mutex_lock(exit_mutex);
> +   queue = first_queue;
> +   /* Wait for all queues to assert idle. */
> +   while (queue) {
> +      util_queue_killall_and_wait(queue);
> +      queue = queue->next;
> +   }
> +   pipe_mutex_unlock(exit_mutex);
> +}
> +
> +static void
> +call_atexit(void)
> +{
> +   atexit(atexit_handler);
> +}
> +
> +static void
> +add_to_atexit_list(struct util_queue *queue)
> +{
> +   call_once(&atexit_once_flag, call_atexit);
> +
> +   pipe_mutex_lock(exit_mutex);
> +   queue->next = first_queue;
> +   first_queue = queue;
> +   pipe_mutex_unlock(exit_mutex);
> +}
> +
> +static void
> +remove_from_atexit_list(struct util_queue *queue)
> +{
> +   struct util_queue *iter;
> +
> +   pipe_mutex_lock(exit_mutex);
> +   assert(first_queue);
> +   iter = first_queue;
> +
> +   /* Search the list except the first one. */
> +   while (iter->next) {
> +      if (iter->next == queue) {
> +         iter->next = queue->next;
> +         pipe_mutex_unlock(exit_mutex);
> +         return;
> +      }
> +      iter = iter->next;
> +   }

Please use double pointers to simplify this code, i.e.

    struct util_queue **iter = &first_queue;

    while (*iter != queue)
       iter = &(*iter)->next;

    *iter = queue->next;

Actually, I think it would even be fine to use util/list.h to simplify 
the code, but I'm fine with either option.


> +
> +   /* It must be the first one. */
> +   assert(first_queue == queue);
> +   first_queue = first_queue->next;
> +   pipe_mutex_unlock(exit_mutex);
> +}
> +
> +/****************************************************************************
> + * util_queue implementation
> + */
> +
>  static void
>  util_queue_fence_signal(struct util_queue_fence *fence)
>  {
>     pipe_mutex_lock(fence->mutex);
>     fence->signalled = true;
>     pipe_condvar_broadcast(fence->cond);
>     pipe_mutex_unlock(fence->mutex);
>  }
>
>  void
> @@ -97,20 +171,21 @@ static PIPE_THREAD_ROUTINE(util_queue_thread_func, input)
>     }
>
>     /* signal remaining jobs before terminating */
>     pipe_mutex_lock(queue->lock);
>     while (queue->jobs[queue->read_idx].job) {
>        util_queue_fence_signal(queue->jobs[queue->read_idx].fence);
>
>        queue->jobs[queue->read_idx].job = NULL;
>        queue->read_idx = (queue->read_idx + 1) % queue->max_jobs;
>     }
> +   queue->num_queued = 0; /* reset this when exiting the thread */
>     pipe_mutex_unlock(queue->lock);
>     return 0;
>  }
>
>  bool
>  util_queue_init(struct util_queue *queue,
>                  const char *name,
>                  unsigned max_jobs,
>                  unsigned num_threads)
>  {
> @@ -150,49 +225,58 @@ util_queue_init(struct util_queue *queue,
>           if (i == 0) {
>              /* no threads created, fail */
>              goto fail;
>           } else {
>              /* at least one thread created, so use it */
>              queue->num_threads = i+1;
>              break;
>           }
>        }
>     }
> +
> +   add_to_atexit_list(queue);
>     return true;
>
>  fail:
>     FREE(queue->threads);
>
>     if (queue->jobs) {
>        pipe_condvar_destroy(queue->has_space_cond);
>        pipe_condvar_destroy(queue->has_queued_cond);
>        pipe_mutex_destroy(queue->lock);
>        FREE(queue->jobs);
>     }
>     /* also util_queue_is_initialized can be used to check for success */
>     memset(queue, 0, sizeof(*queue));
>     return false;
>  }
>
> -void
> -util_queue_destroy(struct util_queue *queue)
> +static void
> +util_queue_killall_and_wait(struct util_queue *queue)
>  {
>     unsigned i;
>
>     /* Signal all threads to terminate. */
>     pipe_mutex_lock(queue->lock);
>     queue->kill_threads = 1;
>     pipe_condvar_broadcast(queue->has_queued_cond);
>     pipe_mutex_unlock(queue->lock);
>
>     for (i = 0; i < queue->num_threads; i++)
>        pipe_thread_wait(queue->threads[i]);
> +}
> +
> +void
> +util_queue_destroy(struct util_queue *queue)
> +{
> +   remove_from_atexit_list(queue);
> +   util_queue_killall_and_wait(queue);

I think those two calls should be the other way around, otherwise the 
problem reappears if another thread calls exit() in between.

Thanks,
Nicolai

>
>     pipe_condvar_destroy(queue->has_space_cond);
>     pipe_condvar_destroy(queue->has_queued_cond);
>     pipe_mutex_destroy(queue->lock);
>     FREE(queue->jobs);
>     FREE(queue->threads);
>  }
>
>  void
>  util_queue_fence_init(struct util_queue_fence *fence)
> diff --git a/src/gallium/auxiliary/util/u_queue.h b/src/gallium/auxiliary/util/u_queue.h
> index 351315c..4ddba33 100644
> --- a/src/gallium/auxiliary/util/u_queue.h
> +++ b/src/gallium/auxiliary/util/u_queue.h
> @@ -59,20 +59,23 @@ struct util_queue {
>     pipe_mutex lock;
>     pipe_condvar has_queued_cond;
>     pipe_condvar has_space_cond;
>     pipe_thread *threads;
>     int num_queued;
>     unsigned num_threads;
>     int kill_threads;
>     int max_jobs;
>     int write_idx, read_idx; /* ring buffer pointers */
>     struct util_queue_job *jobs;
> +
> +   /* for cleanup at exit() */
> +   struct util_queue *next; /* protected by exit_mutex */
>  };
>
>  bool util_queue_init(struct util_queue *queue,
>                       const char *name,
>                       unsigned max_jobs,
>                       unsigned num_threads);
>  void util_queue_destroy(struct util_queue *queue);
>  void util_queue_fence_init(struct util_queue_fence *fence);
>  void util_queue_fence_destroy(struct util_queue_fence *fence);
>
>