Re: [PATCH 3/9] server: remove worker thread creation from dispatcher

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]