[Mesa-dev] [PATCH 3/4] util/queue: add a process name into a thread name

[Eric Engestrom]

On Tuesday, 2018-07-03 19:16:11 -0400, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
> 
> ---
>  src/util/u_queue.c | 35 +++++++++++++++++++++++++++++++++--
>  src/util/u_queue.h |  2 +-
>  2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/u_queue.c b/src/util/u_queue.c
> index da513fd9cc5..6c92e8140a1 100644
> --- a/src/util/u_queue.c
> +++ b/src/util/u_queue.c
> @@ -24,20 +24,21 @@
>   * of the Software.
>   */
>  
>  #include "u_queue.h"
>  
>  #include <time.h>
>  
>  #include "util/os_time.h"
>  #include "util/u_string.h"
>  #include "util/u_thread.h"
> +#include "process.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.
>   */
>  
> @@ -233,21 +234,21 @@ struct thread_input {
>  static int
>  util_queue_thread_func(void *input)
>  {
>     struct util_queue *queue = ((struct thread_input*)input)->queue;
>     int thread_index = ((struct thread_input*)input)->thread_index;
>  
>     free(input);
>  
>     if (queue->name) {
>        char name[16];
> -      util_snprintf(name, sizeof(name), "%s:%i", queue->name, thread_index);
> +      util_snprintf(name, sizeof(name), "%s%i", queue->name, thread_index);
>        u_thread_setname(name);
>     }
>  
>     while (1) {
>        struct util_queue_job job;
>  
>        mtx_lock(&queue->lock);
>        assert(queue->num_queued >= 0 && queue->num_queued <= queue->max_jobs);
>  
>        /* wait if the queue is empty */
> @@ -292,22 +293,52 @@ util_queue_thread_func(void *input)
>  
>  bool
>  util_queue_init(struct util_queue *queue,
>                  const char *name,
>                  unsigned max_jobs,
>                  unsigned num_threads,
>                  unsigned flags)
>  {
>     unsigned i;
>  
> +   /* Form the thread name from process_name and name, limited to 13
> +    * characters. Characters 14-15 are reserved for the thread number.
> +    * Character 16 should be 0. Final form: "process:name12"
> +    *
> +    * If name is too long, it's truncated. If any space is left, the process
> +    * name fills it.
> +    */
> +   const char *process_name = util_get_process_name();
> +   unsigned process_len = process_name ? strlen(process_name) : 0;
> +   unsigned name_len = strlen(name);
> +   const unsigned max_chars = 13;

Let's avoid magic numbers :)

   const unsigned max_chars = sizeof(queue->name) - 1;

> +
> +   name_len = MIN2(name_len, max_chars);
> +
> +   /* See if there is any space left for the process name, add + 1 for
> +    * the colon. */
> +   if (max_chars > name_len + 1)
> +      process_len = MIN2(process_len, max_chars - name_len - 1);
> +   else
> +      process_len = 0;

I think doing the math only once is clearer:

   /* See if there is any space left for the process name; reserve 1 for
    * the colon. */
   process_len = MIN2(process_len, max_chars - name_len - 1);
   if (process_len < 0)
     process_len = 0;

> +
>     memset(queue, 0, sizeof(*queue));
> -   queue->name = name;
> +
> +   if (process_len) {
> +      memcpy(queue->name, process_name, process_len);
> +      queue->name[process_len] = ':';
> +      memcpy(queue->name + process_len + 1, name, name_len);
> +      queue->name[process_len + 1 + name_len] = 0;

If you truncate the process name:

   process_name[process_len] = 0;

Then this `if (process_len)` branch can be a simple:

   snprintf(queue->name, sizeof(queue->name), "%s:%s", process_name, name);

> +   } else {
> +      snprintf(queue->name, max_chars + 1, "%s", name);

nit: replace `max_chars + 1` with `sizeof(queue-name)` :)


With the magic number removed:
Reviewed-by: Eric Engestrom <eric.engestrom at intel.com>

(I'll leave it up to you for the rest)

> +   }
> +
>     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);
> diff --git a/src/util/u_queue.h b/src/util/u_queue.h
> index d702c4bce8d..3c21ef3bc7b 100644
> --- a/src/util/u_queue.h
> +++ b/src/util/u_queue.h
> @@ -192,21 +192,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;
> +   char name[14];
>     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;
> -- 
> 2.17.1
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev