[Spice-devel] [PATCH 3/9] server: remove worker thread creation from dispatcher
Frediano Ziglio
fziglio at redhat.com
Wed Oct 28 08:37:07 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
Hi,
the patch is already merged.
Frediano
More information about the Spice-devel
mailing list