Message ID | 1300290769-31155-3-git-send-email-alevy@redhat.com |
---|---|
State | New |
Headers | show |
Looks good now, ack: Acked-by: Hans de Goede <hdegoede@redhat.com> On 03/16/2011 04:52 PM, Alon Levy wrote: > From: Uri Lublin<uril@redhat.com> > > This patch and the next drop the requirement to lose the global qemu > mutex during dispatcher calls. This patch enables it, the next drops > the unlock/lock pairs around dispatcher calls. > > The current solution of dropping the locks is buggy: > * it allows multiple dispatcher calls from two vcpu threads, the > dispatcher doesn't handle that by design (single fd, not locked, can't > handle writes from two threads) > * it requires us to keep track of cpu_single_env, which is magic. > > The solution implemented in this patch and the next (the next just > drops the locks, this patch allows that to work): > * the only operation that needed locking was qemu_create_simple_update, > * it required locking because it was called from the spice-server thread. > * do it in the iothread by reusing the existing pipe used for set_irq. > > The current flow implemented is now: > spice-server thread: > qxl.c:interface_get_command (called either by polling or from wakeup) > if update!=NULL: > waiting_for_update=0 > update=NULL > return update > else: > if not waiting_for_update: > waiting_for_update=1 > write to pipe, which is read by iothread (main thread) > > iothread: > wakeup from select, > qxl.c:pipe_read > update=qemu_create_simple_update() > wakeup spice-server thread by calling d.worker->wakeup(d.worker) > --- > hw/qxl.c | 81 +++++++++++++++++++++++++++++++++++++++------------ > ui/spice-display.c | 75 +++++++++++++++++++++++++++++++++++++++++++---- > ui/spice-display.h | 16 ++++++++++ > 3 files changed, 146 insertions(+), 26 deletions(-) > > diff --git a/hw/qxl.c b/hw/qxl.c > index 201698f..64580f1 100644 > --- a/hw/qxl.c > +++ b/hw/qxl.c > @@ -340,7 +340,6 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info) > static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) > { > PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); > - SimpleSpiceUpdate *update; > QXLCommandRing *ring; > QXLCommand *cmd; > int notify; > @@ -348,16 +347,25 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) > switch (qxl->mode) { > case QXL_MODE_VGA: > dprint(qxl, 2, "%s: vga\n", __FUNCTION__); > - update = qemu_spice_create_update(&qxl->ssd); > - if (update == NULL) { > - return false; > + if (qxl_vga_mode_get_command(&qxl->ssd, ext)) { > + qxl_log_command(qxl, "vga", ext); > + return true; > } > - *ext = update->ext; > - qxl_log_command(qxl, "vga", ext); > - return true; > + return false; > case QXL_MODE_COMPAT: > case QXL_MODE_NATIVE: > case QXL_MODE_UNDEFINED: > + /* flush any existing updates that we didn't send to the guest. > + * since update != NULL it means the server didn't get it, and > + * because we changed mode to != QXL_MODE_VGA, it won't. */ > + if (qxl->ssd.update != NULL) { > + if (qxl->ssd.update != QXL_EMPTY_UPDATE) { > + qemu_spice_destroy_update(&qxl->ssd, qxl->ssd.update); > + } > + qxl->ssd.update = NULL; > + qxl->ssd.waiting_for_update = 0; > + } > + /* */ > dprint(qxl, 2, "%s: %s\n", __FUNCTION__, > qxl->cmdflags ? "compat" : "native"); > ring =&qxl->ram->cmd_ring; > @@ -1057,17 +1065,50 @@ static void qxl_map(PCIDevice *pci, int region_num, > > static void pipe_read(void *opaque) > { > - PCIQXLDevice *d = opaque; > - char dummy; > - int len; > - > - do { > - len = read(d->ssd.pipe[0],&dummy, sizeof(dummy)); > - } while (len == sizeof(dummy)); > - qxl_set_irq(d); > + SimpleSpiceDisplay *ssd = opaque; > + unsigned char cmd; > + int len, set_irq = 0; > + int create_update = 0; > + > + while (1) { > + cmd = 0; > + len = read(ssd->pipe[0],&cmd, sizeof(cmd)); > + if (len< 0) { > + if (errno == EINTR) { > + continue; > + } > + if (errno == EAGAIN) { > + break; > + } > + perror("qxl: pipe_read: read failed"); > + break; > + } > + switch (cmd) { > + case QXL_SERVER_SET_IRQ: > + set_irq = 1; > + break; > + case QXL_SERVER_CREATE_UPDATE: > + create_update = 1; > + break; > + default: > + fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd); > + abort(); > + } > + } > + /* no need to do either operation more than once */ > + if (create_update) { > + assert(ssd->update == NULL); > + ssd->update = qemu_spice_create_update(ssd); > + if (ssd->update == NULL) { > + ssd->update = QXL_EMPTY_UPDATE; > + } > + ssd->worker->wakeup(ssd->worker); > + } > + if (set_irq) { > + qxl_set_irq(container_of(ssd, PCIQXLDevice, ssd)); > + } > } > > -/* called from spice server thread context only */ > static void qxl_send_events(PCIQXLDevice *d, uint32_t events) > { > uint32_t old_pending; > @@ -1082,9 +1123,11 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events) > /* running in io_thread thread */ > qxl_set_irq(d); > } else { > - if (write(d->ssd.pipe[1], d, 1) != 1) { > - dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__); > - } > + /* called from spice-server thread */ > + int ret; > + unsigned char ack = QXL_SERVER_SET_IRQ; > + ret = write(d->ssd.pipe[1],&ack, 1); > + assert(ret == 1); > } > } > > diff --git a/ui/spice-display.c b/ui/spice-display.c > index ec6e0cb..bdd14b8 100644 > --- a/ui/spice-display.c > +++ b/ui/spice-display.c > @@ -294,18 +294,39 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info) > info->n_surfaces = NUM_SURFACES; > } > > +/* Called from spice server thread context (via interface_get_command) */ > +int qxl_vga_mode_get_command( > + SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext) > +{ > + SimpleSpiceUpdate *update; > + unsigned char req; > + int r; > + > + update = ssd->update; > + if (update != NULL) { > + ssd->waiting_for_update = 0; > + ssd->update = NULL; > + if (update != QXL_EMPTY_UPDATE) { > + *ext = update->ext; > + return true; > + } > + } else { > + if (!ssd->waiting_for_update) { > + ssd->waiting_for_update = 1; > + req = QXL_SERVER_CREATE_UPDATE; > + r = write(ssd->pipe[1],&req , 1); > + assert(r == 1); > + } > + } > + return false; > +} > + > static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) > { > SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl); > - SimpleSpiceUpdate *update; > > dprint(3, "%s:\n", __FUNCTION__); > - update = qemu_spice_create_update(ssd); > - if (update == NULL) { > - return false; > - } > - *ext = update->ext; > - return true; > + return qxl_vga_mode_get_command(ssd, ext); > } > > static int interface_req_cmd_notification(QXLInstance *sin) > @@ -394,6 +415,45 @@ static DisplayChangeListener display_listener = { > .dpy_refresh = display_refresh, > }; > > +static void pipe_read(void *opaque) > +{ > + SimpleSpiceDisplay *ssd = opaque; > + unsigned char cmd; > + int len, create_update = 0; > + > + while (1) { > + cmd = 0; > + len = read(ssd->pipe[0],&cmd, sizeof(cmd)); > + if (len< 0) { > + if (errno == EINTR) { > + continue; > + } > + if (errno == EAGAIN) { > + break; > + } > + perror("qxl: pipe_read: read failed"); > + break; > + } > + switch (cmd) { > + case QXL_SERVER_CREATE_UPDATE: > + create_update = 1; > + break; > + default: > + fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd); > + abort(); > + } > + } > + /* no need to do this more than once */ > + if (create_update) { > + assert(ssd->update == NULL); > + ssd->update = qemu_spice_create_update(ssd); > + if (ssd->update == NULL) { > + ssd->update = QXL_EMPTY_UPDATE; > + } > + ssd->worker->wakeup(ssd->worker); > + } > +} > + > void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd, > IOHandler *pipe_read) > { > @@ -427,6 +487,7 @@ void qemu_spice_display_init(DisplayState *ds) > qemu_spice_add_interface(&sdpy.qxl.base); > assert(sdpy.worker); > > + qxl_create_server_to_iothread_pipe(&sdpy, pipe_read); > qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler,&sdpy); > qemu_spice_create_host_memslot(&sdpy); > qemu_spice_create_host_primary(&sdpy); > diff --git a/ui/spice-display.h b/ui/spice-display.h > index 3e6cf7c..2be6a34 100644 > --- a/ui/spice-display.h > +++ b/ui/spice-display.h > @@ -31,6 +31,15 @@ > > #define NUM_SURFACES 1024 > > +/* For commands/requests from server thread to iothread */ > +#define QXL_EMPTY_UPDATE ((void *)-1) > +enum { > + QXL_SERVER_SET_IRQ = 1, > + QXL_SERVER_CREATE_UPDATE, > +}; > + > +struct SimpleSpiceUpdate; > + > typedef struct SimpleSpiceDisplay { > DisplayState *ds; > void *buf; > @@ -48,6 +57,10 @@ typedef struct SimpleSpiceDisplay { > * and in native mode) and without qxl */ > pthread_t main; > int pipe[2]; /* to iothread */ > + > + /* ssd updates (one request/command at a time) */ > + struct SimpleSpiceUpdate *update; > + int waiting_for_update; > } SimpleSpiceDisplay; > > typedef struct SimpleSpiceUpdate { > @@ -71,6 +84,9 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd, > int x, int y, int w, int h); > void qemu_spice_display_resize(SimpleSpiceDisplay *ssd); > void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd); > +/* shared with qxl.c in vga mode and ui/spice-display (no qxl mode) */ > +int qxl_vga_mode_get_command( > + SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext); > /* used by both qxl and spice-display */ > void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd, > IOHandler *pipe_read);
diff --git a/hw/qxl.c b/hw/qxl.c index 201698f..64580f1 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -340,7 +340,6 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info) static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); - SimpleSpiceUpdate *update; QXLCommandRing *ring; QXLCommand *cmd; int notify; @@ -348,16 +347,25 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) switch (qxl->mode) { case QXL_MODE_VGA: dprint(qxl, 2, "%s: vga\n", __FUNCTION__); - update = qemu_spice_create_update(&qxl->ssd); - if (update == NULL) { - return false; + if (qxl_vga_mode_get_command(&qxl->ssd, ext)) { + qxl_log_command(qxl, "vga", ext); + return true; } - *ext = update->ext; - qxl_log_command(qxl, "vga", ext); - return true; + return false; case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: case QXL_MODE_UNDEFINED: + /* flush any existing updates that we didn't send to the guest. + * since update != NULL it means the server didn't get it, and + * because we changed mode to != QXL_MODE_VGA, it won't. */ + if (qxl->ssd.update != NULL) { + if (qxl->ssd.update != QXL_EMPTY_UPDATE) { + qemu_spice_destroy_update(&qxl->ssd, qxl->ssd.update); + } + qxl->ssd.update = NULL; + qxl->ssd.waiting_for_update = 0; + } + /* */ dprint(qxl, 2, "%s: %s\n", __FUNCTION__, qxl->cmdflags ? "compat" : "native"); ring = &qxl->ram->cmd_ring; @@ -1057,17 +1065,50 @@ static void qxl_map(PCIDevice *pci, int region_num, static void pipe_read(void *opaque) { - PCIQXLDevice *d = opaque; - char dummy; - int len; - - do { - len = read(d->ssd.pipe[0], &dummy, sizeof(dummy)); - } while (len == sizeof(dummy)); - qxl_set_irq(d); + SimpleSpiceDisplay *ssd = opaque; + unsigned char cmd; + int len, set_irq = 0; + int create_update = 0; + + while (1) { + cmd = 0; + len = read(ssd->pipe[0], &cmd, sizeof(cmd)); + if (len < 0) { + if (errno == EINTR) { + continue; + } + if (errno == EAGAIN) { + break; + } + perror("qxl: pipe_read: read failed"); + break; + } + switch (cmd) { + case QXL_SERVER_SET_IRQ: + set_irq = 1; + break; + case QXL_SERVER_CREATE_UPDATE: + create_update = 1; + break; + default: + fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd); + abort(); + } + } + /* no need to do either operation more than once */ + if (create_update) { + assert(ssd->update == NULL); + ssd->update = qemu_spice_create_update(ssd); + if (ssd->update == NULL) { + ssd->update = QXL_EMPTY_UPDATE; + } + ssd->worker->wakeup(ssd->worker); + } + if (set_irq) { + qxl_set_irq(container_of(ssd, PCIQXLDevice, ssd)); + } } -/* called from spice server thread context only */ static void qxl_send_events(PCIQXLDevice *d, uint32_t events) { uint32_t old_pending; @@ -1082,9 +1123,11 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events) /* running in io_thread thread */ qxl_set_irq(d); } else { - if (write(d->ssd.pipe[1], d, 1) != 1) { - dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__); - } + /* called from spice-server thread */ + int ret; + unsigned char ack = QXL_SERVER_SET_IRQ; + ret = write(d->ssd.pipe[1], &ack, 1); + assert(ret == 1); } } diff --git a/ui/spice-display.c b/ui/spice-display.c index ec6e0cb..bdd14b8 100644 --- a/ui/spice-display.c +++ b/ui/spice-display.c @@ -294,18 +294,39 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info) info->n_surfaces = NUM_SURFACES; } +/* Called from spice server thread context (via interface_get_command) */ +int qxl_vga_mode_get_command( + SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext) +{ + SimpleSpiceUpdate *update; + unsigned char req; + int r; + + update = ssd->update; + if (update != NULL) { + ssd->waiting_for_update = 0; + ssd->update = NULL; + if (update != QXL_EMPTY_UPDATE) { + *ext = update->ext; + return true; + } + } else { + if (!ssd->waiting_for_update) { + ssd->waiting_for_update = 1; + req = QXL_SERVER_CREATE_UPDATE; + r = write(ssd->pipe[1], &req , 1); + assert(r == 1); + } + } + return false; +} + static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) { SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl); - SimpleSpiceUpdate *update; dprint(3, "%s:\n", __FUNCTION__); - update = qemu_spice_create_update(ssd); - if (update == NULL) { - return false; - } - *ext = update->ext; - return true; + return qxl_vga_mode_get_command(ssd, ext); } static int interface_req_cmd_notification(QXLInstance *sin) @@ -394,6 +415,45 @@ static DisplayChangeListener display_listener = { .dpy_refresh = display_refresh, }; +static void pipe_read(void *opaque) +{ + SimpleSpiceDisplay *ssd = opaque; + unsigned char cmd; + int len, create_update = 0; + + while (1) { + cmd = 0; + len = read(ssd->pipe[0], &cmd, sizeof(cmd)); + if (len < 0) { + if (errno == EINTR) { + continue; + } + if (errno == EAGAIN) { + break; + } + perror("qxl: pipe_read: read failed"); + break; + } + switch (cmd) { + case QXL_SERVER_CREATE_UPDATE: + create_update = 1; + break; + default: + fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd); + abort(); + } + } + /* no need to do this more than once */ + if (create_update) { + assert(ssd->update == NULL); + ssd->update = qemu_spice_create_update(ssd); + if (ssd->update == NULL) { + ssd->update = QXL_EMPTY_UPDATE; + } + ssd->worker->wakeup(ssd->worker); + } +} + void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd, IOHandler *pipe_read) { @@ -427,6 +487,7 @@ void qemu_spice_display_init(DisplayState *ds) qemu_spice_add_interface(&sdpy.qxl.base); assert(sdpy.worker); + qxl_create_server_to_iothread_pipe(&sdpy, pipe_read); qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler, &sdpy); qemu_spice_create_host_memslot(&sdpy); qemu_spice_create_host_primary(&sdpy); diff --git a/ui/spice-display.h b/ui/spice-display.h index 3e6cf7c..2be6a34 100644 --- a/ui/spice-display.h +++ b/ui/spice-display.h @@ -31,6 +31,15 @@ #define NUM_SURFACES 1024 +/* For commands/requests from server thread to iothread */ +#define QXL_EMPTY_UPDATE ((void *)-1) +enum { + QXL_SERVER_SET_IRQ = 1, + QXL_SERVER_CREATE_UPDATE, +}; + +struct SimpleSpiceUpdate; + typedef struct SimpleSpiceDisplay { DisplayState *ds; void *buf; @@ -48,6 +57,10 @@ typedef struct SimpleSpiceDisplay { * and in native mode) and without qxl */ pthread_t main; int pipe[2]; /* to iothread */ + + /* ssd updates (one request/command at a time) */ + struct SimpleSpiceUpdate *update; + int waiting_for_update; } SimpleSpiceDisplay; typedef struct SimpleSpiceUpdate { @@ -71,6 +84,9 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd, int x, int y, int w, int h); void qemu_spice_display_resize(SimpleSpiceDisplay *ssd); void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd); +/* shared with qxl.c in vga mode and ui/spice-display (no qxl mode) */ +int qxl_vga_mode_get_command( + SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext); /* used by both qxl and spice-display */ void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd, IOHandler *pipe_read);
From: Uri Lublin <uril@redhat.com> This patch and the next drop the requirement to lose the global qemu mutex during dispatcher calls. This patch enables it, the next drops the unlock/lock pairs around dispatcher calls. The current solution of dropping the locks is buggy: * it allows multiple dispatcher calls from two vcpu threads, the dispatcher doesn't handle that by design (single fd, not locked, can't handle writes from two threads) * it requires us to keep track of cpu_single_env, which is magic. The solution implemented in this patch and the next (the next just drops the locks, this patch allows that to work): * the only operation that needed locking was qemu_create_simple_update, * it required locking because it was called from the spice-server thread. * do it in the iothread by reusing the existing pipe used for set_irq. The current flow implemented is now: spice-server thread: qxl.c:interface_get_command (called either by polling or from wakeup) if update!=NULL: waiting_for_update=0 update=NULL return update else: if not waiting_for_update: waiting_for_update=1 write to pipe, which is read by iothread (main thread) iothread: wakeup from select, qxl.c:pipe_read update=qemu_create_simple_update() wakeup spice-server thread by calling d.worker->wakeup(d.worker) --- hw/qxl.c | 81 +++++++++++++++++++++++++++++++++++++++------------ ui/spice-display.c | 75 +++++++++++++++++++++++++++++++++++++++++++---- ui/spice-display.h | 16 ++++++++++ 3 files changed, 146 insertions(+), 26 deletions(-)