Hi, yes, actually it breaks also small checks using replay utility (I should have caught before). Posted a patch. I think this came from the problem how to handle worker <-> timeout/watch relationship. In the current code it uses the queue attached to the worker using the thread identifier (pthread_self). The patch fix the issue however I did some check on the new loop and at the beginning it seems that everything is fine and the proper g_source_attach function is called. However this is not true for timers where a g_timeout_add function is called. According to glib documentation (https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#g-timeout-add) the timeout is attached to the main context (not to be confused with worker->main_context!). This lead to missing timers and possibly leaks due to timers all attached to main context not handled (or handled by different thread, in this case thread race condition will happen). Frediano > > 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@entry=0x55c29555c2e0 <qemu_global_mutex>) at > ../nptl/pthread_mutex_lock.c:81 > #2 0x000055c2950567a9 in qemu_mutex_lock (mutex=mutex@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@xxxxxxxxx> > > > > > > > > --- > > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel > > _______________________________________________ > > Spice-devel mailing list > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/spice-devel > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel