[Qemu-devel] Re: [PATCH v3 2/4] qxl: implement get_command in vga mode without locks

[Hans de Goede <hdegoede@redhat.com>]

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);