Re: [Qemu-devel] [PATCH 2/4] spice: don't create updates in spice server context.

[Alon Levy <alevy@redhat.com>]

On Fri, Apr 29, 2011 at 11:38:30AM +0200, Gerd Hoffmann wrote:
> This patch moves the creation of spice screen updates from the spice
> server context to qemu iothread context (display refresh timer to be
> exact).  This way we avoid accessing qemu internals (display surface)
> from spice thread context which in turn allows us to simplify locking.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qxl.c           |   17 +++++++++++------
>  ui/spice-display.c |   45 ++++++++++++++++++++++++++++-----------------
>  ui/spice-display.h |   21 ++++++++++++++++-----
>  3 files changed, 55 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/qxl.c b/hw/qxl.c
> index fe4212b..bd250db 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -343,18 +343,22 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
>      SimpleSpiceUpdate *update;
>      QXLCommandRing *ring;
>      QXLCommand *cmd;
> -    int notify;
> +    int notify, ret;
>  
>      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;
> +        ret = false;
> +        qemu_mutex_lock(&qxl->ssd.lock);
> +        if (qxl->ssd.update != NULL) {
> +            update = qxl->ssd.update;
> +            qxl->ssd.update = NULL;
> +            *ext = update->ext;
> +            ret = true;
>          }
> -        *ext = update->ext;
> +        qemu_mutex_unlock(&qxl->ssd.lock);
>          qxl_log_command(qxl, "vga", ext);
> -        return true;
> +        return ret;
>      case QXL_MODE_COMPAT:
>      case QXL_MODE_NATIVE:
>      case QXL_MODE_UNDEFINED:
> @@ -1304,6 +1308,7 @@ static int qxl_init_primary(PCIDevice *dev)
>      vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
>                                     qxl_hw_screen_dump, qxl_hw_text_update, qxl);
>      qxl->ssd.ds = vga->ds;
> +    qemu_mutex_init(&qxl->ssd.lock);
>      qxl->ssd.bufsize = (16 * 1024 * 1024);
>      qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
>  
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 020b423..39c0ba1 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -27,7 +27,7 @@
>  
>  #include "spice-display.h"
>  
> -static int debug = 0;
> +static int debug = 1;
Accident?

>  
>  static void GCC_FMT_ATTR(2, 3) dprint(int level, const char *fmt, ...)
>  {
> @@ -62,14 +62,7 @@ void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r)
>      dest->right = MAX(dest->right, r->right);
>  }
>  
> -/*
> - * Called from spice server thread context (via interface_get_command).
> - *
> - * We must aquire the global qemu mutex here to make sure the
> - * DisplayState (+DisplaySurface) we are accessing doesn't change
> - * underneath us.
> - */
> -SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
> +static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
>  {
>      SimpleSpiceUpdate *update;
>      QXLDrawable *drawable;
> @@ -78,9 +71,7 @@ SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
>      uint8_t *src, *dst;
>      int by, bw, bh;
>  
> -    qemu_mutex_lock_iothread();
>      if (qemu_spice_rect_is_empty(&ssd->dirty)) {
> -        qemu_mutex_unlock_iothread();
>          return NULL;
>      };
>  
> @@ -141,7 +132,6 @@ SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
>      cmd->data = (intptr_t)drawable;
>  
>      memset(&ssd->dirty, 0, sizeof(ssd->dirty));
> -    qemu_mutex_unlock_iothread();
>      return update;
>  }
>  
> @@ -241,6 +231,12 @@ void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
>      qemu_pf_conv_put(ssd->conv);
>      ssd->conv = NULL;
>  
> +    qemu_mutex_lock(&ssd->lock);
> +    if (ssd->update != NULL) {
> +        qemu_spice_destroy_update(ssd, ssd->update);
> +        ssd->update = NULL;
> +    }
> +    qemu_mutex_unlock(&ssd->lock);
>      qemu_spice_destroy_host_primary(ssd);
>      qemu_spice_create_host_primary(ssd);
>  
> @@ -252,6 +248,14 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
>  {
>      dprint(3, "%s:\n", __FUNCTION__);
>      vga_hw_update();
> +
> +    qemu_mutex_lock(&ssd->lock);
> +    if (ssd->update == NULL) {
> +        ssd->update = qemu_spice_create_update(ssd);
> +        ssd->notify++;
> +    }
> +    qemu_mutex_unlock(&ssd->lock);
> +
>      if (ssd->notify) {
>          ssd->notify = 0;
>          ssd->worker->wakeup(ssd->worker);
> @@ -298,14 +302,20 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
>  {
>      SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
>      SimpleSpiceUpdate *update;
> +    int ret = false;
>  
>      dprint(3, "%s:\n", __FUNCTION__);
> -    update = qemu_spice_create_update(ssd);
> -    if (update == NULL) {
> -        return false;
> +
> +    qemu_mutex_lock(&ssd->lock);
> +    if (ssd->update != NULL) {
> +        update = ssd->update;
> +        ssd->update = NULL;
> +        *ext = update->ext;
> +        ret = true;
>      }
> -    *ext = update->ext;
> -    return true;
> +    qemu_mutex_unlock(&ssd->lock);
> +
> +    return ret;
>  }
>  
>  static int interface_req_cmd_notification(QXLInstance *sin)
> @@ -398,6 +408,7 @@ void qemu_spice_display_init(DisplayState *ds)
>  {
>      assert(sdpy.ds == NULL);
>      sdpy.ds = ds;
> +    qemu_mutex_init(&sdpy.lock);
>      sdpy.bufsize = (16 * 1024 * 1024);
>      sdpy.buf = qemu_malloc(sdpy.bufsize);
>      register_displaychangelistener(ds, &display_listener);
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index aef0464..e0cc46e 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -19,6 +19,7 @@
>  #include <spice/enums.h>
>  #include <spice/qxl_dev.h>
>  
> +#include "qemu-thread.h"
>  #include "pflib.h"
>  
>  #define NUM_MEMSLOTS 8
> @@ -31,7 +32,10 @@
>  
>  #define NUM_SURFACES 1024
>  
> -typedef struct SimpleSpiceDisplay {
> +typedef struct SimpleSpiceDisplay SimpleSpiceDisplay;
> +typedef struct SimpleSpiceUpdate SimpleSpiceUpdate;
> +
> +struct SimpleSpiceDisplay {
>      DisplayState *ds;
>      void *buf;
>      int bufsize;
> @@ -43,19 +47,26 @@ typedef struct SimpleSpiceDisplay {
>      QXLRect dirty;
>      int notify;
>      int running;
> -} SimpleSpiceDisplay;
>  
> -typedef struct SimpleSpiceUpdate {
> +    /*
> +     * All struct members below this comment can be accessed from
> +     * both spice server and qemu (iothread) context and any access
> +     * to them must be protected by the lock.
> +     */
> +    QemuMutex lock;
> +    SimpleSpiceUpdate *update;
> +};
> +
> +struct SimpleSpiceUpdate {
>      QXLDrawable drawable;
>      QXLImage image;
>      QXLCommandExt ext;
>      uint8_t *bitmap;
> -} SimpleSpiceUpdate;
> +};
>  
>  int qemu_spice_rect_is_empty(const QXLRect* r);
>  void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
>  
> -SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *sdpy);
>  void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate *update);
>  void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd);
>  void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd);
> -- 
> 1.7.1
> 
>