From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=56544 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OK5Gq-00062J-Ht for qemu-devel@nongnu.org; Thu, 03 Jun 2010 03:55:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OK5Gp-0000u5-Cm for qemu-devel@nongnu.org; Thu, 03 Jun 2010 03:55:20 -0400 Received: from mail-ww0-f45.google.com ([74.125.82.45]:41314) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OK5Gp-0000tw-5A for qemu-devel@nongnu.org; Thu, 03 Jun 2010 03:55:19 -0400 Received: by wwb13 with SMTP id 13so2281594wwb.4 for ; Thu, 03 Jun 2010 00:55:18 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <4C075FE0.80704@redhat.com> Date: Thu, 03 Jun 2010 09:55:12 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1275118686-15649-1-git-send-email-corentincj@iksaif.net> <1275118686-15649-4-git-send-email-corentincj@iksaif.net> In-Reply-To: <1275118686-15649-4-git-send-email-corentincj@iksaif.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 3/3] vnc: threaded VNC server List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corentin Chary Cc: Anthony Liguori , Alexander Graf , Adam Litke , qemu-devel@nongnu.org, Gautham R Shenoy 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