[Qemu-devel] [PATCH v2 0/2] Threaded VNC server

[Qemu-devel] [PATCH v2 0/2] Threaded VNC server

[Corentin Chary]

Hi,
This series add a threaded VNC server and should be applied on top on my previous patch set (adding tight encoding).
The first patch add some functions to qemu-thread. The last oneis the threaded VNC server.

Thanks

Since v1:
* Moved locks from VncState to VncDisplay because it's only used in vnc_refresh
* Use trylock in vnc_refresh. If there is an encoding task still running, reschedule the refresh.
  This really boost performances and make the vnc server truly asynchroneous. The only blocking
  lock is the output_mutex which is only held during a simple memcpy().
* Fixed issues found by Paolo, except the exit condition, mainly because we can only have
  one queue per VncState (due to zstreams), so this is not really an issue.
* Rebased on top of jpeg and ui/ patchs

Corentin Chary (2):
  qemu-thread: add qemu_mutex/cond_destroy and qemu_mutex_exit
  vnc: threaded VNC server

 Makefile.objs      |    7 +-
 configure          |   13 ++
 qemu-thread.c      |   22 ++++
 qemu-thread.h      |    4 +
 ui/vnc-jobs-sync.c |   65 ++++++++++
 ui/vnc-jobs.c      |  351 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui/vnc.c           |  169 ++++++++++++++++++++++----
 ui/vnc.h           |   75 +++++++++++
 8 files changed, 683 insertions(+), 23 deletions(-)
 create mode 100644 ui/vnc-jobs-sync.c
 create mode 100644 ui/vnc-jobs.c

[Qemu-devel] [PATCH v2 1/2] qemu-thread: add qemu_mutex/cond_destroy and qemu_mutex_exit

[Corentin Chary]

Add some missing functions in qemu-thread. Currently qemu-thread
is only used for io-thread but it will used by the vnc server soon
and we need those functions instead of calling pthread directly.

Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 qemu-thread.c |   22 ++++++++++++++++++++++
 qemu-thread.h |    4 ++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/qemu-thread.c b/qemu-thread.c
index 3923db7..afc9933 100644
--- a/qemu-thread.c
+++ b/qemu-thread.c
@@ -34,6 +34,15 @@ void qemu_mutex_init(QemuMutex *mutex)
         error_exit(err, __func__);
 }
 
+void qemu_mutex_destroy(QemuMutex *mutex)
+{
+    int err;
+
+    err = pthread_mutex_destroy(&mutex->lock);
+    if (err)
+        error_exit(err, __func__);
+}
+
 void qemu_mutex_lock(QemuMutex *mutex)
 {
     int err;
@@ -90,6 +99,15 @@ void qemu_cond_init(QemuCond *cond)
         error_exit(err, __func__);
 }
 
+void qemu_cond_destroy(QemuCond *cond)
+{
+    int err;
+
+    err = pthread_cond_destroy(&cond->cond);
+    if (err)
+        error_exit(err, __func__);
+}
+
 void qemu_cond_signal(QemuCond *cond)
 {
     int err;
@@ -161,3 +179,7 @@ int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2)
    return pthread_equal(thread1->thread, thread2->thread);
 }
 
+void qemu_thread_exit(void *retval)
+{
+    pthread_exit(retval);
+}
diff --git a/qemu-thread.h b/qemu-thread.h
index 5ef4a3a..19bb30c 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -20,12 +20,14 @@ typedef struct QemuCond QemuCond;
 typedef struct QemuThread QemuThread;
 
 void qemu_mutex_init(QemuMutex *mutex);
+void qemu_mutex_destroy(QemuMutex *mutex);
 void qemu_mutex_lock(QemuMutex *mutex);
 int qemu_mutex_trylock(QemuMutex *mutex);
 int qemu_mutex_timedlock(QemuMutex *mutex, uint64_t msecs);
 void qemu_mutex_unlock(QemuMutex *mutex);
 
 void qemu_cond_init(QemuCond *cond);
+void qemu_cond_destroy(QemuCond *cond);
 void qemu_cond_signal(QemuCond *cond);
 void qemu_cond_broadcast(QemuCond *cond);
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex);
@@ -37,4 +39,6 @@ void qemu_thread_create(QemuThread *thread,
 void qemu_thread_signal(QemuThread *thread, int sig);
 void qemu_thread_self(QemuThread *thread);
 int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2);
+void qemu_thread_exit(void *retval);
+
 #endif
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/2] vnc: threaded VNC server

[Corentin Chary]

Implement a threaded VNC server using the producer-consumer model.
The main thread will push encoding jobs (a list a rectangles to update)
in a queue, and the VNC worker thread will consume that queue and send
framebuffer updates to the output buffer.

The threaded VNC server can be enabled with ./configure --enable-vnc-thread.

If you don't want it, just use ./configure --disable-vnc-thread and a syncrhonous
queue of job will be used (which as exactly the same behavior as the old queue).
If you disable the VNC thread, all thread related code will not be built and there will
be no overhead.

Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 Makefile.objs      |    7 +-
 configure          |   13 ++
 ui/vnc-jobs-sync.c |   65 ++++++++++
 ui/vnc-jobs.c      |  351 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui/vnc.c           |  169 ++++++++++++++++++++++----
 ui/vnc.h           |   75 +++++++++++
 6 files changed, 657 insertions(+), 23 deletions(-)
 create mode 100644 ui/vnc-jobs-sync.c
 create mode 100644 ui/vnc-jobs.c

diff --git a/Makefile.objs b/Makefile.objs
index 22622a9..0c6334b 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -109,10 +109,15 @@ ui-obj-y += vnc-enc-tight.o
 ui-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
 ui-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
 ui-obj-$(CONFIG_COCOA) += cocoa.o
+ifdef CONFIG_VNC_THREAD
+ui-obj-y += vnc-jobs.o
+else
+ui-obj-y += vnc-jobs-sync.o
+endif
 common-obj-y += $(addprefix ui/, $(ui-obj-y))
 
 common-obj-y += iov.o acl.o
-common-obj-$(CONFIG_IOTHREAD) += qemu-thread.o
+common-obj-$(CONFIG_THREAD) += qemu-thread.o
 common-obj-y += notify.o event_notifier.o
 common-obj-y += qemu-timer.o
 
diff --git a/configure b/configure
index 679f2fc..6f2e3a7 100755
--- a/configure
+++ b/configure
@@ -264,6 +264,7 @@ vde=""
 vnc_tls=""
 vnc_sasl=""
 vnc_jpeg=""
+vnc_thread=""
 xen=""
 linux_aio=""
 vhost_net=""
@@ -552,6 +553,10 @@ for opt do
   ;;
   --enable-vnc-jpeg) vnc_jpeg="yes"
   ;;
+  --disable-vnc-thread) vnc_thread="no"
+  ;;
+  --enable-vnc-thread) vnc_thread="yes"
+  ;;
   --disable-slirp) slirp="no"
   ;;
   --disable-uuid) uuid="no"
@@ -786,6 +791,8 @@ echo "  --disable-vnc-sasl       disable SASL encryption for VNC server"
 echo "  --enable-vnc-sasl        enable SASL encryption for VNC server"
 echo "  --disable-vnc-jpeg       disable JPEG lossy compression for VNC server"
 echo "  --enable-vnc-jpeg        enable JPEG lossy compression for VNC server"
+echo "  --disable-vnc-thread     disable threaded VNC server"
+echo "  --enable-vnc-thread      enable threaded VNC server"
 echo "  --disable-curses         disable curses output"
 echo "  --enable-curses          enable curses output"
 echo "  --disable-curl           disable curl connectivity"
@@ -2048,6 +2055,7 @@ echo "Mixer emulation   $mixemu"
 echo "VNC TLS support   $vnc_tls"
 echo "VNC SASL support  $vnc_sasl"
 echo "VNC JPEG support  $vnc_jpeg"
+echo "VNC thread        $vnc_thread"
 if test -n "$sparc_cpu"; then
     echo "Target Sparc Arch $sparc_cpu"
 fi
@@ -2191,6 +2199,10 @@ if test "$vnc_jpeg" = "yes" ; then
   echo "CONFIG_VNC_JPEG=y" >> $config_host_mak
   echo "VNC_JPEG_CFLAGS=$vnc_jpeg_cflags" >> $config_host_mak
 fi
+if test "$vnc_thread" = "yes" ; then
+  echo "CONFIG_VNC_THREAD=y" >> $config_host_mak
+  echo "CONFIG_THREAD=y" >> $config_host_mak
+fi
 if test "$fnmatch" = "yes" ; then
   echo "CONFIG_FNMATCH=y" >> $config_host_mak
 fi
@@ -2267,6 +2279,7 @@ if test "$xen" = "yes" ; then
 fi
 if test "$io_thread" = "yes" ; then
   echo "CONFIG_IOTHREAD=y" >> $config_host_mak
+  echo "CONFIG_THREAD=y" >> $config_host_mak
 fi
 if test "$linux_aio" = "yes" ; then
   echo "CONFIG_LINUX_AIO=y" >> $config_host_mak
diff --git a/ui/vnc-jobs-sync.c b/ui/vnc-jobs-sync.c
new file mode 100644
index 0000000..9f138f5
--- /dev/null
+++ b/ui/vnc-jobs-sync.c
@@ -0,0 +1,65 @@
+/*
+ * QEMU VNC display driver
+ *
+ * Copyright (C) 2006 Anthony Liguori <anthony@codemonkey.ws>
+ * Copyright (C) 2006 Fabrice Bellard
+ * Copyright (C) 2009 Red Hat, Inc
+ * Copyright (C) 2010 Corentin Chary <corentin.chary@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+
+#include "vnc.h"
+
+VncJob *vnc_job_new(VncState *vs)
+{
+    vs->job.vs = vs;
+    vs->job.rectangles = 0;
+
+    vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
+    vnc_write_u8(vs, 0);
+    vs->job.saved_offset = vs->output.offset;
+    vnc_write_u16(vs, 0);
+    return &vs->job;
+}
+
+void vnc_job_push(VncJob *job)
+{
+    VncState *vs = job->vs;
+
+    vs->output.buffer[job->saved_offset] = (job->rectangles >> 8) & 0xFF;
+    vs->output.buffer[job->saved_offset + 1] = job->rectangles & 0xFF;
+    vnc_flush(job->vs);
+}
+
+int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h)
+{
+    int n;
+
+    n = vnc_send_framebuffer_update(job->vs, x, y, w, h);
+    if (n >= 0)
+        job->rectangles += n;
+    return n;
+}
+
+bool vnc_has_job(VncState *vs)
+{
+    return false;
+}
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
new file mode 100644
index 0000000..65ce5f8
--- /dev/null
+++ b/ui/vnc-jobs.c
@@ -0,0 +1,351 @@
+/*
+ * QEMU VNC display driver
+ *
+ * Copyright (C) 2006 Anthony Liguori <anthony@codemonkey.ws>
+ * Copyright (C) 2006 Fabrice Bellard
+ * Copyright (C) 2009 Red Hat, Inc
+ * Copyright (C) 2010 Corentin Chary <corentin.chary@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+
+#include "vnc.h"
+
+/*
+ * Locking:
+ *
+ * There is three levels of locking:
+ * - jobs queue lock: for each operation on the queue (push, pop, isEmpty?)
+ * - VncDisplay global lock: mainly used for framebuffer updates to avoid
+ *                      screen corruption if the framebuffer is updated
+ *			while the worker is doing something.
+ * - VncState::output lock: used to make sure the output buffer is not corrupted
+ * 		   	 if two threads try to write on it at the same time
+ *
+ * While the VNC worker thread is working, the VncDisplay global lock is hold
+ * to avoid screen corruptions (this does not block vnc_refresh() because it
+ * uses trylock()) but the output lock is not hold because the thread work on
+ * its own output buffer.
+ * When the encoding job is done, the worker thread will hold the output lock
+ * and copy its output buffer in vs->output.
+*/
+
+struct VncJobQueue {
+    QemuCond cond;
+    QemuMutex mutex;
+    QemuThread thread;
+    Buffer buffer;
+    bool exit;
+    QTAILQ_HEAD(, VncJob) jobs;
+};
+
+typedef struct VncJobQueue VncJobQueue;
+
+/*
+ * We use a single global queue, but most of the functions are
+ * already reetrant, so we can easilly add more than one encoding thread
+ */
+static VncJobQueue *queue;
+
+static void vnc_lock_queue(VncJobQueue *queue)
+{
+    qemu_mutex_lock(&queue->mutex);
+}
+
+static void vnc_unlock_queue(VncJobQueue *queue)
+{
+    qemu_mutex_unlock(&queue->mutex);
+}
+
+VncJob *vnc_job_new(VncState *vs)
+{
+    VncJob *job = qemu_mallocz(sizeof(VncJob));
+
+    job->vs = vs;
+    vnc_lock_queue(queue);
+    QLIST_INIT(&job->rectangles);
+    vnc_unlock_queue(queue);
+    return job;
+}
+
+int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h)
+{
+    VncRectEntry *entry = qemu_mallocz(sizeof(VncRectEntry));
+
+    entry->rect.x = x;
+    entry->rect.y = y;
+    entry->rect.w = w;
+    entry->rect.h = h;
+
+    vnc_lock_queue(queue);
+    QLIST_INSERT_HEAD(&job->rectangles, entry, next);
+    vnc_unlock_queue(queue);
+    return 1;
+}
+
+void vnc_job_push(VncJob *job)
+{
+    vnc_lock_queue(queue);
+    if (QLIST_EMPTY(&job->rectangles)) {
+        qemu_free(job);
+    } else {
+        QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
+        qemu_cond_broadcast(&queue->cond);
+    }
+    vnc_unlock_queue(queue);
+}
+
+static bool vnc_has_job_locked(VncState *vs)
+{
+    VncJob *job;
+
+    QTAILQ_FOREACH(job, &queue->jobs, next) {
+        if (job->vs == vs || !vs) {
+            return true;
+        }
+    }
+    return false;
+}
+
+bool vnc_has_job(VncState *vs)
+{
+    bool ret;
+
+    vnc_lock_queue(queue);
+    ret = vnc_has_job_locked(vs);
+    vnc_unlock_queue(queue);
+    return ret;
+}
+
+void vnc_jobs_clear(VncState *vs)
+{
+    VncJob *job, *tmp;
+
+    vnc_lock_queue(queue);
+    QTAILQ_FOREACH_SAFE(job, &queue->jobs, next, tmp) {
+        if (job->vs == vs || !vs)
+            QTAILQ_REMOVE(&queue->jobs, job, next);
+    }
+    vnc_unlock_queue(queue);
+}
+
+void vnc_jobs_join(VncState *vs)
+{
+    vnc_lock_queue(queue);
+    while (vnc_has_job_locked(vs)) {
+        qemu_cond_wait(&queue->cond, &queue->mutex);
+    }
+    vnc_unlock_queue(queue);
+}
+
+/*
+ * Copy data for local use
+ * FIXME: isolate what we use in a specific structure
+ * to avoid invalid usage in vnc-encoding-*.c and avoid copying
+ * because what we want is only want is only swapping VncState::output
+ * with the queue buffer
+ */
+static void vnc_async_encoding_start(VncState *orig, VncState *local)
+{
+    local->vnc_encoding = orig->vnc_encoding;
+    local->features = orig->features;
+    local->ds = orig->ds;
+    local->vd = orig->vd;
+    local->write_pixels = orig->write_pixels;
+    local->clientds = orig->clientds;
+    local->tight_quality = orig->tight_quality;
+    local->tight_compression = orig->tight_compression;
+    local->tight_pixel24 = 0;
+    local->tight = orig->tight;
+    local->tight_tmp = orig->tight_tmp;
+    local->tight_zlib = orig->tight_zlib;
+    local->tight_gradient = orig->tight_gradient;
+    local->tight_jpeg = orig->tight_jpeg;
+    memcpy(local->tight_levels, orig->tight_levels, sizeof(orig->tight_levels));
+    memcpy(local->tight_stream, orig->tight_stream, sizeof(orig->tight_stream));
+    local->send_hextile_tile = orig->send_hextile_tile;
+    local->zlib = orig->zlib;
+    local->zlib_tmp = orig->zlib_tmp;
+    local->zlib_stream = orig->zlib_stream;
+    local->zlib_level = orig->zlib_level;
+    local->output =  queue->buffer;
+    local->csock = -1; /* Don't do any network work on this thread */
+
+    buffer_reset(&local->output);
+}
+
+static void vnc_async_encoding_end(VncState *orig, VncState *local)
+{
+    orig->tight_quality = local->tight_quality;
+    orig->tight_compression = local->tight_compression;
+    orig->tight = local->tight;
+    orig->tight_tmp = local->tight_tmp;
+    orig->tight_zlib = local->tight_zlib;
+    orig->tight_gradient = local->tight_gradient;
+    orig->tight_jpeg = local->tight_jpeg;
+    memcpy(orig->tight_levels, local->tight_levels, sizeof(local->tight_levels));
+    memcpy(orig->tight_stream, local->tight_stream, sizeof(local->tight_stream));
+    orig->zlib = local->zlib;
+    orig->zlib_tmp = local->zlib_tmp;
+    orig->zlib_stream = local->zlib_stream;
+    orig->zlib_level = local->zlib_level;
+}
+
+static int vnc_worker_thread_loop(VncJobQueue *queue)
+{
+    VncJob *job;
+    VncRectEntry *entry, *tmp;
+    VncState vs;
+    int n_rectangles;
+    int saved_offset;
+    bool flush;
+
+    vnc_lock_queue(queue);
+    if (QTAILQ_EMPTY(&queue->jobs)) {
+        qemu_cond_wait(&queue->cond, &queue->mutex);
+    }
+
+    /* If the queue is empty, it's an exit order */
+    if (QTAILQ_EMPTY(&queue->jobs)) {
+        vnc_unlock_queue(queue);
+        return -1;
+    }
+
+    job = QTAILQ_FIRST(&queue->jobs);
+    vnc_unlock_queue(queue);
+
+    qemu_mutex_lock(&job->vs->output_mutex);
+    if (job->vs->csock == -1 || job->vs->abording == true) {
+        goto disconnected;
+    }
+    qemu_mutex_unlock(&job->vs->output_mutex);
+
+    /* Make a local copy of vs and switch output buffers */
+    vnc_async_encoding_start(job->vs, &vs);
+
+    /* Start sending rectangles */
+    n_rectangles = 0;
+    vnc_write_u8(&vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
+    vnc_write_u8(&vs, 0);
+    saved_offset = vs.output.offset;
+    vnc_write_u16(&vs, 0);
+
+    qemu_mutex_lock(&job->vs->vd->mutex);
+    QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
+        int n;
+
+        if (job->vs->csock == -1) {
+            qemu_mutex_unlock(&job->vs->vd->mutex);
+            goto disconnected;
+        }
+
+        n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
+                                        entry->rect.w, entry->rect.h);
+
+        if (n >= 0)
+            n_rectangles += n;
+        qemu_free(entry);
+    }
+    qemu_mutex_unlock(&job->vs->vd->mutex);
+
+    /* Put n_rectangles at the beginning of the message */
+    vs.output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
+    vs.output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
+
+    /* Switch back buffers */
+    qemu_mutex_lock(&job->vs->output_mutex);
+    if (job->vs->csock == -1) {
+        goto disconnected;
+    }
+
+    vnc_write(job->vs, vs.output.buffer, vs.output.offset);
+
+disconnected:
+    /* Copy persistent encoding data */
+    vnc_async_encoding_end(job->vs, &vs);
+    flush = (job->vs->csock != -1 && job->vs->abording != true);
+    qemu_mutex_unlock(&job->vs->output_mutex);
+
+    if (flush) {
+        vnc_flush(job->vs);
+    }
+
+    qemu_mutex_lock(&queue->mutex);
+    QTAILQ_REMOVE(&queue->jobs, job, next);
+    qemu_mutex_unlock(&queue->mutex);
+    qemu_cond_broadcast(&queue->cond);
+    qemu_free(job);
+    return 0;
+}
+
+static VncJobQueue *vnc_queue_init(void)
+{
+    VncJobQueue *queue = qemu_mallocz(sizeof(VncJobQueue));
+
+    qemu_cond_init(&queue->cond);
+    qemu_mutex_init(&queue->mutex);
+    QTAILQ_INIT(&queue->jobs);
+    return queue;
+}
+
+static void vnc_queue_clear(VncJobQueue *q)
+{
+    qemu_cond_destroy(&queue->cond);
+    qemu_mutex_destroy(&queue->mutex);
+    buffer_free(&queue->buffer);
+    qemu_free(q);
+    queue = NULL; /* Unset global queue */
+}
+
+static void *vnc_worker_thread(void *arg)
+{
+    VncJobQueue *queue = arg;
+
+    while (!vnc_worker_thread_loop(queue)) ;
+    vnc_queue_clear(queue);
+    return NULL;
+}
+
+void vnc_start_worker_thread(void)
+{
+    VncJobQueue *q;
+
+    if (vnc_worker_thread_running())
+        return ;
+
+    q = vnc_queue_init();
+    qemu_thread_create(&q->thread, vnc_worker_thread, q);
+    queue = q; /* Set global queue */
+}
+
+bool vnc_worker_thread_running(void)
+{
+    return queue; /* Check global queue */
+}
+
+void vnc_stop_worker_thread(void)
+{
+    if (!vnc_worker_thread_running())
+        return ;
+
+    /* Remove all jobs and wake up the thread */
+    vnc_jobs_clear(NULL);
+    qemu_cond_broadcast(&queue->cond);
+}
diff --git a/ui/vnc.c b/ui/vnc.c
index e3ef315..f6475b3 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -45,6 +45,36 @@
     } \
 }
 
+#ifdef CONFIG_VNC_THREAD
+static int vnc_trylock_display(VncDisplay *vd)
+{
+    return qemu_mutex_trylock(&vd->mutex);
+}
+
+static void vnc_unlock_display(VncDisplay *vd)
+{
+    qemu_mutex_unlock(&vd->mutex);
+}
+
+static void vnc_lock_output(VncState *vs)
+{
+    qemu_mutex_lock(&vs->output_mutex);
+}
+
+static void vnc_unlock_output(VncState *vs)
+{
+    qemu_mutex_unlock(&vs->output_mutex);
+}
+#else
+static int vnc_trylock_display(VncDisplay *vd)
+{
+    return 0;
+}
+
+#define vnc_unlock_display(vs) (void) (vs);
+#define vnc_lock_output(vs) (void) (vs);
+#define vnc_unlock_output(vs) (void) (vs);
+#endif
 
 static VncDisplay *vnc_display; /* needed for info vnc */
 static DisplayChangeListener *dcl;
@@ -363,6 +393,7 @@ static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
 */
 
 static int vnc_update_client(VncState *vs, int has_dirty);
+static int vnc_update_client_sync(VncState *vs, int has_dirty);
 static void vnc_disconnect_start(VncState *vs);
 static void vnc_disconnect_finish(VncState *vs);
 static void vnc_init_timer(VncDisplay *vd);
@@ -493,6 +524,7 @@ void buffer_append(Buffer *buffer, const void *data, size_t len)
     buffer->offset += len;
 }
 
+
 static void vnc_desktop_resize(VncState *vs)
 {
     DisplayState *ds = vs->ds;
@@ -506,19 +538,46 @@ static void vnc_desktop_resize(VncState *vs)
     }
     vs->client_width = ds_get_width(ds);
     vs->client_height = ds_get_height(ds);
+    vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
     vnc_write_u16(vs, 1); /* number of rects */
     vnc_framebuffer_update(vs, 0, 0, vs->client_width, vs->client_height,
                            VNC_ENCODING_DESKTOPRESIZE);
+    vnc_unlock_output(vs);
     vnc_flush(vs);
 }
 
+#ifdef CONFIG_VNC_THREAD
+static void vnc_abort_display_jobs(VncDisplay *vd)
+{
+    VncState *vs;
+
+    QTAILQ_FOREACH(vs, &vd->clients, next) {
+        vnc_lock_output(vs);
+        vs->abording = true;
+        vnc_unlock_output(vs);
+    }
+    QTAILQ_FOREACH(vs, &vd->clients, next) {
+        vnc_jobs_join(vs);
+    }
+    QTAILQ_FOREACH(vs, &vd->clients, next) {
+        vnc_lock_output(vs);
+        vs->abording = false;
+        vnc_unlock_output(vs);
+    }
+}
+#else
+#define vnc_abort_display_jobs(vd)
+#endif
+
 static void vnc_dpy_resize(DisplayState *ds)
 {
     VncDisplay *vd = ds->opaque;
     VncState *vs;
 
+    vnc_abort_display_jobs(vd);
+
     /* server surface */
     if (!vd->server)
         vd->server = qemu_mallocz(sizeof(*vd->server));
@@ -646,7 +705,7 @@ int vnc_raw_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
     return 1;
 }
 
-static int send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
+int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
 {
     int n = 0;
 
@@ -672,12 +731,14 @@ static int send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
 static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int dst_y, int w, int h)
 {
     /* send bitblit op to the vnc client */
+    vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
     vnc_write_u16(vs, 1); /* number of rects */
     vnc_framebuffer_update(vs, dst_x, dst_y, w, h, VNC_ENCODING_COPYRECT);
     vnc_write_u16(vs, src_x);
     vnc_write_u16(vs, src_y);
+    vnc_unlock_output(vs);
     vnc_flush(vs);
 }
 
@@ -694,7 +755,7 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int
     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
         if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
             vs->force_update = 1;
-            vnc_update_client(vs, 1);
+            vnc_update_client_sync(vs, 1);
             /* vs might be free()ed here */
         }
     }
@@ -814,15 +875,29 @@ static int find_and_clear_dirty_height(struct VncState *vs,
     return h;
 }
 
+#ifdef CONFIG_VNC_THREAD
+static int vnc_update_client_sync(VncState *vs, int has_dirty)
+{
+    int ret = vnc_update_client(vs, has_dirty);
+    vnc_jobs_join(vs);
+    return ret;
+}
+#else
+static int vnc_update_client_sync(VncState *vs, int has_dirty)
+{
+    return vnc_update_client(vs, has_dirty);
+}
+#endif
+
 static int vnc_update_client(VncState *vs, int has_dirty)
 {
     if (vs->need_update && vs->csock != -1) {
         VncDisplay *vd = vs->vd;
+        VncJob *job;
         int y;
-        int n_rectangles;
-        int saved_offset;
         int width, height;
-        int n;
+        int n = 0;
+
 
         if (vs->output.offset && !vs->audio_cap && !vs->force_update)
             /* kernel send buffers are full -> drop frames to throttle */
@@ -837,11 +912,7 @@ static int vnc_update_client(VncState *vs, int has_dirty)
          * happening in parallel don't disturb us, the next pass will
          * send them to the client.
          */
-        n_rectangles = 0;
-        vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
-        vnc_write_u8(vs, 0);
-        saved_offset = vs->output.offset;
-        vnc_write_u16(vs, 0);
+        job = vnc_job_new(vs);
 
         width = MIN(vd->server->width, vs->client_width);
         height = MIN(vd->server->height, vs->client_height);
@@ -858,25 +929,23 @@ static int vnc_update_client(VncState *vs, int has_dirty)
                 } else {
                     if (last_x != -1) {
                         int h = find_and_clear_dirty_height(vs, y, last_x, x);
-                        n = send_framebuffer_update(vs, last_x * 16, y,
-                                                    (x - last_x) * 16, h);
-                        n_rectangles += n;
+
+                        n += vnc_job_add_rect(job, last_x * 16, y,
+                                              (x - last_x) * 16, h);
                     }
                     last_x = -1;
                 }
             }
             if (last_x != -1) {
                 int h = find_and_clear_dirty_height(vs, y, last_x, x);
-                n = send_framebuffer_update(vs, last_x * 16, y,
-                                            (x - last_x) * 16, h);
-                n_rectangles += n;
+                n += vnc_job_add_rect(job, last_x * 16, y,
+                                      (x - last_x) * 16, h);
             }
         }
-        vs->output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
-        vs->output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
-        vnc_flush(vs);
+
+        vnc_job_push(job);
         vs->force_update = 0;
-        return n_rectangles;
+        return n;
     }
 
     if (vs->csock == -1)
@@ -892,16 +961,20 @@ static void audio_capture_notify(void *opaque, audcnotification_e cmd)
 
     switch (cmd) {
     case AUD_CNOTIFY_DISABLE:
+        vnc_lock_output(vs);
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
         vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_END);
+        vnc_unlock_output(vs);
         vnc_flush(vs);
         break;
 
     case AUD_CNOTIFY_ENABLE:
+        vnc_lock_output(vs);
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
         vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_BEGIN);
+        vnc_unlock_output(vs);
         vnc_flush(vs);
         break;
     }
@@ -915,11 +988,13 @@ static void audio_capture(void *opaque, void *buf, int size)
 {
     VncState *vs = opaque;
 
+    vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
     vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
     vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_DATA);
     vnc_write_u32(vs, size);
     vnc_write(vs, buf, size);
+    vnc_unlock_output(vs);
     vnc_flush(vs);
 }
 
@@ -961,6 +1036,9 @@ static void vnc_disconnect_start(VncState *vs)
 
 static void vnc_disconnect_finish(VncState *vs)
 {
+    vnc_jobs_join(vs); /* Wait encoding jobs */
+
+    vnc_lock_output(vs);
     vnc_qmp_event(vs, QEVENT_VNC_DISCONNECTED);
 
     buffer_free(&vs->input);
@@ -989,6 +1067,11 @@ static void vnc_disconnect_finish(VncState *vs)
     vnc_remove_timer(vs->vd);
     if (vs->vd->lock_key_sync)
         qemu_remove_led_event_handler(vs->led);
+    vnc_unlock_output(vs);
+
+#ifdef CONFIG_VNC_THREAD
+    qemu_mutex_destroy(&vs->output_mutex);
+#endif
     qemu_free(vs);
 }
 
@@ -1108,7 +1191,7 @@ static long vnc_client_write_plain(VncState *vs)
  * the client socket. Will delegate actual work according to whether
  * SASL SSF layers are enabled (thus requiring encryption calls)
  */
-void vnc_client_write(void *opaque)
+static void vnc_client_write_locked(void *opaque)
 {
     VncState *vs = opaque;
 
@@ -1122,6 +1205,19 @@ void vnc_client_write(void *opaque)
         vnc_client_write_plain(vs);
 }
 
+void vnc_client_write(void *opaque)
+{
+    VncState *vs = opaque;
+
+    vnc_lock_output(vs);
+    if (vs->output.offset) {
+        vnc_client_write_locked(opaque);
+    } else {
+        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+    }
+    vnc_unlock_output(vs);
+}
+
 void vnc_read_when(VncState *vs, VncReadEvent *func, size_t expecting)
 {
     vs->read_handler = func;
@@ -1232,6 +1328,7 @@ void vnc_write(VncState *vs, const void *data, size_t len)
 {
     buffer_reserve(&vs->output, len);
 
+
     if (vs->csock != -1 && buffer_empty(&vs->output)) {
         qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, vnc_client_write, vs);
     }
@@ -1273,8 +1370,10 @@ void vnc_write_u8(VncState *vs, uint8_t value)
 
 void vnc_flush(VncState *vs)
 {
+    vnc_lock_output(vs);
     if (vs->csock != -1 && vs->output.offset)
-        vnc_client_write(vs);
+        vnc_client_write_locked(vs);
+    vnc_unlock_output(vs);
 }
 
 uint8_t read_u8(uint8_t *data, size_t offset)
@@ -1309,12 +1408,14 @@ static void check_pointer_type_change(Notifier *notifier)
     int absolute = kbd_mouse_is_absolute();
 
     if (vnc_has_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE) && vs->absolute != absolute) {
+        vnc_lock_output(vs);
         vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
         vnc_write_u8(vs, 0);
         vnc_write_u16(vs, 1);
         vnc_framebuffer_update(vs, absolute, 0,
                                ds_get_width(vs->ds), ds_get_height(vs->ds),
                                VNC_ENCODING_POINTER_TYPE_CHANGE);
+        vnc_unlock_output(vs);
         vnc_flush(vs);
     }
     vs->absolute = absolute;
@@ -1618,21 +1719,25 @@ static void framebuffer_update_request(VncState *vs, int incremental,
 
 static void send_ext_key_event_ack(VncState *vs)
 {
+    vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
     vnc_write_u16(vs, 1);
     vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), ds_get_height(vs->ds),
                            VNC_ENCODING_EXT_KEY_EVENT);
+    vnc_unlock_output(vs);
     vnc_flush(vs);
 }
 
 static void send_ext_audio_ack(VncState *vs)
 {
+    vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
     vnc_write_u16(vs, 1);
     vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), ds_get_height(vs->ds),
                            VNC_ENCODING_AUDIO);
+    vnc_unlock_output(vs);
     vnc_flush(vs);
 }
 
@@ -1791,12 +1896,14 @@ static void vnc_colordepth(VncState *vs)
 {
     if (vnc_has_feature(vs, VNC_FEATURE_WMVI)) {
         /* Sending a WMVi message to notify the client*/
+        vnc_lock_output(vs);
         vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
         vnc_write_u8(vs, 0);
         vnc_write_u16(vs, 1); /* number of rects */
         vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), 
                                ds_get_height(vs->ds), VNC_ENCODING_WMVi);
         pixel_format_message(vs);
+        vnc_unlock_output(vs);
         vnc_flush(vs);
     } else {
         set_pixel_conversion(vs);
@@ -2224,12 +2331,21 @@ static void vnc_refresh(void *opaque)
 
     vga_hw_update();
 
+    if (vnc_trylock_display(vd)) {
+        vd->timer_interval = VNC_REFRESH_INTERVAL_BASE;
+        qemu_mod_timer(vd->timer, qemu_get_clock(rt_clock) +
+                       vd->timer_interval);
+        return;
+    }
+
     has_dirty = vnc_refresh_server_surface(vd);
+    vnc_unlock_display(vd);
 
     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
         rects += vnc_update_client(vs, has_dirty);
         /* vs might be free()ed here */
     }
+
     /* vd->timer could be NULL now if the last client disconnected,
      * in this case don't update the timer */
     if (vd->timer == NULL)
@@ -2288,6 +2404,10 @@ static void vnc_connect(VncDisplay *vd, int csock)
     vs->as.fmt = AUD_FMT_S16;
     vs->as.endianness = 0;
 
+#ifdef CONFIG_VNC_THREAD
+    qemu_mutex_init(&vs->output_mutex);
+#endif
+
     QTAILQ_INSERT_HEAD(&vd->clients, vs, next);
 
     vga_hw_update();
@@ -2345,6 +2465,11 @@ void vnc_display_init(DisplayState *ds)
     if (!vs->kbd_layout)
         exit(1);
 
+#ifdef CONFIG_VNC_THREAD
+    qemu_mutex_init(&vs->mutex);
+    vnc_start_worker_thread();
+#endif
+
     dcl->dpy_copy = vnc_dpy_copy;
     dcl->dpy_update = vnc_dpy_update;
     dcl->dpy_resize = vnc_dpy_resize;
diff --git a/ui/vnc.h b/ui/vnc.h
index cca1946..9405e61 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -29,6 +29,9 @@
 
 #include "qemu-common.h"
 #include "qemu-queue.h"
+#ifdef CONFIG_VNC_THREAD
+#include "qemu-thread.h"
+#endif
 #include "console.h"
 #include "monitor.h"
 #include "audio/audio.h"
@@ -59,6 +62,9 @@ typedef struct Buffer
 } Buffer;
 
 typedef struct VncState VncState;
+typedef struct VncJob VncJob;
+typedef struct VncRect VncRect;
+typedef struct VncRectEntry VncRectEntry;
 
 typedef int VncReadEvent(VncState *vs, uint8_t *data, size_t len);
 
@@ -101,6 +107,9 @@ struct VncDisplay
     DisplayState *ds;
     kbd_layout_t *kbd_layout;
     int lock_key_sync;
+#ifdef CONFIG_VNC_THREAD
+    QemuMutex mutex;
+#endif
 
     QEMUCursor *cursor;
     int cursor_msize;
@@ -122,6 +131,38 @@ struct VncDisplay
 #endif
 };
 
+
+#ifdef CONFIG_VNC_THREAD
+struct VncRect
+{
+    int x;
+    int y;
+    int w;
+    int h;
+};
+
+struct VncRectEntry
+{
+    struct VncRect rect;
+    QLIST_ENTRY(VncRectEntry) next;
+};
+
+struct VncJob
+{
+    VncState *vs;
+
+    QLIST_HEAD(, VncRectEntry) rectangles;
+    QTAILQ_ENTRY(VncJob) next;
+};
+#else
+struct VncJob
+{
+    VncState *vs;
+    int rectangles;
+    size_t saved_offset;
+};
+#endif
+
 struct VncState
 {
     int csock;
@@ -170,6 +211,12 @@ struct VncState
     QEMUPutLEDEntry *led;
 
     /* Encoding specific */
+    bool abording;
+#ifndef CONFIG_VNC_THREAD
+    VncJob job;
+#else
+    QemuMutex output_mutex;
+#endif
 
     /* Tight */
     uint8_t tight_quality;
@@ -412,6 +459,8 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
 void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v);
 
 /* Encodings */
+int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
+
 int vnc_raw_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
 
 int vnc_hextile_send_framebuffer_update(VncState *vs, int x,
@@ -427,4 +476,30 @@ void vnc_zlib_clear(VncState *vs);
 int vnc_tight_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
 void vnc_tight_clear(VncState *vs);
 
+/* Jobs */
+#ifdef CONFIG_VNC_THREAD
+
+VncJob *vnc_job_new(VncState *vs);
+int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h);
+void vnc_job_push(VncJob *job);
+bool vnc_has_job(VncState *vs);
+void vnc_jobs_clear(VncState *vs);
+void vnc_jobs_join(VncState *vs);
+void vnc_start_worker_thread(void);
+bool vnc_worker_thread_running(void);
+void vnc_stop_worker_thread(void);
+
+#else
+
+#define vnc_jobs_clear(vs) (void) (vs);
+#define vnc_jobs_join(vs) (void) (vs);
+
+VncJob *vnc_job_new(VncState *vs);
+bool vnc_has_job(VncState *vs);
+int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h);
+bool vnc_worker_thread_running(void);
+void vnc_job_push(VncJob *job);
+
+#endif /* CONFIG_VNC_THREAD */
+
 #endif /* __QEMU_VNC_H */
-- 
1.7.1

[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Alexander Graf]

On 04.06.2010, at 15:20, Corentin Chary wrote:

> Implement a threaded VNC server using the producer-consumer model.
> The main thread will push encoding jobs (a list a rectangles to update)
> in a queue, and the VNC worker thread will consume that queue and send
> framebuffer updates to the output buffer.

Hrm, does it merge the queue? So imagine the following gets put in the queue

[0,0] - [100,100]
[100,0] - [200,10]
[0,0] - [200,10]

Would that code try and merge those three queue entries? Because by adding the queue, you basically get rid of the automatic merging we get from the lazy dirty bitmap fetching.

Before we were processing each entry and checked the bitmap again, seeing a merged bitmap of previous actions. With this I assume you push to-be-updated regions into the queue and mark them as processed before they are? Or does the processed marking still happen in the worker? Wouldn't that be racy? Hrm.


Alex

[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Corentin Chary]

On Fri, Jun 4, 2010 at 3:25 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 04.06.2010, at 15:20, Corentin Chary wrote:
>
>> Implement a threaded VNC server using the producer-consumer model.
>> The main thread will push encoding jobs (a list a rectangles to update)
>> in a queue, and the VNC worker thread will consume that queue and send
>> framebuffer updates to the output buffer.
>
> Hrm, does it merge the queue? So imagine the following gets put in the queue
>
> [0,0] - [100,100]
> [100,0] - [200,10]
> [0,0] - [200,10]
>
> Would that code try and merge those three queue entries? Because by adding the queue, you basically get rid of the automatic merging we get from the lazy dirty bitmap fetching.

I wanted to add that later.

> Before we were processing each entry and checked the bitmap again, seeing a merged bitmap of previous actions. With this I assume you push to-be-updated regions into the queue and mark them as processed before they are?

I mark them as processed as soon as they are put into the queue.
refresh() is also delayed while encoding job are processing (check the
trylock part) to avoid unecessary dirty bitmap updates and locks.

-- 
Corentin Chary
http://xf.iksaif.net

[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Alexander Graf]

On 04.06.2010, at 15:20, Corentin Chary wrote:

> Implement a threaded VNC server using the producer-consumer model.
> The main thread will push encoding jobs (a list a rectangles to update)
> in a queue, and the VNC worker thread will consume that queue and send
> framebuffer updates to the output buffer.
> 
> The threaded VNC server can be enabled with ./configure --enable-vnc-thread.
> 
> If you don't want it, just use ./configure --disable-vnc-thread and a syncrhonous
> queue of job will be used (which as exactly the same behavior as the old queue).
> If you disable the VNC thread, all thread related code will not be built and there will
> be no overhead.
> 
> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
> ---
> Makefile.objs      |    7 +-
> configure          |   13 ++
> ui/vnc-jobs-sync.c |   65 ++++++++++
> ui/vnc-jobs.c      |  351 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> ui/vnc.c           |  169 ++++++++++++++++++++++----
> ui/vnc.h           |   75 +++++++++++
> 6 files changed, 657 insertions(+), 23 deletions(-)
> create mode 100644 ui/vnc-jobs-sync.c
> create mode 100644 ui/vnc-jobs.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 22622a9..0c6334b 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -109,10 +109,15 @@ ui-obj-y += vnc-enc-tight.o
> ui-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
> ui-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
> ui-obj-$(CONFIG_COCOA) += cocoa.o
> +ifdef CONFIG_VNC_THREAD
> +ui-obj-y += vnc-jobs.o
> +else
> +ui-obj-y += vnc-jobs-sync.o
> +endif
> common-obj-y += $(addprefix ui/, $(ui-obj-y))
> 
> common-obj-y += iov.o acl.o
> -common-obj-$(CONFIG_IOTHREAD) += qemu-thread.o
> +common-obj-$(CONFIG_THREAD) += qemu-thread.o
> common-obj-y += notify.o event_notifier.o
> common-obj-y += qemu-timer.o
> 
> diff --git a/configure b/configure
> index 679f2fc..6f2e3a7 100755
> --- a/configure
> +++ b/configure
> @@ -264,6 +264,7 @@ vde=""
> vnc_tls=""
> vnc_sasl=""
> vnc_jpeg=""
> +vnc_thread=""
> xen=""
> linux_aio=""
> vhost_net=""
> @@ -552,6 +553,10 @@ for opt do
>   ;;
>   --enable-vnc-jpeg) vnc_jpeg="yes"
>   ;;
> +  --disable-vnc-thread) vnc_thread="no"
> +  ;;
> +  --enable-vnc-thread) vnc_thread="yes"
> +  ;;
>   --disable-slirp) slirp="no"
>   ;;
>   --disable-uuid) uuid="no"
> @@ -786,6 +791,8 @@ echo "  --disable-vnc-sasl       disable SASL encryption for VNC server"
> echo "  --enable-vnc-sasl        enable SASL encryption for VNC server"
> echo "  --disable-vnc-jpeg       disable JPEG lossy compression for VNC server"
> echo "  --enable-vnc-jpeg        enable JPEG lossy compression for VNC server"
> +echo "  --disable-vnc-thread     disable threaded VNC server"
> +echo "  --enable-vnc-thread      enable threaded VNC server"
> echo "  --disable-curses         disable curses output"
> echo "  --enable-curses          enable curses output"
> echo "  --disable-curl           disable curl connectivity"
> @@ -2048,6 +2055,7 @@ echo "Mixer emulation   $mixemu"
> echo "VNC TLS support   $vnc_tls"
> echo "VNC SASL support  $vnc_sasl"
> echo "VNC JPEG support  $vnc_jpeg"
> +echo "VNC thread        $vnc_thread"
> if test -n "$sparc_cpu"; then
>     echo "Target Sparc Arch $sparc_cpu"
> fi
> @@ -2191,6 +2199,10 @@ if test "$vnc_jpeg" = "yes" ; then
>   echo "CONFIG_VNC_JPEG=y" >> $config_host_mak
>   echo "VNC_JPEG_CFLAGS=$vnc_jpeg_cflags" >> $config_host_mak
> fi
> +if test "$vnc_thread" = "yes" ; then

So it's disabled by default? Sounds like a pretty cool and useful feature to me that should be enabled by default.

> +  echo "CONFIG_VNC_THREAD=y" >> $config_host_mak
> +  echo "CONFIG_THREAD=y" >> $config_host_mak
> +fi
> if test "$fnmatch" = "yes" ; then
>   echo "CONFIG_FNMATCH=y" >> $config_host_mak
> fi
> @@ -2267,6 +2279,7 @@ if test "$xen" = "yes" ; then
> fi
> if test "$io_thread" = "yes" ; then
>   echo "CONFIG_IOTHREAD=y" >> $config_host_mak
> +  echo "CONFIG_THREAD=y" >> $config_host_mak
> fi
> if test "$linux_aio" = "yes" ; then
>   echo "CONFIG_LINUX_AIO=y" >> $config_host_mak
> diff --git a/ui/vnc-jobs-sync.c b/ui/vnc-jobs-sync.c
> new file mode 100644
> index 0000000..9f138f5
> --- /dev/null
> +++ b/ui/vnc-jobs-sync.c
> @@ -0,0 +1,65 @@
> +/*
> + * QEMU VNC display driver
> + *
> + * Copyright (C) 2006 Anthony Liguori <anthony@codemonkey.ws>
> + * Copyright (C) 2006 Fabrice Bellard
> + * Copyright (C) 2009 Red Hat, Inc
> + * Copyright (C) 2010 Corentin Chary <corentin.chary@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +
> +#include "vnc.h"
> +
> +VncJob *vnc_job_new(VncState *vs)
> +{
> +    vs->job.vs = vs;
> +    vs->job.rectangles = 0;
> +
> +    vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
> +    vnc_write_u8(vs, 0);

Creating a job writes out a framebuffer update message? Why?

> +    vs->job.saved_offset = vs->output.offset;
> +    vnc_write_u16(vs, 0);
> +    return &vs->job;
> +}
> +
> +void vnc_job_push(VncJob *job)
> +{
> +    VncState *vs = job->vs;
> +
> +    vs->output.buffer[job->saved_offset] = (job->rectangles >> 8) & 0xFF;
> +    vs->output.buffer[job->saved_offset + 1] = job->rectangles & 0xFF;

There's a 16 bit little endian pointer helper somewhere. Probably better to use that - makes the code more readable.

> +    vnc_flush(job->vs);
> +}
> +
> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h)
> +{
> +    int n;
> +
> +    n = vnc_send_framebuffer_update(job->vs, x, y, w, h);
> +    if (n >= 0)
> +        job->rectangles += n;

Coding style.

> +    return n;
> +}
> +
> +bool vnc_has_job(VncState *vs)
> +{
> +    return false;
> +}
> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> new file mode 100644
> index 0000000..65ce5f8
> --- /dev/null
> +++ b/ui/vnc-jobs.c
> @@ -0,0 +1,351 @@
> +/*
> + * QEMU VNC display driver
> + *
> + * Copyright (C) 2006 Anthony Liguori <anthony@codemonkey.ws>
> + * Copyright (C) 2006 Fabrice Bellard
> + * Copyright (C) 2009 Red Hat, Inc
> + * Copyright (C) 2010 Corentin Chary <corentin.chary@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +
> +#include "vnc.h"
> +
> +/*
> + * Locking:
> + *
> + * There is three levels of locking:
> + * - jobs queue lock: for each operation on the queue (push, pop, isEmpty?)
> + * - VncDisplay global lock: mainly used for framebuffer updates to avoid
> + *                      screen corruption if the framebuffer is updated
> + *			while the worker is doing something.
> + * - VncState::output lock: used to make sure the output buffer is not corrupted
> + * 		   	 if two threads try to write on it at the same time
> + *
> + * While the VNC worker thread is working, the VncDisplay global lock is hold
> + * to avoid screen corruptions (this does not block vnc_refresh() because it
> + * uses trylock()) but the output lock is not hold because the thread work on
> + * its own output buffer.
> + * When the encoding job is done, the worker thread will hold the output lock
> + * and copy its output buffer in vs->output.
> +*/
> +
> +struct VncJobQueue {
> +    QemuCond cond;
> +    QemuMutex mutex;
> +    QemuThread thread;
> +    Buffer buffer;
> +    bool exit;
> +    QTAILQ_HEAD(, VncJob) jobs;
> +};
> +
> +typedef struct VncJobQueue VncJobQueue;
> +
> +/*
> + * We use a single global queue, but most of the functions are
> + * already reetrant, so we can easilly add more than one encoding thread
> + */
> +static VncJobQueue *queue;
> +
> +static void vnc_lock_queue(VncJobQueue *queue)
> +{
> +    qemu_mutex_lock(&queue->mutex);
> +}
> +
> +static void vnc_unlock_queue(VncJobQueue *queue)
> +{
> +    qemu_mutex_unlock(&queue->mutex);
> +}
> +
> +VncJob *vnc_job_new(VncState *vs)
> +{
> +    VncJob *job = qemu_mallocz(sizeof(VncJob));
> +
> +    job->vs = vs;
> +    vnc_lock_queue(queue);
> +    QLIST_INIT(&job->rectangles);
> +    vnc_unlock_queue(queue);
> +    return job;
> +}
> +
> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h)
> +{
> +    VncRectEntry *entry = qemu_mallocz(sizeof(VncRectEntry));
> +
> +    entry->rect.x = x;
> +    entry->rect.y = y;
> +    entry->rect.w = w;
> +    entry->rect.h = h;
> +
> +    vnc_lock_queue(queue);
> +    QLIST_INSERT_HEAD(&job->rectangles, entry, next);
> +    vnc_unlock_queue(queue);
> +    return 1;
> +}
> +
> +void vnc_job_push(VncJob *job)
> +{
> +    vnc_lock_queue(queue);
> +    if (QLIST_EMPTY(&job->rectangles)) {
> +        qemu_free(job);
> +    } else {
> +        QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
> +        qemu_cond_broadcast(&queue->cond);
> +    }
> +    vnc_unlock_queue(queue);
> +}
> +
> +static bool vnc_has_job_locked(VncState *vs)
> +{
> +    VncJob *job;
> +
> +    QTAILQ_FOREACH(job, &queue->jobs, next) {
> +        if (job->vs == vs || !vs) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +bool vnc_has_job(VncState *vs)
> +{
> +    bool ret;
> +
> +    vnc_lock_queue(queue);
> +    ret = vnc_has_job_locked(vs);
> +    vnc_unlock_queue(queue);
> +    return ret;
> +}
> +
> +void vnc_jobs_clear(VncState *vs)
> +{
> +    VncJob *job, *tmp;
> +
> +    vnc_lock_queue(queue);
> +    QTAILQ_FOREACH_SAFE(job, &queue->jobs, next, tmp) {
> +        if (job->vs == vs || !vs)
> +            QTAILQ_REMOVE(&queue->jobs, job, next);

Coding style...

> +    }
> +    vnc_unlock_queue(queue);
> +}
> +
> +void vnc_jobs_join(VncState *vs)
> +{
> +    vnc_lock_queue(queue);
> +    while (vnc_has_job_locked(vs)) {
> +        qemu_cond_wait(&queue->cond, &queue->mutex);
> +    }
> +    vnc_unlock_queue(queue);
> +}
> +
> +/*
> + * Copy data for local use
> + * FIXME: isolate what we use in a specific structure
> + * to avoid invalid usage in vnc-encoding-*.c and avoid copying
> + * because what we want is only want is only swapping VncState::output
> + * with the queue buffer
> + */
> +static void vnc_async_encoding_start(VncState *orig, VncState *local)
> +{
> +    local->vnc_encoding = orig->vnc_encoding;
> +    local->features = orig->features;
> +    local->ds = orig->ds;
> +    local->vd = orig->vd;
> +    local->write_pixels = orig->write_pixels;
> +    local->clientds = orig->clientds;
> +    local->tight_quality = orig->tight_quality;
> +    local->tight_compression = orig->tight_compression;
> +    local->tight_pixel24 = 0;
> +    local->tight = orig->tight;
> +    local->tight_tmp = orig->tight_tmp;
> +    local->tight_zlib = orig->tight_zlib;
> +    local->tight_gradient = orig->tight_gradient;
> +    local->tight_jpeg = orig->tight_jpeg;
> +    memcpy(local->tight_levels, orig->tight_levels, sizeof(orig->tight_levels));
> +    memcpy(local->tight_stream, orig->tight_stream, sizeof(orig->tight_stream));
> +    local->send_hextile_tile = orig->send_hextile_tile;
> +    local->zlib = orig->zlib;
> +    local->zlib_tmp = orig->zlib_tmp;
> +    local->zlib_stream = orig->zlib_stream;
> +    local->zlib_level = orig->zlib_level;
> +    local->output =  queue->buffer;
> +    local->csock = -1; /* Don't do any network work on this thread */

Wow, this looks like a lot of copies. How about a *local = *orig? Then just clean up the 3 variables you have to.

> +
> +    buffer_reset(&local->output);
> +}
> +
> +static void vnc_async_encoding_end(VncState *orig, VncState *local)
> +{
> +    orig->tight_quality = local->tight_quality;
> +    orig->tight_compression = local->tight_compression;
> +    orig->tight = local->tight;
> +    orig->tight_tmp = local->tight_tmp;
> +    orig->tight_zlib = local->tight_zlib;
> +    orig->tight_gradient = local->tight_gradient;
> +    orig->tight_jpeg = local->tight_jpeg;
> +    memcpy(orig->tight_levels, local->tight_levels, sizeof(local->tight_levels));
> +    memcpy(orig->tight_stream, local->tight_stream, sizeof(local->tight_stream));
> +    orig->zlib = local->zlib;
> +    orig->zlib_tmp = local->zlib_tmp;
> +    orig->zlib_stream = local->zlib_stream;
> +    orig->zlib_level = local->zlib_level;

This is probably a bit more complicated to get clean. How about putting the tight and the zlib information in structs, so you can just move those around? orig->tight = local->tight;

> +}
> +
> +static int vnc_worker_thread_loop(VncJobQueue *queue)
> +{
> +    VncJob *job;
> +    VncRectEntry *entry, *tmp;
> +    VncState vs;
> +    int n_rectangles;
> +    int saved_offset;
> +    bool flush;
> +
> +    vnc_lock_queue(queue);
> +    if (QTAILQ_EMPTY(&queue->jobs)) {
> +        qemu_cond_wait(&queue->cond, &queue->mutex);
> +    }
> +
> +    /* If the queue is empty, it's an exit order */
> +    if (QTAILQ_EMPTY(&queue->jobs)) {
> +        vnc_unlock_queue(queue);
> +        return -1;
> +    }
> +
> +    job = QTAILQ_FIRST(&queue->jobs);
> +    vnc_unlock_queue(queue);
> +
> +    qemu_mutex_lock(&job->vs->output_mutex);
> +    if (job->vs->csock == -1 || job->vs->abording == true) {
> +        goto disconnected;
> +    }
> +    qemu_mutex_unlock(&job->vs->output_mutex);
> +
> +    /* Make a local copy of vs and switch output buffers */
> +    vnc_async_encoding_start(job->vs, &vs);
> +
> +    /* Start sending rectangles */
> +    n_rectangles = 0;
> +    vnc_write_u8(&vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
> +    vnc_write_u8(&vs, 0);
> +    saved_offset = vs.output.offset;
> +    vnc_write_u16(&vs, 0);

This too strikes me as odd.

> +
> +    qemu_mutex_lock(&job->vs->vd->mutex);
> +    QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> +        int n;
> +
> +        if (job->vs->csock == -1) {
> +            qemu_mutex_unlock(&job->vs->vd->mutex);
> +            goto disconnected;
> +        }
> +
> +        n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
> +                                        entry->rect.w, entry->rect.h);

Wow, wait. You're coming from a rect, put everything in individual parameters and form them into a rect again afterwards? Just keep the rect :).

> +
> +        if (n >= 0)
> +            n_rectangles += n;
> +        qemu_free(entry);
> +    }
> +    qemu_mutex_unlock(&job->vs->vd->mutex);
> +
> +    /* Put n_rectangles at the beginning of the message */
> +    vs.output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
> +    vs.output.buffer[saved_offset + 1] = n_rectangles & 0xFF;

Deja vu?

> +
> +    /* Switch back buffers */
> +    qemu_mutex_lock(&job->vs->output_mutex);
> +    if (job->vs->csock == -1) {
> +        goto disconnected;
> +    }
> +
> +    vnc_write(job->vs, vs.output.buffer, vs.output.offset);
> +
> +disconnected:
> +    /* Copy persistent encoding data */
> +    vnc_async_encoding_end(job->vs, &vs);
> +    flush = (job->vs->csock != -1 && job->vs->abording != true);
> +    qemu_mutex_unlock(&job->vs->output_mutex);
> +
> +    if (flush) {
> +        vnc_flush(job->vs);
> +    }
> +
> +    qemu_mutex_lock(&queue->mutex);
> +    QTAILQ_REMOVE(&queue->jobs, job, next);
> +    qemu_mutex_unlock(&queue->mutex);
> +    qemu_cond_broadcast(&queue->cond);
> +    qemu_free(job);
> +    return 0;
> +}
> +
> +static VncJobQueue *vnc_queue_init(void)
> +{
> +    VncJobQueue *queue = qemu_mallocz(sizeof(VncJobQueue));
> +
> +    qemu_cond_init(&queue->cond);
> +    qemu_mutex_init(&queue->mutex);
> +    QTAILQ_INIT(&queue->jobs);
> +    return queue;
> +}
> +
> +static void vnc_queue_clear(VncJobQueue *q)
> +{
> +    qemu_cond_destroy(&queue->cond);
> +    qemu_mutex_destroy(&queue->mutex);
> +    buffer_free(&queue->buffer);
> +    qemu_free(q);
> +    queue = NULL; /* Unset global queue */
> +}
> +
> +static void *vnc_worker_thread(void *arg)
> +{
> +    VncJobQueue *queue = arg;
> +
> +    while (!vnc_worker_thread_loop(queue)) ;
> +    vnc_queue_clear(queue);
> +    return NULL;
> +}
> +
> +void vnc_start_worker_thread(void)
> +{
> +    VncJobQueue *q;
> +
> +    if (vnc_worker_thread_running())
> +        return ;
> +
> +    q = vnc_queue_init();
> +    qemu_thread_create(&q->thread, vnc_worker_thread, q);
> +    queue = q; /* Set global queue */
> +}
> +
> +bool vnc_worker_thread_running(void)
> +{
> +    return queue; /* Check global queue */
> +}
> +
> +void vnc_stop_worker_thread(void)
> +{
> +    if (!vnc_worker_thread_running())
> +        return ;
> +
> +    /* Remove all jobs and wake up the thread */
> +    vnc_jobs_clear(NULL);
> +    qemu_cond_broadcast(&queue->cond);
> +}
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e3ef315..f6475b3 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -45,6 +45,36 @@
>     } \
> }
> 
> +#ifdef CONFIG_VNC_THREAD
> +static int vnc_trylock_display(VncDisplay *vd)
> +{
> +    return qemu_mutex_trylock(&vd->mutex);
> +}
> +
> +static void vnc_unlock_display(VncDisplay *vd)
> +{
> +    qemu_mutex_unlock(&vd->mutex);
> +}
> +
> +static void vnc_lock_output(VncState *vs)
> +{
> +    qemu_mutex_lock(&vs->output_mutex);
> +}
> +
> +static void vnc_unlock_output(VncState *vs)
> +{
> +    qemu_mutex_unlock(&vs->output_mutex);
> +}

Not static, but static inline. I guess. But then again that doesn't really hurt - we're not in a header file.

> +#else
> +static int vnc_trylock_display(VncDisplay *vd)
> +{
> +    return 0;
> +}
> +
> +#define vnc_unlock_display(vs) (void) (vs);
> +#define vnc_lock_output(vs) (void) (vs);
> +#define vnc_unlock_output(vs) (void) (vs);

Please make those static functions too.

> +#endif
> 
> static VncDisplay *vnc_display; /* needed for info vnc */
> static DisplayChangeListener *dcl;
> @@ -363,6 +393,7 @@ static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
> */
> 
> static int vnc_update_client(VncState *vs, int has_dirty);
> +static int vnc_update_client_sync(VncState *vs, int has_dirty);
> static void vnc_disconnect_start(VncState *vs);
> static void vnc_disconnect_finish(VncState *vs);
> static void vnc_init_timer(VncDisplay *vd);
> @@ -493,6 +524,7 @@ void buffer_append(Buffer *buffer, const void *data, size_t len)
>     buffer->offset += len;
> }
> 
> +

Huh?

> static void vnc_desktop_resize(VncState *vs)
> {
>     DisplayState *ds = vs->ds;
> @@ -506,19 +538,46 @@ static void vnc_desktop_resize(VncState *vs)
>     }
>     vs->client_width = ds_get_width(ds);
>     vs->client_height = ds_get_height(ds);
> +    vnc_lock_output(vs);
>     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>     vnc_write_u8(vs, 0);
>     vnc_write_u16(vs, 1); /* number of rects */
>     vnc_framebuffer_update(vs, 0, 0, vs->client_width, vs->client_height,
>                            VNC_ENCODING_DESKTOPRESIZE);
> +    vnc_unlock_output(vs);
>     vnc_flush(vs);
> }
> 
> +#ifdef CONFIG_VNC_THREAD
> +static void vnc_abort_display_jobs(VncDisplay *vd)
> +{
> +    VncState *vs;
> +
> +    QTAILQ_FOREACH(vs, &vd->clients, next) {
> +        vnc_lock_output(vs);
> +        vs->abording = true;
> +        vnc_unlock_output(vs);
> +    }
> +    QTAILQ_FOREACH(vs, &vd->clients, next) {
> +        vnc_jobs_join(vs);
> +    }
> +    QTAILQ_FOREACH(vs, &vd->clients, next) {
> +        vnc_lock_output(vs);
> +        vs->abording = false;
> +        vnc_unlock_output(vs);
> +    }
> +}
> +#else
> +#define vnc_abort_display_jobs(vd)

see above

> +#endif
> +
> static void vnc_dpy_resize(DisplayState *ds)
> {
>     VncDisplay *vd = ds->opaque;
>     VncState *vs;
> 
> +    vnc_abort_display_jobs(vd);
> +
>     /* server surface */
>     if (!vd->server)
>         vd->server = qemu_mallocz(sizeof(*vd->server));
> @@ -646,7 +705,7 @@ int vnc_raw_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
>     return 1;
> }
> 
> -static int send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
> +int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
> {
>     int n = 0;
> 
> @@ -672,12 +731,14 @@ static int send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
> static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int dst_y, int w, int h)
> {
>     /* send bitblit op to the vnc client */
> +    vnc_lock_output(vs);
>     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>     vnc_write_u8(vs, 0);
>     vnc_write_u16(vs, 1); /* number of rects */
>     vnc_framebuffer_update(vs, dst_x, dst_y, w, h, VNC_ENCODING_COPYRECT);
>     vnc_write_u16(vs, src_x);
>     vnc_write_u16(vs, src_y);
> +    vnc_unlock_output(vs);
>     vnc_flush(vs);
> }
> 
> @@ -694,7 +755,7 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int
>     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
>         if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
>             vs->force_update = 1;
> -            vnc_update_client(vs, 1);
> +            vnc_update_client_sync(vs, 1);
>             /* vs might be free()ed here */
>         }
>     }
> @@ -814,15 +875,29 @@ static int find_and_clear_dirty_height(struct VncState *vs,
>     return h;
> }
> 
> +#ifdef CONFIG_VNC_THREAD
> +static int vnc_update_client_sync(VncState *vs, int has_dirty)
> +{
> +    int ret = vnc_update_client(vs, has_dirty);
> +    vnc_jobs_join(vs);
> +    return ret;
> +}
> +#else
> +static int vnc_update_client_sync(VncState *vs, int has_dirty)
> +{
> +    return vnc_update_client(vs, has_dirty);
> +}
> +#endif
> +
> static int vnc_update_client(VncState *vs, int has_dirty)
> {
>     if (vs->need_update && vs->csock != -1) {
>         VncDisplay *vd = vs->vd;
> +        VncJob *job;
>         int y;
> -        int n_rectangles;
> -        int saved_offset;
>         int width, height;
> -        int n;
> +        int n = 0;
> +
> 
>         if (vs->output.offset && !vs->audio_cap && !vs->force_update)
>             /* kernel send buffers are full -> drop frames to throttle */
> @@ -837,11 +912,7 @@ static int vnc_update_client(VncState *vs, int has_dirty)
>          * happening in parallel don't disturb us, the next pass will
>          * send them to the client.
>          */
> -        n_rectangles = 0;
> -        vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
> -        vnc_write_u8(vs, 0);
> -        saved_offset = vs->output.offset;
> -        vnc_write_u16(vs, 0);
> +        job = vnc_job_new(vs);
> 
>         width = MIN(vd->server->width, vs->client_width);
>         height = MIN(vd->server->height, vs->client_height);
> @@ -858,25 +929,23 @@ static int vnc_update_client(VncState *vs, int has_dirty)
>                 } else {
>                     if (last_x != -1) {
>                         int h = find_and_clear_dirty_height(vs, y, last_x, x);
> -                        n = send_framebuffer_update(vs, last_x * 16, y,
> -                                                    (x - last_x) * 16, h);
> -                        n_rectangles += n;
> +
> +                        n += vnc_job_add_rect(job, last_x * 16, y,
> +                                              (x - last_x) * 16, h);
>                     }
>                     last_x = -1;
>                 }
>             }
>             if (last_x != -1) {
>                 int h = find_and_clear_dirty_height(vs, y, last_x, x);
> -                n = send_framebuffer_update(vs, last_x * 16, y,
> -                                            (x - last_x) * 16, h);
> -                n_rectangles += n;
> +                n += vnc_job_add_rect(job, last_x * 16, y,
> +                                      (x - last_x) * 16, h);

Oh, so that's why. Well, better keep the individual parameters then.

>             }
>         }
> -        vs->output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
> -        vs->output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
> -        vnc_flush(vs);
> +
> +        vnc_job_push(job);
>         vs->force_update = 0;
> -        return n_rectangles;
> +        return n;

So by now the rest of the code thought you successfully processed that rect, right?

>     }
> 
>     if (vs->csock == -1)
> @@ -892,16 +961,20 @@ static void audio_capture_notify(void *opaque, audcnotification_e cmd)
> 
>     switch (cmd) {
>     case AUD_CNOTIFY_DISABLE:
> +        vnc_lock_output(vs);
>         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
>         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
>         vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_END);
> +        vnc_unlock_output(vs);
>         vnc_flush(vs);
>         break;
> 
>     case AUD_CNOTIFY_ENABLE:
> +        vnc_lock_output(vs);
>         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
>         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
>         vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_BEGIN);
> +        vnc_unlock_output(vs);
>         vnc_flush(vs);
>         break;
>     }
> @@ -915,11 +988,13 @@ static void audio_capture(void *opaque, void *buf, int size)
> {
>     VncState *vs = opaque;
> 
> +    vnc_lock_output(vs);
>     vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
>     vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
>     vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_DATA);
>     vnc_write_u32(vs, size);
>     vnc_write(vs, buf, size);
> +    vnc_unlock_output(vs);
>     vnc_flush(vs);
> }
> 
> @@ -961,6 +1036,9 @@ static void vnc_disconnect_start(VncState *vs)
> 
> static void vnc_disconnect_finish(VncState *vs)
> {
> +    vnc_jobs_join(vs); /* Wait encoding jobs */
> +
> +    vnc_lock_output(vs);
>     vnc_qmp_event(vs, QEVENT_VNC_DISCONNECTED);
> 
>     buffer_free(&vs->input);
> @@ -989,6 +1067,11 @@ static void vnc_disconnect_finish(VncState *vs)
>     vnc_remove_timer(vs->vd);
>     if (vs->vd->lock_key_sync)
>         qemu_remove_led_event_handler(vs->led);
> +    vnc_unlock_output(vs);
> +
> +#ifdef CONFIG_VNC_THREAD
> +    qemu_mutex_destroy(&vs->output_mutex);
> +#endif
>     qemu_free(vs);
> }
> 
> @@ -1108,7 +1191,7 @@ static long vnc_client_write_plain(VncState *vs)
>  * the client socket. Will delegate actual work according to whether
>  * SASL SSF layers are enabled (thus requiring encryption calls)
>  */
> -void vnc_client_write(void *opaque)
> +static void vnc_client_write_locked(void *opaque)
> {
>     VncState *vs = opaque;
> 
> @@ -1122,6 +1205,19 @@ void vnc_client_write(void *opaque)
>         vnc_client_write_plain(vs);
> }
> 
> +void vnc_client_write(void *opaque)
> +{
> +    VncState *vs = opaque;
> +
> +    vnc_lock_output(vs);
> +    if (vs->output.offset) {
> +        vnc_client_write_locked(opaque);
> +    } else {
> +        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
> +    }
> +    vnc_unlock_output(vs);
> +}
> +
> void vnc_read_when(VncState *vs, VncReadEvent *func, size_t expecting)
> {
>     vs->read_handler = func;
> @@ -1232,6 +1328,7 @@ void vnc_write(VncState *vs, const void *data, size_t len)
> {
>     buffer_reserve(&vs->output, len);
> 
> +

Eeh.

>     if (vs->csock != -1 && buffer_empty(&vs->output)) {
>         qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, vnc_client_write, vs);
>     }
> @@ -1273,8 +1370,10 @@ void vnc_write_u8(VncState *vs, uint8_t value)
> 
> void vnc_flush(VncState *vs)
> {
> +    vnc_lock_output(vs);
>     if (vs->csock != -1 && vs->output.offset)
> -        vnc_client_write(vs);
> +        vnc_client_write_locked(vs);

Please change the brackets while you're at it. Some genius thought it'd be a good idea to define a "new" coding style that's different from all the current qemu code. But consistency is better than none, so we need to stick with it now.

> +    vnc_unlock_output(vs);
> }
> 
> uint8_t read_u8(uint8_t *data, size_t offset)
> @@ -1309,12 +1408,14 @@ static void check_pointer_type_change(Notifier *notifier)
>     int absolute = kbd_mouse_is_absolute();
> 
>     if (vnc_has_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE) && vs->absolute != absolute) {
> +        vnc_lock_output(vs);
>         vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>         vnc_write_u8(vs, 0);
>         vnc_write_u16(vs, 1);
>         vnc_framebuffer_update(vs, absolute, 0,
>                                ds_get_width(vs->ds), ds_get_height(vs->ds),
>                                VNC_ENCODING_POINTER_TYPE_CHANGE);

Man I really think those framebuffer update message headers should be folded into the respective update function.

> +        vnc_unlock_output(vs);
>         vnc_flush(vs);
>     }
>     vs->absolute = absolute;
> @@ -1618,21 +1719,25 @@ static void framebuffer_update_request(VncState *vs, int incremental,
> 
> static void send_ext_key_event_ack(VncState *vs)
> {
> +    vnc_lock_output(vs);
>     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>     vnc_write_u8(vs, 0);
>     vnc_write_u16(vs, 1);
>     vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), ds_get_height(vs->ds),
>                            VNC_ENCODING_EXT_KEY_EVENT);
> +    vnc_unlock_output(vs);
>     vnc_flush(vs);
> }
> 
> static void send_ext_audio_ack(VncState *vs)
> {
> +    vnc_lock_output(vs);
>     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>     vnc_write_u8(vs, 0);
>     vnc_write_u16(vs, 1);
>     vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), ds_get_height(vs->ds),
>                            VNC_ENCODING_AUDIO);
> +    vnc_unlock_output(vs);
>     vnc_flush(vs);
> }
> 
> @@ -1791,12 +1896,14 @@ static void vnc_colordepth(VncState *vs)
> {
>     if (vnc_has_feature(vs, VNC_FEATURE_WMVI)) {
>         /* Sending a WMVi message to notify the client*/
> +        vnc_lock_output(vs);
>         vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>         vnc_write_u8(vs, 0);
>         vnc_write_u16(vs, 1); /* number of rects */
>         vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), 
>                                ds_get_height(vs->ds), VNC_ENCODING_WMVi);
>         pixel_format_message(vs);
> +        vnc_unlock_output(vs);
>         vnc_flush(vs);
>     } else {
>         set_pixel_conversion(vs);
> @@ -2224,12 +2331,21 @@ static void vnc_refresh(void *opaque)
> 
>     vga_hw_update();
> 
> +    if (vnc_trylock_display(vd)) {
> +        vd->timer_interval = VNC_REFRESH_INTERVAL_BASE;
> +        qemu_mod_timer(vd->timer, qemu_get_clock(rt_clock) +
> +                       vd->timer_interval);
> +        return;
> +    }
> +
>     has_dirty = vnc_refresh_server_surface(vd);
> +    vnc_unlock_display(vd);
> 
>     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
>         rects += vnc_update_client(vs, has_dirty);
>         /* vs might be free()ed here */
>     }
> +

...

>     /* vd->timer could be NULL now if the last client disconnected,
>      * in this case don't update the timer */
>     if (vd->timer == NULL)
> @@ -2288,6 +2404,10 @@ static void vnc_connect(VncDisplay *vd, int csock)
>     vs->as.fmt = AUD_FMT_S16;
>     vs->as.endianness = 0;
> 
> +#ifdef CONFIG_VNC_THREAD
> +    qemu_mutex_init(&vs->output_mutex);
> +#endif
> +
>     QTAILQ_INSERT_HEAD(&vd->clients, vs, next);
> 
>     vga_hw_update();
> @@ -2345,6 +2465,11 @@ void vnc_display_init(DisplayState *ds)
>     if (!vs->kbd_layout)
>         exit(1);
> 
> +#ifdef CONFIG_VNC_THREAD
> +    qemu_mutex_init(&vs->mutex);
> +    vnc_start_worker_thread();
> +#endif
> +
>     dcl->dpy_copy = vnc_dpy_copy;
>     dcl->dpy_update = vnc_dpy_update;
>     dcl->dpy_resize = vnc_dpy_resize;
> diff --git a/ui/vnc.h b/ui/vnc.h
> index cca1946..9405e61 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -29,6 +29,9 @@
> 
> #include "qemu-common.h"
> #include "qemu-queue.h"
> +#ifdef CONFIG_VNC_THREAD
> +#include "qemu-thread.h"
> +#endif
> #include "console.h"
> #include "monitor.h"
> #include "audio/audio.h"
> @@ -59,6 +62,9 @@ typedef struct Buffer
> } Buffer;
> 
> typedef struct VncState VncState;
> +typedef struct VncJob VncJob;
> +typedef struct VncRect VncRect;
> +typedef struct VncRectEntry VncRectEntry;
> 
> typedef int VncReadEvent(VncState *vs, uint8_t *data, size_t len);
> 
> @@ -101,6 +107,9 @@ struct VncDisplay
>     DisplayState *ds;
>     kbd_layout_t *kbd_layout;
>     int lock_key_sync;
> +#ifdef CONFIG_VNC_THREAD
> +    QemuMutex mutex;
> +#endif
> 
>     QEMUCursor *cursor;
>     int cursor_msize;
> @@ -122,6 +131,38 @@ struct VncDisplay
> #endif
> };
> 
> +
> +#ifdef CONFIG_VNC_THREAD
> +struct VncRect
> +{
> +    int x;
> +    int y;
> +    int w;
> +    int h;
> +};
> +
> +struct VncRectEntry
> +{
> +    struct VncRect rect;
> +    QLIST_ENTRY(VncRectEntry) next;
> +};
> +
> +struct VncJob
> +{
> +    VncState *vs;
> +
> +    QLIST_HEAD(, VncRectEntry) rectangles;
> +    QTAILQ_ENTRY(VncJob) next;
> +};
> +#else
> +struct VncJob
> +{
> +    VncState *vs;
> +    int rectangles;
> +    size_t saved_offset;
> +};
> +#endif
> +
> struct VncState
> {
>     int csock;
> @@ -170,6 +211,12 @@ struct VncState
>     QEMUPutLEDEntry *led;
> 
>     /* Encoding specific */
> +    bool abording;
> +#ifndef CONFIG_VNC_THREAD

Please stay on your positive attitude :)

> +    VncJob job;
> +#else
> +    QemuMutex output_mutex;
> +#endif
> 
>     /* Tight */
>     uint8_t tight_quality;
> @@ -412,6 +459,8 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
> void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v);
> 
> /* Encodings */
> +int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> +
> int vnc_raw_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> 
> int vnc_hextile_send_framebuffer_update(VncState *vs, int x,
> @@ -427,4 +476,30 @@ void vnc_zlib_clear(VncState *vs);
> int vnc_tight_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> void vnc_tight_clear(VncState *vs);
> 
> +/* Jobs */
> +#ifdef CONFIG_VNC_THREAD
> +
> +VncJob *vnc_job_new(VncState *vs);
> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h);
> +void vnc_job_push(VncJob *job);
> +bool vnc_has_job(VncState *vs);
> +void vnc_jobs_clear(VncState *vs);
> +void vnc_jobs_join(VncState *vs);
> +void vnc_start_worker_thread(void);
> +bool vnc_worker_thread_running(void);
> +void vnc_stop_worker_thread(void);
> +
> +#else
> +
> +#define vnc_jobs_clear(vs) (void) (vs);
> +#define vnc_jobs_join(vs) (void) (vs);

static inline functions please.

> +
> +VncJob *vnc_job_new(VncState *vs);
> +bool vnc_has_job(VncState *vs);
> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h);
> +bool vnc_worker_thread_running(void);
> +void vnc_job_push(VncJob *job);
> +
> +#endif /* CONFIG_VNC_THREAD */
> +
> #endif /* __QEMU_VNC_H */
> -- 
> 1.7.1
> 

[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Corentin Chary]

On Fri, Jun 4, 2010 at 3:44 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 04.06.2010, at 15:20, Corentin Chary wrote:
>
>> Implement a threaded VNC server using the producer-consumer model.
>> The main thread will push encoding jobs (a list a rectangles to update)
>> in a queue, and the VNC worker thread will consume that queue and send
>> framebuffer updates to the output buffer.
>>
>> The threaded VNC server can be enabled with ./configure --enable-vnc-thread.
>>
>> If you don't want it, just use ./configure --disable-vnc-thread and a syncrhonous
>> queue of job will be used (which as exactly the same behavior as the old queue).
>> If you disable the VNC thread, all thread related code will not be built and there will
>> be no overhead.
>>
>> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
>> ---
>> Makefile.objs      |    7 +-
>> configure          |   13 ++
>> ui/vnc-jobs-sync.c |   65 ++++++++++
>> ui/vnc-jobs.c      |  351 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ui/vnc.c           |  169 ++++++++++++++++++++++----
>> ui/vnc.h           |   75 +++++++++++
>> 6 files changed, 657 insertions(+), 23 deletions(-)
>> create mode 100644 ui/vnc-jobs-sync.c
>> create mode 100644 ui/vnc-jobs.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 22622a9..0c6334b 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -109,10 +109,15 @@ ui-obj-y += vnc-enc-tight.o
>> ui-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
>> ui-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
>> ui-obj-$(CONFIG_COCOA) += cocoa.o
>> +ifdef CONFIG_VNC_THREAD
>> +ui-obj-y += vnc-jobs.o
>> +else
>> +ui-obj-y += vnc-jobs-sync.o
>> +endif
>> common-obj-y += $(addprefix ui/, $(ui-obj-y))
>>
>> common-obj-y += iov.o acl.o
>> -common-obj-$(CONFIG_IOTHREAD) += qemu-thread.o
>> +common-obj-$(CONFIG_THREAD) += qemu-thread.o
>> common-obj-y += notify.o event_notifier.o
>> common-obj-y += qemu-timer.o
>>
>> diff --git a/configure b/configure
>> index 679f2fc..6f2e3a7 100755
>> --- a/configure
>> +++ b/configure
>> @@ -264,6 +264,7 @@ vde=""
>> vnc_tls=""
>> vnc_sasl=""
>> vnc_jpeg=""
>> +vnc_thread=""
>> xen=""
>> linux_aio=""
>> vhost_net=""
>> @@ -552,6 +553,10 @@ for opt do
>>   ;;
>>   --enable-vnc-jpeg) vnc_jpeg="yes"
>>   ;;
>> +  --disable-vnc-thread) vnc_thread="no"
>> +  ;;
>> +  --enable-vnc-thread) vnc_thread="yes"
>> +  ;;
>>   --disable-slirp) slirp="no"
>>   ;;
>>   --disable-uuid) uuid="no"
>> @@ -786,6 +791,8 @@ echo "  --disable-vnc-sasl       disable SASL encryption for VNC server"
>> echo "  --enable-vnc-sasl        enable SASL encryption for VNC server"
>> echo "  --disable-vnc-jpeg       disable JPEG lossy compression for VNC server"
>> echo "  --enable-vnc-jpeg        enable JPEG lossy compression for VNC server"
>> +echo "  --disable-vnc-thread     disable threaded VNC server"
>> +echo "  --enable-vnc-thread      enable threaded VNC server"
>> echo "  --disable-curses         disable curses output"
>> echo "  --enable-curses          enable curses output"
>> echo "  --disable-curl           disable curl connectivity"
>> @@ -2048,6 +2055,7 @@ echo "Mixer emulation   $mixemu"
>> echo "VNC TLS support   $vnc_tls"
>> echo "VNC SASL support  $vnc_sasl"
>> echo "VNC JPEG support  $vnc_jpeg"
>> +echo "VNC thread        $vnc_thread"
>> if test -n "$sparc_cpu"; then
>>     echo "Target Sparc Arch $sparc_cpu"
>> fi
>> @@ -2191,6 +2199,10 @@ if test "$vnc_jpeg" = "yes" ; then
>>   echo "CONFIG_VNC_JPEG=y" >> $config_host_mak
>>   echo "VNC_JPEG_CFLAGS=$vnc_jpeg_cflags" >> $config_host_mak
>> fi
>> +if test "$vnc_thread" = "yes" ; then
>
> So it's disabled by default? Sounds like a pretty cool and useful feature to me that should be enabled by default.

Because it's does not work on  windows (qemu-thread.c only uses
pthread) and because I don't want to break everything :)

>> +  echo "CONFIG_VNC_THREAD=y" >> $config_host_mak
>> +  echo "CONFIG_THREAD=y" >> $config_host_mak
>> +fi
>> if test "$fnmatch" = "yes" ; then
>>   echo "CONFIG_FNMATCH=y" >> $config_host_mak
>> fi
>> @@ -2267,6 +2279,7 @@ if test "$xen" = "yes" ; then
>> fi
>> if test "$io_thread" = "yes" ; then
>>   echo "CONFIG_IOTHREAD=y" >> $config_host_mak
>> +  echo "CONFIG_THREAD=y" >> $config_host_mak
>> fi
>> if test "$linux_aio" = "yes" ; then
>>   echo "CONFIG_LINUX_AIO=y" >> $config_host_mak
>> diff --git a/ui/vnc-jobs-sync.c b/ui/vnc-jobs-sync.c
>> new file mode 100644
>> index 0000000..9f138f5
>> --- /dev/null
>> +++ b/ui/vnc-jobs-sync.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * QEMU VNC display driver
>> + *
>> + * Copyright (C) 2006 Anthony Liguori <anthony@codemonkey.ws>
>> + * Copyright (C) 2006 Fabrice Bellard
>> + * Copyright (C) 2009 Red Hat, Inc
>> + * Copyright (C) 2010 Corentin Chary <corentin.chary@gmail.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +
>> +#include "vnc.h"
>> +
>> +VncJob *vnc_job_new(VncState *vs)
>> +{
>> +    vs->job.vs = vs;
>> +    vs->job.rectangles = 0;
>> +
>> +    vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>> +    vnc_write_u8(vs, 0);
>
> Creating a job writes out a framebuffer update message? Why?

This is vnc-job-sync.c, since it's synchroneous, we have to start the
update here.

>> +    vs->job.saved_offset = vs->output.offset;
>> +    vnc_write_u16(vs, 0);
>> +    return &vs->job;
>> +}
>> +
>> +void vnc_job_push(VncJob *job)
>> +{
>> +    VncState *vs = job->vs;
>> +
>> +    vs->output.buffer[job->saved_offset] = (job->rectangles >> 8) & 0xFF;
>> +    vs->output.buffer[job->saved_offset + 1] = job->rectangles & 0xFF;
>
> There's a 16 bit little endian pointer helper somewhere. Probably better to use that - makes the code more readable.

Hum right,

>> +    vnc_flush(job->vs);
>> +}
>> +
>> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h)
>> +{
>> +    int n;
>> +
>> +    n = vnc_send_framebuffer_update(job->vs, x, y, w, h);
>> +    if (n >= 0)
>> +        job->rectangles += n;
>
> Coding style.
>
>> +    return n;
>> +}
>> +
>> +bool vnc_has_job(VncState *vs)
>> +{
>> +    return false;
>> +}
>> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
>> new file mode 100644
>> index 0000000..65ce5f8
>> --- /dev/null
>> +++ b/ui/vnc-jobs.c
>> @@ -0,0 +1,351 @@
>> +/*
>> + * QEMU VNC display driver
>> + *
>> + * Copyright (C) 2006 Anthony Liguori <anthony@codemonkey.ws>
>> + * Copyright (C) 2006 Fabrice Bellard
>> + * Copyright (C) 2009 Red Hat, Inc
>> + * Copyright (C) 2010 Corentin Chary <corentin.chary@gmail.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +
>> +#include "vnc.h"
>> +
>> +/*
>> + * Locking:
>> + *
>> + * There is three levels of locking:
>> + * - jobs queue lock: for each operation on the queue (push, pop, isEmpty?)
>> + * - VncDisplay global lock: mainly used for framebuffer updates to avoid
>> + *                      screen corruption if the framebuffer is updated
>> + *                   while the worker is doing something.
>> + * - VncState::output lock: used to make sure the output buffer is not corrupted
>> + *                    if two threads try to write on it at the same time
>> + *
>> + * While the VNC worker thread is working, the VncDisplay global lock is hold
>> + * to avoid screen corruptions (this does not block vnc_refresh() because it
>> + * uses trylock()) but the output lock is not hold because the thread work on
>> + * its own output buffer.
>> + * When the encoding job is done, the worker thread will hold the output lock
>> + * and copy its output buffer in vs->output.
>> +*/
>> +
>> +struct VncJobQueue {
>> +    QemuCond cond;
>> +    QemuMutex mutex;
>> +    QemuThread thread;
>> +    Buffer buffer;
>> +    bool exit;
>> +    QTAILQ_HEAD(, VncJob) jobs;
>> +};
>> +
>> +typedef struct VncJobQueue VncJobQueue;
>> +
>> +/*
>> + * We use a single global queue, but most of the functions are
>> + * already reetrant, so we can easilly add more than one encoding thread
>> + */
>> +static VncJobQueue *queue;
>> +
>> +static void vnc_lock_queue(VncJobQueue *queue)
>> +{
>> +    qemu_mutex_lock(&queue->mutex);
>> +}
>> +
>> +static void vnc_unlock_queue(VncJobQueue *queue)
>> +{
>> +    qemu_mutex_unlock(&queue->mutex);
>> +}
>> +
>> +VncJob *vnc_job_new(VncState *vs)
>> +{
>> +    VncJob *job = qemu_mallocz(sizeof(VncJob));
>> +
>> +    job->vs = vs;
>> +    vnc_lock_queue(queue);
>> +    QLIST_INIT(&job->rectangles);
>> +    vnc_unlock_queue(queue);
>> +    return job;
>> +}
>> +
>> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h)
>> +{
>> +    VncRectEntry *entry = qemu_mallocz(sizeof(VncRectEntry));
>> +
>> +    entry->rect.x = x;
>> +    entry->rect.y = y;
>> +    entry->rect.w = w;
>> +    entry->rect.h = h;
>> +
>> +    vnc_lock_queue(queue);
>> +    QLIST_INSERT_HEAD(&job->rectangles, entry, next);
>> +    vnc_unlock_queue(queue);
>> +    return 1;
>> +}
>> +
>> +void vnc_job_push(VncJob *job)
>> +{
>> +    vnc_lock_queue(queue);
>> +    if (QLIST_EMPTY(&job->rectangles)) {
>> +        qemu_free(job);
>> +    } else {
>> +        QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
>> +        qemu_cond_broadcast(&queue->cond);
>> +    }
>> +    vnc_unlock_queue(queue);
>> +}
>> +
>> +static bool vnc_has_job_locked(VncState *vs)
>> +{
>> +    VncJob *job;
>> +
>> +    QTAILQ_FOREACH(job, &queue->jobs, next) {
>> +        if (job->vs == vs || !vs) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>> +bool vnc_has_job(VncState *vs)
>> +{
>> +    bool ret;
>> +
>> +    vnc_lock_queue(queue);
>> +    ret = vnc_has_job_locked(vs);
>> +    vnc_unlock_queue(queue);
>> +    return ret;
>> +}
>> +
>> +void vnc_jobs_clear(VncState *vs)
>> +{
>> +    VncJob *job, *tmp;
>> +
>> +    vnc_lock_queue(queue);
>> +    QTAILQ_FOREACH_SAFE(job, &queue->jobs, next, tmp) {
>> +        if (job->vs == vs || !vs)
>> +            QTAILQ_REMOVE(&queue->jobs, job, next);
>
> Coding style...
>
>> +    }
>> +    vnc_unlock_queue(queue);
>> +}
>> +
>> +void vnc_jobs_join(VncState *vs)
>> +{
>> +    vnc_lock_queue(queue);
>> +    while (vnc_has_job_locked(vs)) {
>> +        qemu_cond_wait(&queue->cond, &queue->mutex);
>> +    }
>> +    vnc_unlock_queue(queue);
>> +}
>> +
>> +/*
>> + * Copy data for local use
>> + * FIXME: isolate what we use in a specific structure
>> + * to avoid invalid usage in vnc-encoding-*.c and avoid copying
>> + * because what we want is only want is only swapping VncState::output
>> + * with the queue buffer
>> + */
>> +static void vnc_async_encoding_start(VncState *orig, VncState *local)
>> +{
>> +    local->vnc_encoding = orig->vnc_encoding;
>> +    local->features = orig->features;
>> +    local->ds = orig->ds;
>> +    local->vd = orig->vd;
>> +    local->write_pixels = orig->write_pixels;
>> +    local->clientds = orig->clientds;
>> +    local->tight_quality = orig->tight_quality;
>> +    local->tight_compression = orig->tight_compression;
>> +    local->tight_pixel24 = 0;
>> +    local->tight = orig->tight;
>> +    local->tight_tmp = orig->tight_tmp;
>> +    local->tight_zlib = orig->tight_zlib;
>> +    local->tight_gradient = orig->tight_gradient;
>> +    local->tight_jpeg = orig->tight_jpeg;
>> +    memcpy(local->tight_levels, orig->tight_levels, sizeof(orig->tight_levels));
>> +    memcpy(local->tight_stream, orig->tight_stream, sizeof(orig->tight_stream));
>> +    local->send_hextile_tile = orig->send_hextile_tile;
>> +    local->zlib = orig->zlib;
>> +    local->zlib_tmp = orig->zlib_tmp;
>> +    local->zlib_stream = orig->zlib_stream;
>> +    local->zlib_level = orig->zlib_level;
>> +    local->output =  queue->buffer;
>> +    local->csock = -1; /* Don't do any network work on this thread */
>
> Wow, this looks like a lot of copies. How about a *local = *orig? Then just clean up the 3 variables you have to.
>
>> +
>> +    buffer_reset(&local->output);
>> +}
>> +
>> +static void vnc_async_encoding_end(VncState *orig, VncState *local)
>> +{
>> +    orig->tight_quality = local->tight_quality;
>> +    orig->tight_compression = local->tight_compression;
>> +    orig->tight = local->tight;
>> +    orig->tight_tmp = local->tight_tmp;
>> +    orig->tight_zlib = local->tight_zlib;
>> +    orig->tight_gradient = local->tight_gradient;
>> +    orig->tight_jpeg = local->tight_jpeg;
>> +    memcpy(orig->tight_levels, local->tight_levels, sizeof(local->tight_levels));
>> +    memcpy(orig->tight_stream, local->tight_stream, sizeof(local->tight_stream));
>> +    orig->zlib = local->zlib;
>> +    orig->zlib_tmp = local->zlib_tmp;
>> +    orig->zlib_stream = local->zlib_stream;
>> +    orig->zlib_level = local->zlib_level;
>
> This is probably a bit more complicated to get clean. How about putting the tight and the zlib information in structs, so you can just move those around? orig->tight = local->tight;

Yep, I should use specific structures, but this will come in a later patch.

>> +}
>> +
>> +static int vnc_worker_thread_loop(VncJobQueue *queue)
>> +{
>> +    VncJob *job;
>> +    VncRectEntry *entry, *tmp;
>> +    VncState vs;
>> +    int n_rectangles;
>> +    int saved_offset;
>> +    bool flush;
>> +
>> +    vnc_lock_queue(queue);
>> +    if (QTAILQ_EMPTY(&queue->jobs)) {
>> +        qemu_cond_wait(&queue->cond, &queue->mutex);
>> +    }
>> +
>> +    /* If the queue is empty, it's an exit order */
>> +    if (QTAILQ_EMPTY(&queue->jobs)) {
>> +        vnc_unlock_queue(queue);
>> +        return -1;
>> +    }
>> +
>> +    job = QTAILQ_FIRST(&queue->jobs);
>> +    vnc_unlock_queue(queue);
>> +
>> +    qemu_mutex_lock(&job->vs->output_mutex);
>> +    if (job->vs->csock == -1 || job->vs->abording == true) {
>> +        goto disconnected;
>> +    }
>> +    qemu_mutex_unlock(&job->vs->output_mutex);
>> +
>> +    /* Make a local copy of vs and switch output buffers */
>> +    vnc_async_encoding_start(job->vs, &vs);
>> +
>> +    /* Start sending rectangles */
>> +    n_rectangles = 0;
>> +    vnc_write_u8(&vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>> +    vnc_write_u8(&vs, 0);
>> +    saved_offset = vs.output.offset;
>> +    vnc_write_u16(&vs, 0);
>
> This too strikes me as odd.
>
>> +
>> +    qemu_mutex_lock(&job->vs->vd->mutex);
>> +    QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
>> +        int n;
>> +
>> +        if (job->vs->csock == -1) {
>> +            qemu_mutex_unlock(&job->vs->vd->mutex);
>> +            goto disconnected;
>> +        }
>> +
>> +        n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
>> +                                        entry->rect.w, entry->rect.h);
>
> Wow, wait. You're coming from a rect, put everything in individual parameters and form them into a rect again afterwards? Just keep the rect :).
>
>> +
>> +        if (n >= 0)
>> +            n_rectangles += n;
>> +        qemu_free(entry);
>> +    }
>> +    qemu_mutex_unlock(&job->vs->vd->mutex);
>> +
>> +    /* Put n_rectangles at the beginning of the message */
>> +    vs.output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
>> +    vs.output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
>
> Deja vu?
>
>> +
>> +    /* Switch back buffers */
>> +    qemu_mutex_lock(&job->vs->output_mutex);
>> +    if (job->vs->csock == -1) {
>> +        goto disconnected;
>> +    }
>> +
>> +    vnc_write(job->vs, vs.output.buffer, vs.output.offset);
>> +
>> +disconnected:
>> +    /* Copy persistent encoding data */
>> +    vnc_async_encoding_end(job->vs, &vs);
>> +    flush = (job->vs->csock != -1 && job->vs->abording != true);
>> +    qemu_mutex_unlock(&job->vs->output_mutex);
>> +
>> +    if (flush) {
>> +        vnc_flush(job->vs);
>> +    }
>> +
>> +    qemu_mutex_lock(&queue->mutex);
>> +    QTAILQ_REMOVE(&queue->jobs, job, next);
>> +    qemu_mutex_unlock(&queue->mutex);
>> +    qemu_cond_broadcast(&queue->cond);
>> +    qemu_free(job);
>> +    return 0;
>> +}
>> +
>> +static VncJobQueue *vnc_queue_init(void)
>> +{
>> +    VncJobQueue *queue = qemu_mallocz(sizeof(VncJobQueue));
>> +
>> +    qemu_cond_init(&queue->cond);
>> +    qemu_mutex_init(&queue->mutex);
>> +    QTAILQ_INIT(&queue->jobs);
>> +    return queue;
>> +}
>> +
>> +static void vnc_queue_clear(VncJobQueue *q)
>> +{
>> +    qemu_cond_destroy(&queue->cond);
>> +    qemu_mutex_destroy(&queue->mutex);
>> +    buffer_free(&queue->buffer);
>> +    qemu_free(q);
>> +    queue = NULL; /* Unset global queue */
>> +}
>> +
>> +static void *vnc_worker_thread(void *arg)
>> +{
>> +    VncJobQueue *queue = arg;
>> +
>> +    while (!vnc_worker_thread_loop(queue)) ;
>> +    vnc_queue_clear(queue);
>> +    return NULL;
>> +}
>> +
>> +void vnc_start_worker_thread(void)
>> +{
>> +    VncJobQueue *q;
>> +
>> +    if (vnc_worker_thread_running())
>> +        return ;
>> +
>> +    q = vnc_queue_init();
>> +    qemu_thread_create(&q->thread, vnc_worker_thread, q);
>> +    queue = q; /* Set global queue */
>> +}
>> +
>> +bool vnc_worker_thread_running(void)
>> +{
>> +    return queue; /* Check global queue */
>> +}
>> +
>> +void vnc_stop_worker_thread(void)
>> +{
>> +    if (!vnc_worker_thread_running())
>> +        return ;
>> +
>> +    /* Remove all jobs and wake up the thread */
>> +    vnc_jobs_clear(NULL);
>> +    qemu_cond_broadcast(&queue->cond);
>> +}
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index e3ef315..f6475b3 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -45,6 +45,36 @@
>>     } \
>> }
>>
>> +#ifdef CONFIG_VNC_THREAD
>> +static int vnc_trylock_display(VncDisplay *vd)
>> +{
>> +    return qemu_mutex_trylock(&vd->mutex);
>> +}
>> +
>> +static void vnc_unlock_display(VncDisplay *vd)
>> +{
>> +    qemu_mutex_unlock(&vd->mutex);
>> +}
>> +
>> +static void vnc_lock_output(VncState *vs)
>> +{
>> +    qemu_mutex_lock(&vs->output_mutex);
>> +}
>> +
>> +static void vnc_unlock_output(VncState *vs)
>> +{
>> +    qemu_mutex_unlock(&vs->output_mutex);
>> +}
>
> Not static, but static inline. I guess. But then again that doesn't really hurt - we're not in a header file.

I think it's better to let gcc decide if this should be inlined or
not. (Here it will probably be inline with -O2).

>> +#else
>> +static int vnc_trylock_display(VncDisplay *vd)
>> +{
>> +    return 0;
>> +}
>> +
>> +#define vnc_unlock_display(vs) (void) (vs);
>> +#define vnc_lock_output(vs) (void) (vs);
>> +#define vnc_unlock_output(vs) (void) (vs);
>
> Please make those static functions too.
Ok,

>> +#endif
>>
>> static VncDisplay *vnc_display; /* needed for info vnc */
>> static DisplayChangeListener *dcl;
>> @@ -363,6 +393,7 @@ static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
>> */
>>
>> static int vnc_update_client(VncState *vs, int has_dirty);
>> +static int vnc_update_client_sync(VncState *vs, int has_dirty);
>> static void vnc_disconnect_start(VncState *vs);
>> static void vnc_disconnect_finish(VncState *vs);
>> static void vnc_init_timer(VncDisplay *vd);
>> @@ -493,6 +524,7 @@ void buffer_append(Buffer *buffer, const void *data, size_t len)
>>     buffer->offset += len;
>> }
>>
>> +
>
> Huh?
>
>> static void vnc_desktop_resize(VncState *vs)
>> {
>>     DisplayState *ds = vs->ds;
>> @@ -506,19 +538,46 @@ static void vnc_desktop_resize(VncState *vs)
>>     }
>>     vs->client_width = ds_get_width(ds);
>>     vs->client_height = ds_get_height(ds);
>> +    vnc_lock_output(vs);
>>     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>>     vnc_write_u8(vs, 0);
>>     vnc_write_u16(vs, 1); /* number of rects */
>>     vnc_framebuffer_update(vs, 0, 0, vs->client_width, vs->client_height,
>>                            VNC_ENCODING_DESKTOPRESIZE);
>> +    vnc_unlock_output(vs);
>>     vnc_flush(vs);
>> }
>>
>> +#ifdef CONFIG_VNC_THREAD
>> +static void vnc_abort_display_jobs(VncDisplay *vd)
>> +{
>> +    VncState *vs;
>> +
>> +    QTAILQ_FOREACH(vs, &vd->clients, next) {
>> +        vnc_lock_output(vs);
>> +        vs->abording = true;
>> +        vnc_unlock_output(vs);
>> +    }
>> +    QTAILQ_FOREACH(vs, &vd->clients, next) {
>> +        vnc_jobs_join(vs);
>> +    }
>> +    QTAILQ_FOREACH(vs, &vd->clients, next) {
>> +        vnc_lock_output(vs);
>> +        vs->abording = false;
>> +        vnc_unlock_output(vs);
>> +    }
>> +}
>> +#else
>> +#define vnc_abort_display_jobs(vd)
>
> see above
>
>> +#endif
>> +
>> static void vnc_dpy_resize(DisplayState *ds)
>> {
>>     VncDisplay *vd = ds->opaque;
>>     VncState *vs;
>>
>> +    vnc_abort_display_jobs(vd);
>> +
>>     /* server surface */
>>     if (!vd->server)
>>         vd->server = qemu_mallocz(sizeof(*vd->server));
>> @@ -646,7 +705,7 @@ int vnc_raw_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
>>     return 1;
>> }
>>
>> -static int send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
>> +int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
>> {
>>     int n = 0;
>>
>> @@ -672,12 +731,14 @@ static int send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
>> static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int dst_y, int w, int h)
>> {
>>     /* send bitblit op to the vnc client */
>> +    vnc_lock_output(vs);
>>     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>>     vnc_write_u8(vs, 0);
>>     vnc_write_u16(vs, 1); /* number of rects */
>>     vnc_framebuffer_update(vs, dst_x, dst_y, w, h, VNC_ENCODING_COPYRECT);
>>     vnc_write_u16(vs, src_x);
>>     vnc_write_u16(vs, src_y);
>> +    vnc_unlock_output(vs);
>>     vnc_flush(vs);
>> }
>>
>> @@ -694,7 +755,7 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int
>>     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
>>         if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
>>             vs->force_update = 1;
>> -            vnc_update_client(vs, 1);
>> +            vnc_update_client_sync(vs, 1);
>>             /* vs might be free()ed here */
>>         }
>>     }
>> @@ -814,15 +875,29 @@ static int find_and_clear_dirty_height(struct VncState *vs,
>>     return h;
>> }
>>
>> +#ifdef CONFIG_VNC_THREAD
>> +static int vnc_update_client_sync(VncState *vs, int has_dirty)
>> +{
>> +    int ret = vnc_update_client(vs, has_dirty);
>> +    vnc_jobs_join(vs);
>> +    return ret;
>> +}
>> +#else
>> +static int vnc_update_client_sync(VncState *vs, int has_dirty)
>> +{
>> +    return vnc_update_client(vs, has_dirty);
>> +}
>> +#endif
>> +
>> static int vnc_update_client(VncState *vs, int has_dirty)
>> {
>>     if (vs->need_update && vs->csock != -1) {
>>         VncDisplay *vd = vs->vd;
>> +        VncJob *job;
>>         int y;
>> -        int n_rectangles;
>> -        int saved_offset;
>>         int width, height;
>> -        int n;
>> +        int n = 0;
>> +
>>
>>         if (vs->output.offset && !vs->audio_cap && !vs->force_update)
>>             /* kernel send buffers are full -> drop frames to throttle */
>> @@ -837,11 +912,7 @@ static int vnc_update_client(VncState *vs, int has_dirty)
>>          * happening in parallel don't disturb us, the next pass will
>>          * send them to the client.
>>          */
>> -        n_rectangles = 0;
>> -        vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>> -        vnc_write_u8(vs, 0);
>> -        saved_offset = vs->output.offset;
>> -        vnc_write_u16(vs, 0);
>> +        job = vnc_job_new(vs);
>>
>>         width = MIN(vd->server->width, vs->client_width);
>>         height = MIN(vd->server->height, vs->client_height);
>> @@ -858,25 +929,23 @@ static int vnc_update_client(VncState *vs, int has_dirty)
>>                 } else {
>>                     if (last_x != -1) {
>>                         int h = find_and_clear_dirty_height(vs, y, last_x, x);
>> -                        n = send_framebuffer_update(vs, last_x * 16, y,
>> -                                                    (x - last_x) * 16, h);
>> -                        n_rectangles += n;
>> +
>> +                        n += vnc_job_add_rect(job, last_x * 16, y,
>> +                                              (x - last_x) * 16, h);
>>                     }
>>                     last_x = -1;
>>                 }
>>             }
>>             if (last_x != -1) {
>>                 int h = find_and_clear_dirty_height(vs, y, last_x, x);
>> -                n = send_framebuffer_update(vs, last_x * 16, y,
>> -                                            (x - last_x) * 16, h);
>> -                n_rectangles += n;
>> +                n += vnc_job_add_rect(job, last_x * 16, y,
>> +                                      (x - last_x) * 16, h);
>
> Oh, so that's why. Well, better keep the individual parameters then.
>
>>             }
>>         }
>> -        vs->output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
>> -        vs->output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
>> -        vnc_flush(vs);
>> +
>> +        vnc_job_push(job);
>>         vs->force_update = 0;
>> -        return n_rectangles;
>> +        return n;
>
> So by now the rest of the code thought you successfully processed that rect, right?
Yes.

>>     }
>>
>>     if (vs->csock == -1)
>> @@ -892,16 +961,20 @@ static void audio_capture_notify(void *opaque, audcnotification_e cmd)
>>
>>     switch (cmd) {
>>     case AUD_CNOTIFY_DISABLE:
>> +        vnc_lock_output(vs);
>>         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
>>         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
>>         vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_END);
>> +        vnc_unlock_output(vs);
>>         vnc_flush(vs);
>>         break;
>>
>>     case AUD_CNOTIFY_ENABLE:
>> +        vnc_lock_output(vs);
>>         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
>>         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
>>         vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_BEGIN);
>> +        vnc_unlock_output(vs);
>>         vnc_flush(vs);
>>         break;
>>     }
>> @@ -915,11 +988,13 @@ static void audio_capture(void *opaque, void *buf, int size)
>> {
>>     VncState *vs = opaque;
>>
>> +    vnc_lock_output(vs);
>>     vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
>>     vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
>>     vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_DATA);
>>     vnc_write_u32(vs, size);
>>     vnc_write(vs, buf, size);
>> +    vnc_unlock_output(vs);
>>     vnc_flush(vs);
>> }
>>
>> @@ -961,6 +1036,9 @@ static void vnc_disconnect_start(VncState *vs)
>>
>> static void vnc_disconnect_finish(VncState *vs)
>> {
>> +    vnc_jobs_join(vs); /* Wait encoding jobs */
>> +
>> +    vnc_lock_output(vs);
>>     vnc_qmp_event(vs, QEVENT_VNC_DISCONNECTED);
>>
>>     buffer_free(&vs->input);
>> @@ -989,6 +1067,11 @@ static void vnc_disconnect_finish(VncState *vs)
>>     vnc_remove_timer(vs->vd);
>>     if (vs->vd->lock_key_sync)
>>         qemu_remove_led_event_handler(vs->led);
>> +    vnc_unlock_output(vs);
>> +
>> +#ifdef CONFIG_VNC_THREAD
>> +    qemu_mutex_destroy(&vs->output_mutex);
>> +#endif
>>     qemu_free(vs);
>> }
>>
>> @@ -1108,7 +1191,7 @@ static long vnc_client_write_plain(VncState *vs)
>>  * the client socket. Will delegate actual work according to whether
>>  * SASL SSF layers are enabled (thus requiring encryption calls)
>>  */
>> -void vnc_client_write(void *opaque)
>> +static void vnc_client_write_locked(void *opaque)
>> {
>>     VncState *vs = opaque;
>>
>> @@ -1122,6 +1205,19 @@ void vnc_client_write(void *opaque)
>>         vnc_client_write_plain(vs);
>> }
>>
>> +void vnc_client_write(void *opaque)
>> +{
>> +    VncState *vs = opaque;
>> +
>> +    vnc_lock_output(vs);
>> +    if (vs->output.offset) {
>> +        vnc_client_write_locked(opaque);
>> +    } else {
>> +        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
>> +    }
>> +    vnc_unlock_output(vs);
>> +}
>> +
>> void vnc_read_when(VncState *vs, VncReadEvent *func, size_t expecting)
>> {
>>     vs->read_handler = func;
>> @@ -1232,6 +1328,7 @@ void vnc_write(VncState *vs, const void *data, size_t len)
>> {
>>     buffer_reserve(&vs->output, len);
>>
>> +
>
> Eeh.
>
>>     if (vs->csock != -1 && buffer_empty(&vs->output)) {
>>         qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, vnc_client_write, vs);
>>     }
>> @@ -1273,8 +1370,10 @@ void vnc_write_u8(VncState *vs, uint8_t value)
>>
>> void vnc_flush(VncState *vs)
>> {
>> +    vnc_lock_output(vs);
>>     if (vs->csock != -1 && vs->output.offset)
>> -        vnc_client_write(vs);
>> +        vnc_client_write_locked(vs);
>
> Please change the brackets while you're at it. Some genius thought it'd be a good idea to define a "new" coding style that's different from all the current qemu code. But consistency is better than none, so we need to stick with it now.
>
>> +    vnc_unlock_output(vs);
>> }
>>
>> uint8_t read_u8(uint8_t *data, size_t offset)
>> @@ -1309,12 +1408,14 @@ static void check_pointer_type_change(Notifier *notifier)
>>     int absolute = kbd_mouse_is_absolute();
>>
>>     if (vnc_has_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE) && vs->absolute != absolute) {
>> +        vnc_lock_output(vs);
>>         vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>>         vnc_write_u8(vs, 0);
>>         vnc_write_u16(vs, 1);
>>         vnc_framebuffer_update(vs, absolute, 0,
>>                                ds_get_width(vs->ds), ds_get_height(vs->ds),
>>                                VNC_ENCODING_POINTER_TYPE_CHANGE);
>
> Man I really think those framebuffer update message headers should be folded into the respective update function.

Yep

>> +        vnc_unlock_output(vs);
>>         vnc_flush(vs);
>>     }
>>     vs->absolute = absolute;
>> @@ -1618,21 +1719,25 @@ static void framebuffer_update_request(VncState *vs, int incremental,
>>
>> static void send_ext_key_event_ack(VncState *vs)
>> {
>> +    vnc_lock_output(vs);
>>     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>>     vnc_write_u8(vs, 0);
>>     vnc_write_u16(vs, 1);
>>     vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), ds_get_height(vs->ds),
>>                            VNC_ENCODING_EXT_KEY_EVENT);
>> +    vnc_unlock_output(vs);
>>     vnc_flush(vs);
>> }
>>
>> static void send_ext_audio_ack(VncState *vs)
>> {
>> +    vnc_lock_output(vs);
>>     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>>     vnc_write_u8(vs, 0);
>>     vnc_write_u16(vs, 1);
>>     vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), ds_get_height(vs->ds),
>>                            VNC_ENCODING_AUDIO);
>> +    vnc_unlock_output(vs);
>>     vnc_flush(vs);
>> }
>>
>> @@ -1791,12 +1896,14 @@ static void vnc_colordepth(VncState *vs)
>> {
>>     if (vnc_has_feature(vs, VNC_FEATURE_WMVI)) {
>>         /* Sending a WMVi message to notify the client*/
>> +        vnc_lock_output(vs);
>>         vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
>>         vnc_write_u8(vs, 0);
>>         vnc_write_u16(vs, 1); /* number of rects */
>>         vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds),
>>                                ds_get_height(vs->ds), VNC_ENCODING_WMVi);
>>         pixel_format_message(vs);
>> +        vnc_unlock_output(vs);
>>         vnc_flush(vs);
>>     } else {
>>         set_pixel_conversion(vs);
>> @@ -2224,12 +2331,21 @@ static void vnc_refresh(void *opaque)
>>
>>     vga_hw_update();
>>
>> +    if (vnc_trylock_display(vd)) {
>> +        vd->timer_interval = VNC_REFRESH_INTERVAL_BASE;
>> +        qemu_mod_timer(vd->timer, qemu_get_clock(rt_clock) +
>> +                       vd->timer_interval);
>> +        return;
>> +    }
>> +
>>     has_dirty = vnc_refresh_server_surface(vd);
>> +    vnc_unlock_display(vd);
>>
>>     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
>>         rects += vnc_update_client(vs, has_dirty);
>>         /* vs might be free()ed here */
>>     }
>> +
>
> ...
>
>>     /* vd->timer could be NULL now if the last client disconnected,
>>      * in this case don't update the timer */
>>     if (vd->timer == NULL)
>> @@ -2288,6 +2404,10 @@ static void vnc_connect(VncDisplay *vd, int csock)
>>     vs->as.fmt = AUD_FMT_S16;
>>     vs->as.endianness = 0;
>>
>> +#ifdef CONFIG_VNC_THREAD
>> +    qemu_mutex_init(&vs->output_mutex);
>> +#endif
>> +
>>     QTAILQ_INSERT_HEAD(&vd->clients, vs, next);
>>
>>     vga_hw_update();
>> @@ -2345,6 +2465,11 @@ void vnc_display_init(DisplayState *ds)
>>     if (!vs->kbd_layout)
>>         exit(1);
>>
>> +#ifdef CONFIG_VNC_THREAD
>> +    qemu_mutex_init(&vs->mutex);
>> +    vnc_start_worker_thread();
>> +#endif
>> +
>>     dcl->dpy_copy = vnc_dpy_copy;
>>     dcl->dpy_update = vnc_dpy_update;
>>     dcl->dpy_resize = vnc_dpy_resize;
>> diff --git a/ui/vnc.h b/ui/vnc.h
>> index cca1946..9405e61 100644
>> --- a/ui/vnc.h
>> +++ b/ui/vnc.h
>> @@ -29,6 +29,9 @@
>>
>> #include "qemu-common.h"
>> #include "qemu-queue.h"
>> +#ifdef CONFIG_VNC_THREAD
>> +#include "qemu-thread.h"
>> +#endif
>> #include "console.h"
>> #include "monitor.h"
>> #include "audio/audio.h"
>> @@ -59,6 +62,9 @@ typedef struct Buffer
>> } Buffer;
>>
>> typedef struct VncState VncState;
>> +typedef struct VncJob VncJob;
>> +typedef struct VncRect VncRect;
>> +typedef struct VncRectEntry VncRectEntry;
>>
>> typedef int VncReadEvent(VncState *vs, uint8_t *data, size_t len);
>>
>> @@ -101,6 +107,9 @@ struct VncDisplay
>>     DisplayState *ds;
>>     kbd_layout_t *kbd_layout;
>>     int lock_key_sync;
>> +#ifdef CONFIG_VNC_THREAD
>> +    QemuMutex mutex;
>> +#endif
>>
>>     QEMUCursor *cursor;
>>     int cursor_msize;
>> @@ -122,6 +131,38 @@ struct VncDisplay
>> #endif
>> };
>>
>> +
>> +#ifdef CONFIG_VNC_THREAD
>> +struct VncRect
>> +{
>> +    int x;
>> +    int y;
>> +    int w;
>> +    int h;
>> +};
>> +
>> +struct VncRectEntry
>> +{
>> +    struct VncRect rect;
>> +    QLIST_ENTRY(VncRectEntry) next;
>> +};
>> +
>> +struct VncJob
>> +{
>> +    VncState *vs;
>> +
>> +    QLIST_HEAD(, VncRectEntry) rectangles;
>> +    QTAILQ_ENTRY(VncJob) next;
>> +};
>> +#else
>> +struct VncJob
>> +{
>> +    VncState *vs;
>> +    int rectangles;
>> +    size_t saved_offset;
>> +};
>> +#endif
>> +
>> struct VncState
>> {
>>     int csock;
>> @@ -170,6 +211,12 @@ struct VncState
>>     QEMUPutLEDEntry *led;
>>
>>     /* Encoding specific */
>> +    bool abording;
>> +#ifndef CONFIG_VNC_THREAD
>
> Please stay on your positive attitude :)
>
>> +    VncJob job;
>> +#else
>> +    QemuMutex output_mutex;
>> +#endif
>>
>>     /* Tight */
>>     uint8_t tight_quality;
>> @@ -412,6 +459,8 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
>> void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v);
>>
>> /* Encodings */
>> +int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
>> +
>> int vnc_raw_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
>>
>> int vnc_hextile_send_framebuffer_update(VncState *vs, int x,
>> @@ -427,4 +476,30 @@ void vnc_zlib_clear(VncState *vs);
>> int vnc_tight_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
>> void vnc_tight_clear(VncState *vs);
>>
>> +/* Jobs */
>> +#ifdef CONFIG_VNC_THREAD
>> +
>> +VncJob *vnc_job_new(VncState *vs);
>> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h);
>> +void vnc_job_push(VncJob *job);
>> +bool vnc_has_job(VncState *vs);
>> +void vnc_jobs_clear(VncState *vs);
>> +void vnc_jobs_join(VncState *vs);
>> +void vnc_start_worker_thread(void);
>> +bool vnc_worker_thread_running(void);
>> +void vnc_stop_worker_thread(void);
>> +
>> +#else
>> +
>> +#define vnc_jobs_clear(vs) (void) (vs);
>> +#define vnc_jobs_join(vs) (void) (vs);
>
> static inline functions please.
>
>> +
>> +VncJob *vnc_job_new(VncState *vs);
>> +bool vnc_has_job(VncState *vs);
>> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h);
>> +bool vnc_worker_thread_running(void);
>> +void vnc_job_push(VncJob *job);
>> +
>> +#endif /* CONFIG_VNC_THREAD */
>> +
>> #endif /* __QEMU_VNC_H */
>> --
>> 1.7.1
>>
>
>



-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Avi Kivity]

On 06/05/2010 11:03 AM, Corentin Chary wrote:
>>
>> So it's disabled by default? Sounds like a pretty cool and useful feature to me that should be enabled by default.
>>      
> Because it's does not work on  windows (qemu-thread.c only uses
> pthread) and because I don't want to break everything :)
>    

One option is to disable vnc on Windows and let a Windows maintainer 
materialize and add the corresponding support.  Introducing more and 
more config options is not a good approach.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Corentin Chary]

On Sun, Jun 6, 2010 at 3:54 PM, Avi Kivity <avi@redhat.com> wrote:
> On 06/05/2010 11:03 AM, Corentin Chary wrote:
>>>
>>> So it's disabled by default? Sounds like a pretty cool and useful feature
>>> to me that should be enabled by default.
>>>
>>
>> Because it's does not work on  windows (qemu-thread.c only uses
>> pthread) and because I don't want to break everything :)
>>
>
> One option is to disable vnc on Windows and let a Windows maintainer
> materialize and add the corresponding support.  Introducing more and more
> config options is not a good approach.

I think keeping the non-threaded code is a good thing (and there is
not much code). There is probably case where you want to avoid
threads.



-- 
Corentin Chary
http://xf.iksaif.net

[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Paolo Bonzini]

On 06/06/2010 03:54 PM, Avi Kivity wrote:
> One option is to disable vnc on Windows and let a Windows maintainer
> materialize and add the corresponding support.

Adding Windows support to qemu-threads must be done anyway sooner or 
later and most of the code can be found for example in git.

Being a QEMU/Windows maintainer is a bit harder than that though, 
especially if at some point someone would like iothread to become the 
only supported option...

Paolo

[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Paolo Bonzini]

> +    vnc_lock_queue(queue);
> +    if (QTAILQ_EMPTY(&queue->jobs)) {
> +        qemu_cond_wait(&queue->cond,&queue->mutex);
> +    }
> +
> +    /* If the queue is empty, it's an exit order */
> +    if (QTAILQ_EMPTY(&queue->jobs)) {
> +        vnc_unlock_queue(queue);
> +        return -1;
> +    }
> +
> +    job = QTAILQ_FIRST(&queue->jobs);
> +    vnc_unlock_queue(queue);

Wrong usage of condition variables...

> +    flush = (job->vs->csock != -1&&  job->vs->abording != true);

and typo still there.

> +static void *vnc_worker_thread(void *arg)
> +{
> +    VncJobQueue *queue = arg;

Also, it's better (future proof) to call qemu_thread_self here.

Thanks!

Paolo

[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Corentin Chary]

On Fri, Jun 4, 2010 at 4:41 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> +    vnc_lock_queue(queue);
>> +    if (QTAILQ_EMPTY(&queue->jobs)) {
>> +        qemu_cond_wait(&queue->cond,&queue->mutex);
>> +    }
>> +
>> +    /* If the queue is empty, it's an exit order */
>> +    if (QTAILQ_EMPTY(&queue->jobs)) {
>> +        vnc_unlock_queue(queue);
>> +        return -1;
>> +    }
>> +
>> +    job = QTAILQ_FIRST(&queue->jobs);
>> +    vnc_unlock_queue(queue);
>
> Wrong usage of condition variables...

Because it's not in a loop ? Like I said in the cover-letter (but this
should probably be in a comment here) you can't use multiple worker
on a single queue because of zlib streams. But if the issue is more a
style issue than the actual implementation, then I can use a loop and
an exit flag.

>> +    flush = (job->vs->csock != -1&&  job->vs->abording != true);
>
> and typo still there.

Ooops, I did it again.

>> +static void *vnc_worker_thread(void *arg)
>> +{
>> +    VncJobQueue *queue = arg;
>
> Also, it's better (future proof) to call qemu_thread_self here.

Ok,


-- 
Corentin Chary
http://xf.iksaif.net

[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server

[Paolo Bonzini]

On 06/04/2010 04:55 PM, Corentin Chary wrote:
> Because it's not in a loop ? Like I said in the cover-letter (but this
> should probably be in a comment here) you can't use multiple worker
> on a single queue because of zlib streams. But if the issue is more a
> style issue than the actual implementation, then I can use a loop and
> an exit flag.

Yes, it's both a style issue and a future-proofing issue.  I think it's 
better to be boring when it comes to locking. :-)

(The cover letter hasn't arrived yet, so I haven't read it).

Paolo

Re: [Qemu-devel] [PATCH v2 2/2] vnc: threaded VNC server

[Avi Kivity]

On 06/04/2010 04:20 PM, Corentin Chary wrote:
>
> +    if (vnc_trylock_display(vd)) {
> +        vd->timer_interval = VNC_REFRESH_INTERVAL_BASE;
> +        qemu_mod_timer(vd->timer, qemu_get_clock(rt_clock) +
> +                       vd->timer_interval);
> +        return;
> +    }
> +
>       has_dirty = vnc_refresh_server_surface(vd);
> +    vnc_unlock_display(vd);
>    

This could delay the update by quite a bit, no?

A more elaborate approach would be to enqueue the refresh job into the 
queue.  May need the iothread enabled so we have qemu_mutex.

btw, I could not find other uses of vd->mutex, shouldn't it protect 
against the work thread?


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 2/2] vnc: threaded VNC server

[Corentin Chary]

On Sun, Jun 6, 2010 at 4:11 PM, Avi Kivity <avi@redhat.com> wrote:
> On 06/04/2010 04:20 PM, Corentin Chary wrote:
>>
>> +    if (vnc_trylock_display(vd)) {
>> +        vd->timer_interval = VNC_REFRESH_INTERVAL_BASE;
>> +        qemu_mod_timer(vd->timer, qemu_get_clock(rt_clock) +
>> +                       vd->timer_interval);
>> +        return;
>> +    }
>> +
>>      has_dirty = vnc_refresh_server_surface(vd);
>> +    vnc_unlock_display(vd);
>>
>
> This could delay the update by quite a bit, no?

Yep, but it's far better than waiting the lock because it doesn't slow
down the main thread.
I played big buck bunny trailler (33sec) in mplayer and tight encoding:
- ~40 sec with the non-threaded server
- ~37 sec with a lock
- ~33 sec with a try_lock

> A more elaborate approach would be to enqueue the refresh job into the
> queue.  May need the iothread enabled so we have qemu_mutex.

Maybe, but I'd like to wait the generic async work subsystem before
adding different kind of jobs to the queue. And it's already a big
improvment over the current code :).

> btw, I could not find other uses of vd->mutex, shouldn't it protect against
> the work thread?

Check vnc-jobs.c, there is a qemu_mutex_lock(vs->vd->mutex);

-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] [PATCH v2 2/2] vnc: threaded VNC server

[Avi Kivity]

On 06/06/2010 05:48 PM, Corentin Chary wrote:
> On Sun, Jun 6, 2010 at 4:11 PM, Avi Kivity<avi@redhat.com>  wrote:
>    
>> On 06/04/2010 04:20 PM, Corentin Chary wrote:
>>      
>>> +    if (vnc_trylock_display(vd)) {
>>> +        vd->timer_interval = VNC_REFRESH_INTERVAL_BASE;
>>> +        qemu_mod_timer(vd->timer, qemu_get_clock(rt_clock) +
>>> +                       vd->timer_interval);
>>> +        return;
>>> +    }
>>> +
>>>       has_dirty = vnc_refresh_server_surface(vd);
>>> +    vnc_unlock_display(vd);
>>>
>>>        
>> This could delay the update by quite a bit, no?
>>      
> Yep, but it's far better than waiting the lock because it doesn't slow
> down the main thread.
> I played big buck bunny trailler (33sec) in mplayer and tight encoding:
> - ~40 sec with the non-threaded server
> - ~37 sec with a lock
> - ~33 sec with a try_lock
>    

Definitely, blocking the main thread is a no-no.

>> A more elaborate approach would be to enqueue the refresh job into the
>> queue.  May need the iothread enabled so we have qemu_mutex.
>>      
> Maybe, but I'd like to wait the generic async work subsystem before
> adding different kind of jobs to the queue. And it's already a big
> improvment over the current code :).
>    

Hm, ok.

>> btw, I could not find other uses of vd->mutex, shouldn't it protect against
>> the work thread?
>>      
> Check vnc-jobs.c, there is a qemu_mutex_lock(vs->vd->mutex);
>
>    

Shouldn't it use vnc_lock_display()?  That's why I missed it.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 2/2] vnc: threaded VNC server

[Corentin Chary]

On Sun, Jun 6, 2010 at 4:53 PM, Avi Kivity <avi@redhat.com> wrote:
> On 06/06/2010 05:48 PM, Corentin Chary wrote:
>>
>> On Sun, Jun 6, 2010 at 4:11 PM, Avi Kivity<avi@redhat.com>  wrote:
>>
>>>
>>> On 06/04/2010 04:20 PM, Corentin Chary wrote:
>>>
>>>>
>>>> +    if (vnc_trylock_display(vd)) {
>>>> +        vd->timer_interval = VNC_REFRESH_INTERVAL_BASE;
>>>> +        qemu_mod_timer(vd->timer, qemu_get_clock(rt_clock) +
>>>> +                       vd->timer_interval);
>>>> +        return;
>>>> +    }
>>>> +
>>>>      has_dirty = vnc_refresh_server_surface(vd);
>>>> +    vnc_unlock_display(vd);
>>>>
>>>>
>>>
>>> This could delay the update by quite a bit, no?
>>>
>>
>> Yep, but it's far better than waiting the lock because it doesn't slow
>> down the main thread.
>> I played big buck bunny trailler (33sec) in mplayer and tight encoding:
>> - ~40 sec with the non-threaded server
>> - ~37 sec with a lock
>> - ~33 sec with a try_lock
>>
>
> Definitely, blocking the main thread is a no-no.
>
>>> A more elaborate approach would be to enqueue the refresh job into the
>>> queue.  May need the iothread enabled so we have qemu_mutex.
>>>
>>
>> Maybe, but I'd like to wait the generic async work subsystem before
>> adding different kind of jobs to the queue. And it's already a big
>> improvment over the current code :).
>>
>
> Hm, ok.
>
>>> btw, I could not find other uses of vd->mutex, shouldn't it protect
>>> against
>>> the work thread?
>>>
>>
>> Check vnc-jobs.c, there is a qemu_mutex_lock(vs->vd->mutex);
>>
>>
>
> Shouldn't it use vnc_lock_display()?  That's why I missed it.

I didn't use vnc_lock_display because I didn't want to export it first.
Maybe I should also use vnc_lock_output() in vnc-jobs.c ...



-- 
Corentin Chary
http://xf.iksaif.net