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

[Paolo Bonzini <pbonzini@redhat.com>]

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