Comments

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.
There is three levels of locking:
- jobs queue lock: for each operation on the queue (push, pop, isEmpty?)
- VncState global lock: mainly used for framebuffer updates to avoid
screen corruption if the framebuffer is updated
while the worker threaded 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 VncState global lock is hold
to avoid screen corruptions (this block vnc_refresh() for a short time) 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.
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 | 4 +
Makefile.objs | 7 +-
configure | 13 ++
vnc-jobs-sync.c | 70 ++++++++++++
vnc-jobs.c | 328 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
vnc.c | 161 +++++++++++++++++++++++----
vnc.h | 73 ++++++++++++
7 files changed, 633 insertions(+), 23 deletions(-)
create mode 100644 vnc-jobs-sync.c
create mode 100644 vnc-jobs.c

On 05/29/2010 09:38 AM, 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.>> There is three levels of locking:> - jobs queue lock: for each operation on the queue (push, pop, isEmpty?)> - VncState global lock: mainly used for framebuffer updates to avoid> screen corruption if the framebuffer is updated> while the worker threaded 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 VncState global lock is hold> to avoid screen corruptions (this block vnc_refresh() for a short time) 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.
This belong in a comment in the code, not in the commit message (or in
both).
> +void vnc_job_push(VncJob *job)> +{> + vnc_lock_queue(queue);> + if (QLIST_EMPTY(&job->rectangles)) {> + qemu_free(job);
No need to lock if you get into the "then" block.
> + } else {> + QTAILQ_INSERT_TAIL(&queue->jobs, job, next);> + qemu_cond_broadcast(&queue->cond);> + }> + vnc_unlock_queue(queue);> +}
...
> +static int vnc_worker_thread_loop(VncJobQueue *queue)> +{> + VncJob *job;> + VncRectEntry *entry, *tmp;> + VncState vs;> + int n_rectangles;> + int saved_offset;> +> + 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;> + }
This is not safe. It might work with a single consumer, but something
like this is better:
vnc_lock_queue(queue);
while (!queue->exit && QTAILQ_EMPTY(&queue->jobs)) {
qemu_cond_wait(&queue->cond,&queue->mutex);
}
if (queue->exit) {
vnc_unlock_queue(queue);
return -1;
}
(It occurred to me now that maybe you can reuse ->aborting. Not sure
though).
> + qemu_mutex_unlock(&job->vs->output_mutex);> +> + if (job->vs->csock != -1 && job->vs->abording != true) {> + vnc_flush(job->vs);> + }> +
You're accessing the abort flag outside the mutex here. Also, you are
not using vnc_{,un}lock_output.
> + job = QTAILQ_FIRST(&queue->jobs);> + vnc_unlock_queue(queue);
...
> +static void vnc_abord_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); > + } > +}
It's "abort" not "abord". :-)
...
> static void vnc_disconnect_finish(VncState *vs)> {> + vnc_jobs_join(vs); /* Wait encoding jobs */> + vnc_lock(vs);
Possibly racy? Maybe you have to set the aforementioned new flag
queue->exit at the beginning of vnc_jobs_join, and refuse new jobs if it
is set.
Also, if anything waits on the same vs in vnc_refresh while you own it
in vnc_disconnect_finish, as soon as you unlock they'll have a dangling
pointer. (After you unlock the mutex the OS wakes the thread, but then
pthread_mutex_lock has to check again that no one got the lock in the
meanwhile; so QTAILQ_FOREACH_SAFE is not protecting you). Probably it's
better to use a single lock on vd->clients instead of one lock per VncState.
> +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);> + }
Why the if? The "else" branch is already done by vnc_client_write_plain.
This may be a good time to port qemu-threads to Windows too. IO thread
has no hope to work under Windows at least without major hacks (because
Windows has no asynchronous interrupts; the only way I can imagine to
emulate them is a breakpoint) but threaded VNC should work.
Paolo

On Thu, Jun 3, 2010 at 9:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 05/29/2010 09:38 AM, 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.>>>> There is three levels of locking:>> - jobs queue lock: for each operation on the queue (push, pop, isEmpty?)>> - VncState global lock: mainly used for framebuffer updates to avoid>> screen corruption if the framebuffer is updated>> while the worker threaded 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 VncState global lock is hold>> to avoid screen corruptions (this block vnc_refresh() for a short time)>> 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.>> This belong in a comment in the code, not in the commit message (or in> both).
Right
>> +void vnc_job_push(VncJob *job)>> +{>> + vnc_lock_queue(queue);>> + if (QLIST_EMPTY(&job->rectangles)) {>> + qemu_free(job);>> No need to lock if you get into the "then" block.
I locked it because the main thread can try to push a job while a
consumer is removing one, so I can't call QLIST_EMPTY() without
locking the queue.
>> + } else {>> + QTAILQ_INSERT_TAIL(&queue->jobs, job, next);>> + qemu_cond_broadcast(&queue->cond);>> + }>> + vnc_unlock_queue(queue);>> +}>> ...>>> +static int vnc_worker_thread_loop(VncJobQueue *queue)>> +{>> + VncJob *job;>> + VncRectEntry *entry, *tmp;>> + VncState vs;>> + int n_rectangles;>> + int saved_offset;>> +>> + 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;>> + }>> This is not safe. It might work with a single consumer, but something like> this is better:>> vnc_lock_queue(queue);> while (!queue->exit && QTAILQ_EMPTY(&queue->jobs)) {> qemu_cond_wait(&queue->cond,&queue->mutex);> }> if (queue->exit) {> vnc_unlock_queue(queue);> return -1;> }
Right,
> (It occurred to me now that maybe you can reuse ->aborting. Not sure> though).>>> + qemu_mutex_unlock(&job->vs->output_mutex);>> +>> + if (job->vs->csock != -1 && job->vs->abording != true) {>> + vnc_flush(job->vs);>> + }>> +>> You're accessing the abort flag outside the mutex here. Also, you are not> using vnc_{,un}lock_output.
I assumed that bool (int) where atomic .. but you're right I should lock that.
>> + job = QTAILQ_FIRST(&queue->jobs);>> + vnc_unlock_queue(queue);>> ...>>> +static void vnc_abord_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);>> + }>> +}>> It's "abort" not "abord". :-)
Ooops ...
> ...>>> static void vnc_disconnect_finish(VncState *vs)>> {>> + vnc_jobs_join(vs); /* Wait encoding jobs */>> + vnc_lock(vs);>> Possibly racy? Maybe you have to set the aforementioned new flag> queue->exit at the beginning of vnc_jobs_join, and refuse new jobs if it is> set.>> Also, if anything waits on the same vs in vnc_refresh while you own it in> vnc_disconnect_finish, as soon as you unlock they'll have a dangling> pointer. (After you unlock the mutex the OS wakes the thread, but then> pthread_mutex_lock has to check again that no one got the lock in the> meanwhile; so QTAILQ_FOREACH_SAFE is not protecting you). Probably it's> better to use a single lock on vd->clients instead of one lock per VncState.
vnc_disconnect_finish can only be called by the main thread, I don't
see how this could be racy, any hint ?
I am missing something ?
>> +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);>> + }>> Why the if? The "else" branch is already done by vnc_client_write_plain.
This is because the vnc_write fd handler can be set by the thread, and
this can end up calling vnc_client_write_plain
with vs->output.offset == 0 and disconnecting.
> This may be a good time to port qemu-threads to Windows too. IO thread has> no hope to work under Windows at least without major hacks (because Windows> has no asynchronous interrupts; the only way I can imagine to emulate them> is a breakpoint) but threaded VNC should work.
Right, but I won't do that since I don't have Windows :).