[Spice-devel] [PATCH 3/9] server: remove worker thread creation from dispatcher
Uri Lublin
ulublin at redhat.com
Wed Oct 28 06:15:36 PDT 2015
On 10/21/2015 03:00 PM, Frediano Ziglio wrote:
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
>
> ---
> server/dispatcher.c | 13 +++++++++++++
> server/dispatcher.h | 2 ++
> server/red_dispatcher.c | 21 +++------------------
> server/red_dispatcher.h | 6 +-----
> server/red_worker.c | 33 ++++++++++++++++++++++++++++++---
> server/red_worker.h | 5 ++++-
> 6 files changed, 53 insertions(+), 27 deletions(-)
>
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index d6c03ca..d334117 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -32,6 +32,7 @@
> #include "common/mem.h"
> #include "common/spice_common.h"
> #include "dispatcher.h"
> +#include "red_dispatcher.h"
>
> //#define DEBUG_DISPATCHER
>
> @@ -200,6 +201,18 @@ unlock:
> pthread_mutex_unlock(&dispatcher->lock);
> }
>
> +uint32_t dispatcher_read_message(Dispatcher *dispatcher)
> +{
> + uint32_t message;
> +
> + spice_return_val_if_fail(dispatcher, 0);
> + spice_return_val_if_fail(dispatcher->send_fd != -1, 0);
> +
> + receive_data(dispatcher->send_fd, &message, sizeof(message));
> +
> + return message;
> +}
> +
Hi,
This change can go in a separate patch. (mark [1])
> void dispatcher_register_async_done_callback(
> Dispatcher *dispatcher,
> dispatcher_handle_async_done handler)
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index c3e7c74..8882532 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -19,6 +19,7 @@
> #define DISPATCHER_H
>
> #include <spice.h>
> +#include "spice_server_utils.h"
Why do we need this header here ?
>
> typedef struct Dispatcher Dispatcher;
>
> @@ -63,6 +64,7 @@ struct Dispatcher {
> */
> void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
> void *payload);
> +uint32_t dispatcher_read_message(Dispatcher *dispatcher);
same as [1]
>
> /*
> * dispatcher_init
> diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> index 7ad860c..0bc853d 100644
> --- a/server/red_dispatcher.c
> +++ b/server/red_dispatcher.c
> @@ -56,7 +56,6 @@ struct RedDispatcher {
> QXLWorker base;
> QXLInstance *qxl;
> Dispatcher dispatcher;
> - pthread_t worker_thread;
> uint32_t pending;
> int primary_active;
> int x_res;
> @@ -1064,14 +1063,10 @@ static RedChannel *red_dispatcher_cursor_channel_create(RedDispatcher *dispatche
> void red_dispatcher_init(QXLInstance *qxl)
> {
> RedDispatcher *red_dispatcher;
> - RedWorkerMessage message;
> WorkerInitData init_data;
> QXLDevInitInfo init_info;
> - int r;
> RedChannel *display_channel;
> RedChannel *cursor_channel;
> - sigset_t thread_sig_mask;
> - sigset_t curr_sig_mask;
> ClientCbs client_cbs = { NULL, };
>
> spice_return_if_fail(qxl->st->dispatcher == NULL);
> @@ -1135,19 +1130,9 @@ void red_dispatcher_init(QXLInstance *qxl)
>
> num_active_workers = 1;
>
> - sigfillset(&thread_sig_mask);
> - sigdelset(&thread_sig_mask, SIGILL);
> - sigdelset(&thread_sig_mask, SIGFPE);
> - sigdelset(&thread_sig_mask, SIGSEGV);
> - pthread_sigmask(SIG_SETMASK, &thread_sig_mask, &curr_sig_mask);
> - if ((r = pthread_create(&red_dispatcher->worker_thread, NULL, red_worker_main, &init_data))) {
> - spice_error("create thread failed %d", r);
> - }
> - pthread_sigmask(SIG_SETMASK, &curr_sig_mask, NULL);
> -
> - read_message(red_dispatcher->dispatcher.send_fd, &message);
> - spice_assert(message == RED_WORKER_MESSAGE_READY);
Note, here it's spice_assert. mark [2]
> -
> + // TODO: reference and free
> + RedWorker *worker = red_worker_new(&init_data);
> + red_worker_run(worker);
return value is not checked (related to [2])
> display_channel = red_dispatcher_display_channel_create(red_dispatcher);
>
> if (display_channel) {
> diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
> index dbe65d1..2121007 100644
> --- a/server/red_dispatcher.h
> +++ b/server/red_dispatcher.h
> @@ -18,6 +18,7 @@
> #ifndef _H_RED_DISPATCHER
> #define _H_RED_DISPATCHER
>
> +#include <unistd.h>
> #include <errno.h>
> #include "spice_server_utils.h"
> #include "red_channel.h"
> @@ -83,11 +84,6 @@ static inline void receive_data(int fd, void *in_buf, int n)
> } while (n);
> }
>
> -static inline void read_message(int fd, RedWorkerMessage *message)
> -{
> - receive_data(fd, message, sizeof(RedWorkerMessage));
> -}
> -
see [1]
> enum {
> RED_WORKER_MESSAGE_NOP,
> RED_WORKER_MESSAGE_UPDATE,
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 225c272..daa233b 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -868,6 +868,7 @@ typedef struct ItemTrace {
> #define NUM_CURSORS 100
>
> typedef struct RedWorker {
> + pthread_t thread;
> DisplayChannel *display_channel;
> CursorChannel *cursor_channel;
> QXLInstance *qxl;
> @@ -11849,7 +11850,7 @@ static void handle_dev_input(int fd, int event, void *opaque)
> dispatcher_handle_recv_read(red_dispatcher_get_dispatcher(worker->red_dispatcher));
> }
>
> -static RedWorker* red_worker_new(WorkerInitData *init_data)
> +RedWorker* red_worker_new(WorkerInitData *init_data)
> {
> RedWorker *worker = spice_new0(RedWorker, 1);
> RedWorkerMessage message;
> @@ -11954,14 +11955,15 @@ static void red_display_cc_free_glz_drawables(RedChannelClient *rcc)
> red_display_handle_glz_drawables_to_free(dcc);
> }
>
> -SPICE_GNUC_NORETURN void *red_worker_main(void *arg)
> +SPICE_GNUC_NORETURN static void *red_worker_main(void *arg)
> {
> - RedWorker *worker = red_worker_new(arg);
> + RedWorker *worker = arg;
>
> spice_info("begin");
> spice_assert(MAX_PIPE_SIZE > WIDE_CLIENT_ACK_WINDOW &&
> MAX_PIPE_SIZE > NARROW_CLIENT_ACK_WINDOW); //ensure wakeup by ack message
>
> + // TODO: call once unconditionnally
> if (pthread_getcpuclockid(pthread_self(), &clock_id)) {
> spice_error("pthread_getcpuclockid failed");
> }
> @@ -12028,3 +12030,28 @@ SPICE_GNUC_NORETURN void *red_worker_main(void *arg)
>
> spice_warn_if_reached();
> }
> +
> +bool red_worker_run(RedWorker *worker)
> +{
> + uint32_t message;
> + sigset_t thread_sig_mask;
> + sigset_t curr_sig_mask;
> + int r;
> +
> + spice_return_val_if_fail(worker, FALSE);
> + spice_return_val_if_fail(!worker->thread, FALSE);
> +
> + sigfillset(&thread_sig_mask);
> + sigdelset(&thread_sig_mask, SIGILL);
> + sigdelset(&thread_sig_mask, SIGFPE);
> + sigdelset(&thread_sig_mask, SIGSEGV);
> + pthread_sigmask(SIG_SETMASK, &thread_sig_mask, &curr_sig_mask);
> + if ((r = pthread_create(&worker->thread, NULL, red_worker_main, worker))) {
> + spice_error("create thread failed %d", r);
> + }
> + pthread_sigmask(SIG_SETMASK, &curr_sig_mask, NULL);
> +
> + message = dispatcher_read_message(red_dispatcher_get_dispatcher(worker->red_dispatcher));
> +
> + return message == RED_WORKER_MESSAGE_READY;
See [2] -- returning false
> +}
> diff --git a/server/red_worker.h b/server/red_worker.h
> index c71e9c8..c935e0a 100644
> --- a/server/red_worker.h
> +++ b/server/red_worker.h
> @@ -37,6 +37,8 @@ enum {
> RED_RENDERER_LAST
> };
>
> +typedef struct RedWorker RedWorker;
> +
> typedef struct WorkerInitData {
> struct QXLInstance *qxl;
> int id;
> @@ -56,6 +58,7 @@ typedef struct WorkerInitData {
> RedDispatcher *red_dispatcher;
> } WorkerInitData;
>
> -void *red_worker_main(void *arg);
> +RedWorker* red_worker_new(WorkerInitData *init_data);
> +bool red_worker_run(RedWorker *worker);
>
> #endif
>
If splitting this patch is not an option we can accept it
and add patches on top of it.
Ack.
- Uri
More information about the Spice-devel
mailing list