On Tuesday, 2018-07-03 19:16:11 -0400, Marek Olšák wrote:
> From: Marek Olšák <marek.ol...@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.engest...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
