From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36924 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pz3wW-000364-FK for qemu-devel@nongnu.org; Mon, 14 Mar 2011 05:20:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pz3wU-00041V-9J for qemu-devel@nongnu.org; Mon, 14 Mar 2011 05:19:59 -0400 Received: from mail-ew0-f45.google.com ([209.85.215.45]:49723) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pz3wU-00041K-0F for qemu-devel@nongnu.org; Mon, 14 Mar 2011 05:19:58 -0400 Received: by ewy24 with SMTP id 24so1670468ewy.4 for ; Mon, 14 Mar 2011 02:19:56 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1299769982-18815-1-git-send-email-corentin.chary@gmail.com> References: <4D7767C0.6060609@siemens.com> <1299769982-18815-1-git-send-email-corentin.chary@gmail.com> Date: Mon, 14 Mar 2011 09:19:56 +0000 Message-ID: From: Corentin Chary Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH v5] vnc: don't mess up with iohandlers in the vnc thread List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: kvm@vger.kernel.org, Peter Lieven , qemu-devel , Corentin Chary , Jan Kiszka , Paolo Bonzini On Thu, Mar 10, 2011 at 3:13 PM, Corentin Chary wrote: > The threaded VNC servers messed up with QEMU fd handlers without > any kind of locking, and that can cause some nasty race conditions. > > Using qemu_mutex_lock_iothread() won't work because vnc_dpy_cpy(), > which will wait for the current job queue to finish, can be called with > the iothread lock held. > > Instead, we now store the data in a temporary buffer, and use a bottom > half to notify the main thread that new data is available. > > vnc_[un]lock_ouput() is still needed to access VncState members like > abort, csock or jobs_buffer. > > Signed-off-by: Corentin Chary > --- > =C2=A0ui/vnc-jobs-async.c | =C2=A0 48 +++++++++++++++++++++++++++++------= ------------- > =C2=A0ui/vnc-jobs.h =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A01 + > =C2=A0ui/vnc.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 12 +++++= +++++++ > =C2=A0ui/vnc.h =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A02 = ++ > =C2=A04 files changed, 44 insertions(+), 19 deletions(-) > > diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c > index f596247..4438776 100644 > --- a/ui/vnc-jobs-async.c > +++ b/ui/vnc-jobs-async.c > @@ -28,6 +28,7 @@ > > =C2=A0#include "vnc.h" > =C2=A0#include "vnc-jobs.h" > +#include "qemu_socket.h" > > =C2=A0/* > =C2=A0* Locking: > @@ -155,6 +156,24 @@ void vnc_jobs_join(VncState *vs) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_cond_wait(&queue->cond, &queue->mutex); > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 vnc_unlock_queue(queue); > + =C2=A0 =C2=A0vnc_jobs_consume_buffer(vs); > +} > + > +void vnc_jobs_consume_buffer(VncState *vs) > +{ > + =C2=A0 =C2=A0bool flush; > + > + =C2=A0 =C2=A0vnc_lock_output(vs); > + =C2=A0 =C2=A0if (vs->jobs_buffer.offset) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0vnc_write(vs, vs->jobs_buffer.buffer, vs->jo= bs_buffer.offset); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0buffer_reset(&vs->jobs_buffer); > + =C2=A0 =C2=A0} > + =C2=A0 =C2=A0flush =3D vs->csock !=3D -1 && vs->abort !=3D true; > + =C2=A0 =C2=A0vnc_unlock_output(vs); > + > + =C2=A0 =C2=A0if (flush) { > + =C2=A0 =C2=A0 =C2=A0vnc_flush(vs); > + =C2=A0 =C2=A0} > =C2=A0} > > =C2=A0/* > @@ -197,7 +216,6 @@ static int vnc_worker_thread_loop(VncJobQueue *queue) > =C2=A0 =C2=A0 VncState vs; > =C2=A0 =C2=A0 int n_rectangles; > =C2=A0 =C2=A0 int saved_offset; > - =C2=A0 =C2=A0bool flush; > > =C2=A0 =C2=A0 vnc_lock_queue(queue); > =C2=A0 =C2=A0 while (QTAILQ_EMPTY(&queue->jobs) && !queue->exit) { > @@ -213,6 +231,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue) > > =C2=A0 =C2=A0 vnc_lock_output(job->vs); > =C2=A0 =C2=A0 if (job->vs->csock =3D=3D -1 || job->vs->abort =3D=3D true)= { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0vnc_unlock_output(job->vs); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto disconnected; > =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 vnc_unlock_output(job->vs); > @@ -233,10 +252,6 @@ static int vnc_worker_thread_loop(VncJobQueue *queue= ) > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (job->vs->csock =3D=3D -1) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vnc_unlock_display(job->vs->vd)= ; > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* output mutex must be locked= before going to > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * disconnected: > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */ > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vnc_lock_output(job->vs); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto disconnected; > =C2=A0 =C2=A0 =C2=A0 =C2=A0 } > > @@ -254,24 +269,19 @@ static int vnc_worker_thread_loop(VncJobQueue *queu= e) > =C2=A0 =C2=A0 vs.output.buffer[saved_offset] =3D (n_rectangles >> 8) & 0x= FF; > =C2=A0 =C2=A0 vs.output.buffer[saved_offset + 1] =3D n_rectangles & 0xFF; > > - =C2=A0 =C2=A0/* Switch back buffers */ > =C2=A0 =C2=A0 vnc_lock_output(job->vs); > - =C2=A0 =C2=A0if (job->vs->csock =3D=3D -1) { > - =C2=A0 =C2=A0 =C2=A0 =C2=A0goto disconnected; > + =C2=A0 =C2=A0if (job->vs->csock !=3D -1) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0buffer_reserve(&job->vs->jobs_buffer, vs.out= put.offset); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0buffer_append(&job->vs->jobs_buffer, vs.outp= ut.buffer, > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0vs.output.offset); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Copy persistent encoding data */ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0vnc_async_encoding_end(job->vs, &vs); > + > + =C2=A0 =C2=A0 =C2=A0 qemu_bh_schedule(job->vs->bh); > =C2=A0 =C2=A0 } > - > - =C2=A0 =C2=A0vnc_write(job->vs, vs.output.buffer, vs.output.offset); > - > -disconnected: > - =C2=A0 =C2=A0/* Copy persistent encoding data */ > - =C2=A0 =C2=A0vnc_async_encoding_end(job->vs, &vs); > - =C2=A0 =C2=A0flush =3D (job->vs->csock !=3D -1 && job->vs->abort !=3D t= rue); > =C2=A0 =C2=A0 vnc_unlock_output(job->vs); > > - =C2=A0 =C2=A0if (flush) { > - =C2=A0 =C2=A0 =C2=A0 =C2=A0vnc_flush(job->vs); > - =C2=A0 =C2=A0} > - > +disconnected: > =C2=A0 =C2=A0 vnc_lock_queue(queue); > =C2=A0 =C2=A0 QTAILQ_REMOVE(&queue->jobs, job, next); > =C2=A0 =C2=A0 vnc_unlock_queue(queue); > diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h > index b8dab81..4c661f9 100644 > --- a/ui/vnc-jobs.h > +++ b/ui/vnc-jobs.h > @@ -40,6 +40,7 @@ void vnc_jobs_join(VncState *vs); > > =C2=A0#ifdef CONFIG_VNC_THREAD > > +void vnc_jobs_consume_buffer(VncState *vs); > =C2=A0void vnc_start_worker_thread(void); > =C2=A0bool vnc_worker_thread_running(void); > =C2=A0void vnc_stop_worker_thread(void); > diff --git a/ui/vnc.c b/ui/vnc.c > index 610f884..f28873b 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -1011,7 +1011,10 @@ static void vnc_disconnect_finish(VncState *vs) > > =C2=A0#ifdef CONFIG_VNC_THREAD > =C2=A0 =C2=A0 qemu_mutex_destroy(&vs->output_mutex); > + =C2=A0 =C2=A0qemu_bh_delete(vs->bh); > + =C2=A0 =C2=A0buffer_free(&vs->jobs_buffer); > =C2=A0#endif > + > =C2=A0 =C2=A0 for (i =3D 0; i < VNC_STAT_ROWS; ++i) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_free(vs->lossy_rect[i]); > =C2=A0 =C2=A0 } > @@ -1226,6 +1229,14 @@ static long vnc_client_read_plain(VncState *vs) > =C2=A0 =C2=A0 return ret; > =C2=A0} > > +#ifdef CONFIG_VNC_THREAD > +static void vnc_jobs_bh(void *opaque) > +{ > + =C2=A0 =C2=A0VncState *vs =3D opaque; > + > + =C2=A0 =C2=A0vnc_jobs_consume_buffer(vs); > +} > +#endif > > =C2=A0/* > =C2=A0* First function called whenever there is more data to be read from > @@ -2525,6 +2536,7 @@ static void vnc_connect(VncDisplay *vd, int csock) > > =C2=A0#ifdef CONFIG_VNC_THREAD > =C2=A0 =C2=A0 qemu_mutex_init(&vs->output_mutex); > + =C2=A0 =C2=A0vs->bh =3D qemu_bh_new(vnc_jobs_bh, vs); > =C2=A0#endif > > =C2=A0 =C2=A0 QTAILQ_INSERT_HEAD(&vd->clients, vs, next); > diff --git a/ui/vnc.h b/ui/vnc.h > index 8a1e7b9..bca5f87 100644 > --- a/ui/vnc.h > +++ b/ui/vnc.h > @@ -283,6 +283,8 @@ struct VncState > =C2=A0 =C2=A0 VncJob job; > =C2=A0#else > =C2=A0 =C2=A0 QemuMutex output_mutex; > + =C2=A0 =C2=A0QEMUBH *bh; > + =C2=A0 =C2=A0Buffer jobs_buffer; > =C2=A0#endif > > =C2=A0 =C2=A0 /* Encoding specific, if you add something here, don't forg= et to > -- > 1.7.3.4 > > Paolo, Anthony, Jan, are you ok with that one ? Peter Lieve, could you test that patch ? Thanks --=20 Corentin Chary http://xf.iksaif.net