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

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

Hi,

Looks good, except for one small issue again...

On 03/15/2011 09:17 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           |  118 +++++++++++++++++++++++++++++++++++++++++++--------
>   ui/spice-display.c |   10 +----
>   ui/spice-display.h |    8 +++-
>   3 files changed, 108 insertions(+), 28 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index d1d3c9c..2419236 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -28,6 +28,8 @@
>
>   #include "qxl.h"
>
> +#define EMPTY_UPDATE ((void *)-1)
> +
>   #undef SPICE_RING_PROD_ITEM
>   #define SPICE_RING_PROD_ITEM(r, ret) {                                  \
>           typeof(r) start = r;                                            \
> @@ -117,6 +119,15 @@ static QXLMode qxl_modes[] = {
>   #endif
>   };
>
> +
> +/*
> + * commands/requests from server thread to iothread.
> + */
> +enum {
> +    QXL_SERVER_SET_IRQ = 1,
> +    QXL_SERVER_CREATE_UPDATE,
> +};
> +
>   static PCIQXLDevice *qxl0;
>
>   static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
> @@ -336,11 +347,36 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>       info->n_surfaces = NUM_SURFACES;
>   }
>
> +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 != 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;
> +}
> +
>   /* called from spice server thread context only */
>   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 +384,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 != 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 +1102,50 @@ static void qxl_map(PCIDevice *pci, int region_num,
>
>   static void pipe_read(void *opaque)
>   {
> -    PCIQXLDevice *d = opaque;
> -    char dummy;
> -    int len;
> +    SimpleSpiceDisplay *ssd = opaque;
> +    unsigned char cmd;
> +    int len, set_irq = 0;
> +    int create_update = 0;
>
>       do {
> -        len = read(d->ssd.pipe[0],&dummy, sizeof(dummy));
> -    } while (len == sizeof(dummy));
> -    qxl_set_irq(d);
> +        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();
> +        }
> +    } while (1);
> +    /* 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 = 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,13 +1160,15 @@ 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);
>       }
>   }
>
> -void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
> +void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd)
>   {
>      if (pipe(ssd->pipe)<  0) {
>          fprintf(stderr, "%s: pipe creation failed\n", __FUNCTION__);

This function was introduced in your previous patch I agree that
qxl_create_server_to_iothread_pipe is the correct name, but lets name
it that in the first patch then, rather then introduce it under a
different name in patch1, only to rename it in patch2.

> @@ -1110,7 +1190,7 @@ void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd)
>
>   static void init_pipe_signaling(PCIQXLDevice *d)
>   {
> -   qxl_create_vcpu_to_iothread_pipe(&d->ssd);
> +   qxl_create_server_to_iothread_pipe(&d->ssd);
>   }
>
>   /* graphics console */
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index e0ac616..a9ecee0 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -297,15 +297,9 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>   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)
> @@ -406,7 +400,7 @@ void qemu_spice_display_init(DisplayState *ds)
>       qemu_spice_add_interface(&sdpy.qxl.base);
>       assert(sdpy.worker);
>
> -    qxl_create_vcpu_to_iothread_pipe(&sdpy);
> +    qxl_create_server_to_iothread_pipe(&sdpy);
>       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 a708d59..210b0b5 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -31,6 +31,8 @@
>
>   #define NUM_SURFACES 1024
>
> +struct SimpleSpiceUpdate;
> +
>   typedef struct SimpleSpiceDisplay {
>       DisplayState *ds;
>       void *buf;
> @@ -48,6 +50,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 {
> @@ -75,4 +81,4 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd);
>   int qxl_vga_mode_get_command(
>       SimpleSpiceDisplay *ssd, struct QXLCommandExt *ext);
>   /* used by both qxl and spice-display */
> -void qxl_create_vcpu_to_iothread_pipe(SimpleSpiceDisplay *ssd);
> +void qxl_create_server_to_iothread_pipe(SimpleSpiceDisplay *ssd);

Regards,

Hans