qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paolo Bonzini
Subject: [Qemu-devel] Re: [PATCH 3/3] vnc: threaded VNC server
Date: Thu, 03 Jun 2010 09:55:12 +0200
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100430 Fedora/3.0.4-3.fc13 Lightning/1.0b2pre Thunderbird/3.0.4

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]