From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=45031 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OK5kn-0000tm-0S for qemu-devel@nongnu.org; Thu, 03 Jun 2010 04:26:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OK5kk-0005Xx-5Z for qemu-devel@nongnu.org; Thu, 03 Jun 2010 04:26:16 -0400 Received: from mail-bw0-f45.google.com ([209.85.214.45]:48438) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OK5kj-0005XY-JG for qemu-devel@nongnu.org; Thu, 03 Jun 2010 04:26:14 -0400 Received: by bwz4 with SMTP id 4so404882bwz.4 for ; Thu, 03 Jun 2010 01:26:12 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4C075FE0.80704@redhat.com> References: <1275118686-15649-1-git-send-email-corentincj@iksaif.net> <1275118686-15649-4-git-send-email-corentincj@iksaif.net> <4C075FE0.80704@redhat.com> Date: Thu, 3 Jun 2010 10:26:12 +0200 Message-ID: Subject: Re: [Qemu-devel] Re: [PATCH 3/3] vnc: threaded VNC server From: Corentin Chary Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Anthony Liguori , Adam Litke , Gautham R Shenoy , qemu-devel@nongnu.org, Alexander Graf On Thu, Jun 3, 2010 at 9:55 AM, Paolo Bonzini 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 >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 screen corruption if the= framebuffer is updated >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0while the worker threaded= is doing something. >> - VncState::output lock: used to make sure the output buffer is not >> corrupted >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if two threads try to wr= ite 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 an= d >> 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) >> +{ >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0if (QLIST_EMPTY(&job->rectangles)) { >> + =A0 =A0 =A0 =A0qemu_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. >> + =A0 =A0} else { >> + =A0 =A0 =A0 =A0QTAILQ_INSERT_TAIL(&queue->jobs, job, next); >> + =A0 =A0 =A0 =A0qemu_cond_broadcast(&queue->cond); >> + =A0 =A0} >> + =A0 =A0vnc_unlock_queue(queue); >> +} > > ... > >> +static int vnc_worker_thread_loop(VncJobQueue *queue) >> +{ >> + =A0 =A0VncJob *job; >> + =A0 =A0VncRectEntry *entry, *tmp; >> + =A0 =A0VncState vs; >> + =A0 =A0int n_rectangles; >> + =A0 =A0int saved_offset; >> + >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0if (QTAILQ_EMPTY(&queue->jobs)) { >> + =A0 =A0 =A0 =A0qemu_cond_wait(&queue->cond,&queue->mutex); >> + =A0 =A0} >> + >> + =A0 =A0/* If the queue is empty, it's an exit order */ >> + =A0 =A0if (QTAILQ_EMPTY(&queue->jobs)) { >> + =A0 =A0 =A0 =A0vnc_unlock_queue(queue); >> + =A0 =A0 =A0 =A0return -1; >> + =A0 =A0} > > This is not safe. =A0It might work with a single consumer, but something = like > this is better: > > =A0 vnc_lock_queue(queue); > =A0 while (!queue->exit && QTAILQ_EMPTY(&queue->jobs)) { > =A0 =A0 =A0 =A0qemu_cond_wait(&queue->cond,&queue->mutex); > =A0 } > =A0 if (queue->exit) { > =A0 =A0 =A0 vnc_unlock_queue(queue); > =A0 =A0 =A0 return -1; > =A0 } Right, > (It occurred to me now that maybe you can reuse ->aborting. =A0Not sure > though). > >> + =A0 =A0qemu_mutex_unlock(&job->vs->output_mutex); >> + >> + =A0 =A0if (job->vs->csock !=3D -1 && job->vs->abording !=3D true) { >> + =A0 =A0 =A0 =A0vnc_flush(job->vs); >> + =A0 =A0} >> + > > You're accessing the abort flag outside the mutex here. =A0Also, you are = not > using vnc_{,un}lock_output. I assumed that bool (int) where atomic .. but you're right I should lock th= at. >> + =A0 =A0job =3D QTAILQ_FIRST(&queue->jobs); >> + =A0 =A0vnc_unlock_queue(queue); > > ... > >> +static void vnc_abord_display_jobs(VncDisplay *vd) >> +{ >> + =A0 =A0VncState *vs; >> + >> + =A0 =A0QTAILQ_FOREACH(vs, &vd->clients, next) { >> + =A0 =A0 =A0 =A0vnc_lock_output(vs); >> + =A0 =A0 =A0 =A0vs->abording =3D true; >> + =A0 =A0 =A0 =A0vnc_unlock_output(vs); >> + =A0 =A0} >> + =A0 =A0QTAILQ_FOREACH(vs, &vd->clients, next) { >> + =A0 =A0 =A0 =A0vnc_jobs_join(vs); >> + =A0 =A0} >> + =A0 =A0QTAILQ_FOREACH(vs, &vd->clients, next) { >> + =A0 =A0 =A0 =A0vnc_lock_output(vs); >> + =A0 =A0 =A0 =A0vs->abording =3D false; >> + =A0 =A0 =A0 =A0vnc_unlock_output(vs); >> + =A0 =A0} >> +} > > It's "abort" not "abord". :-) Ooops ... > ... > >> =A0static void vnc_disconnect_finish(VncState *vs) >> =A0{ >> + =A0 =A0vnc_jobs_join(vs); /* Wait encoding jobs */ >> + =A0 =A0vnc_lock(vs); > > Possibly racy? =A0Maybe 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. =A0(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). =A0Probably it'= s > better to use a single lock on vd->clients instead of one lock per VncSta= te. 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) >> +{ >> + =A0 =A0VncState *vs =3D opaque; >> + >> + =A0 =A0vnc_lock_output(vs); >> + =A0 =A0if (vs->output.offset) { >> + =A0 =A0 =A0 =A0vnc_client_write_locked(opaque); >> + =A0 =A0} else { >> + =A0 =A0 =A0 =A0qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, = NULL, vs); >> + =A0 =A0} > > Why the if? =A0The "else" branch is already done by vnc_client_write_plai= n. 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 =3D=3D 0 and disconnecting. > This may be a good time to port qemu-threads to Windows too. =A0IO thread= has > no hope to work under Windows at least without major hacks (because Windo= ws > has no asynchronous interrupts; the only way I can imagine to emulate the= m > is a breakpoint) but threaded VNC should work. Right, but I won't do that since I don't have Windows :). --=20 Corentin Chary http://xf.iksaif.net