[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server
|
From: |
Alexander Graf |
|
Subject: |
[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server |
|
Date: |
Fri, 4 Jun 2010 15:44:33 +0200 |
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 <address@hidden>
> ---
> 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 <address@hidden>
> + * Copyright (C) 2006 Fabrice Bellard
> + * Copyright (C) 2009 Red Hat, Inc
> + * Copyright (C) 2010 Corentin Chary <address@hidden>
> + *
> + * 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 <address@hidden>
> + * Copyright (C) 2006 Fabrice Bellard
> + * Copyright (C) 2009 Red Hat, Inc
> + * Copyright (C) 2010 Corentin Chary <address@hidden>
> + *
> + * 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
>
Re: [Qemu-devel] [PATCH v2 2/2] vnc: threaded VNC server, Avi Kivity, 2010/06/06