[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: |
Corentin Chary |
|
Subject: |
[Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server |
|
Date: |
Sat, 5 Jun 2010 10:03:14 +0200 |
On Fri, Jun 4, 2010 at 3:44 PM, Alexander Graf <address@hidden> 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 <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.
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 <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?
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 <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;
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] [PATCH v2 2/2] vnc: threaded VNC server, Avi Kivity, 2010/06/06