[Spice-devel] [PATCH 3/9] server: remove worker thread creation from dispatcher
Christophe Fergeau
cfergeau at redhat.com
Mon Oct 26 04:14:52 PDT 2015
Hey, this patch breaks starting VMs through virsh for me:
$ virsh start win7-home-pr
erreur :Impossible de démarrer le domaine win7-home-pr
erreur :internal error: early end of file from monitor: possible problem:
((null):27254): Spice-ERROR **: spice_timer_queue.c:226:spice_timer_queue_get_timeout_ms: assertion `queue != NULL' failed
Thread 8 (Thread 0x7fc404d98700 (LWP 27259)):
#0 0x00007fc4204cc89d in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1 0x00007fc4204c69cd in __GI___pthread_mutex_lock (mutex=mutex at entry=0x55c29555c2e0 <qemu_global_mutex>) at ../nptl/pthread_mutex_lock.c:81
#2 0x000055c2950567a9 in qemu_mutex_lock (mutex=mutex at entry=0x55c29555c2e0 <qemu_global_mutex>) at util/qemu-thread-posix.c:73
#3 0x000055c294d97991 in qemu_mutex_lock_iothread () at /usr/src/debug/qemu-2.4.0.1/cpus.c:1170
#4 0x000055c29506410e in call_rcu_thread (opaque=<optimized out>) at util/rcu.c:243
#5 0x00007fc4204c460a in start_thread (arg=0x7fc404d98700) at pthread_create.c:334
#6 0x00007fc411d48bbd in clone () at ../sysdeps/unix/sysv/linux/x86_64/c
(haven't tried to figure out what's going on)
Christophe
On Thu, Oct 22, 2015 at 10:49:33AM -0500, Jonathon Jongsma wrote:
> On Wed, 2015-10-21 at 08:21 -0400, 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;
> > > +}
> > > +
> > > 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"
> > >
> > > typedef struct Dispatcher Dispatcher;
> > >
> >
> > This include can be removed
> >
> > > @@ -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);
> > >
> > > /*
> > > * 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);
> > > -
> > > + // TODO: reference and free
> > > + RedWorker *worker = red_worker_new(&init_data);
> > > + red_worker_run(worker);
> > > 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"
> >
> > Even this include
> >
> > > @@ -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));
> > > -}
> > > -
> > > 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;
> > > +}
> > > 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
> >
> > Beside these small changes I agree with this patch.
>
> Agree, ACK from me as well.
>
>
> >
> > Frediano
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151026/028fceab/attachment.sig>
More information about the Spice-devel
mailing list