From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36594 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PxJuy-00031W-Bn for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:59:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PxJuw-00035A-St for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:59:12 -0500 Received: from mail-ey0-f173.google.com ([209.85.215.173]:60234) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PxJuw-000355-KN for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:59:10 -0500 Received: by eyb6 with SMTP id 6so158794eyb.4 for ; Wed, 09 Mar 2011 05:59:09 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <4D7785CE.9020706@redhat.com> References: <4D7767C0.6060609@siemens.com> <1299676892-19246-1-git-send-email-corentin.chary@gmail.com> <4D7785CE.9020706@redhat.com> Date: Wed, 9 Mar 2011 13:59:09 +0000 Message-ID: From: Corentin Chary Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH v2] vnc: threaded server depends on io-thread List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kvm@vger.kernel.org, Jan Kiszka , qemu-devel , Anthony Liguori , Jan Kiszka , Peter Lieven On Wed, Mar 9, 2011 at 1:51 PM, Paolo Bonzini wrote: > On 03/09/2011 02:21 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. >> >> The IO-Thread provides appropriate locking primitives to avoid that. >> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD, >> and add lock and unlock calls around the two faulty calls. >> >> Thanks to Jan Kiszka for helping me solve this issue. >> >> Cc: Jan Kiszka >> Signed-off-by: Corentin Chary >> --- >> The previous patch was total crap, introduced race conditions, >> and probably crashs on client disconnections. >> >> =C2=A0configure =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A09 ++++= +++++ >> =C2=A0ui/vnc-jobs-async.c | =C2=A0 24 +++++++++++++++++++----- >> =C2=A02 files changed, 28 insertions(+), 5 deletions(-) >> >> diff --git a/configure b/configure >> index 5513d3e..c8c1ac1 100755 >> --- a/configure >> +++ b/configure >> @@ -2455,6 +2455,15 @@ if test \( "$cpu" =3D "i386" -o "$cpu" =3D "x86_6= 4" \) >> -a \ >> =C2=A0 =C2=A0roms=3D"optionrom" >> =C2=A0fi >> >> +# VNC Thread depends on IO Thread >> +if test "$vnc_thread" =3D "yes" -a "$io_thread" !=3D "yes"; then >> + =C2=A0echo >> + =C2=A0echo "ERROR: VNC thread depends on IO thread which isn't enabled= ." >> + =C2=A0echo "Please use --enable-io-thread if you want to enable it." >> + =C2=A0echo >> + =C2=A0exit 1 >> +fi >> + >> >> =C2=A0echo "Install prefix =C2=A0 =C2=A0$prefix" >> =C2=A0echo "BIOS directory =C2=A0 =C2=A0`eval echo $datadir`" >> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c >> index f596247..d0c6f61 100644 >> --- a/ui/vnc-jobs-async.c >> +++ b/ui/vnc-jobs-async.c >> @@ -190,6 +190,18 @@ static void vnc_async_encoding_end(VncState *orig, >> VncState *local) >> =C2=A0 =C2=A0 =C2=A0queue->buffer =3D local->output; >> =C2=A0} >> >> +static void vnc_worker_lock_output(VncState *vs) >> +{ >> + =C2=A0 =C2=A0qemu_mutex_lock_iothread(); >> + =C2=A0 =C2=A0vnc_lock_output(vs); >> +} >> + >> +static void vnc_worker_unlock_output(VncState *vs) >> +{ >> + =C2=A0 =C2=A0vnc_unlock_output(vs); >> + =C2=A0 =C2=A0qemu_mutex_unlock_iothread(); >> +} >> + >> =C2=A0static int vnc_worker_thread_loop(VncJobQueue *queue) >> =C2=A0{ >> =C2=A0 =C2=A0 =C2=A0VncJob *job; >> @@ -211,11 +223,11 @@ static int vnc_worker_thread_loop(VncJobQueue >> *queue) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; >> =C2=A0 =C2=A0 =C2=A0} >> >> - =C2=A0 =C2=A0vnc_lock_output(job->vs); >> + =C2=A0 =C2=A0vnc_worker_lock_output(job->vs); >> =C2=A0 =C2=A0 =C2=A0if (job->vs->csock =3D=3D -1 || job->vs->abort =3D= =3D true) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto disconnected; >> =C2=A0 =C2=A0 =C2=A0} >> - =C2=A0 =C2=A0vnc_unlock_output(job->vs); >> + =C2=A0 =C2=A0vnc_worker_unlock_output(job->vs); >> >> =C2=A0 =C2=A0 =C2=A0/* Make a local copy of vs and switch output buffers= */ >> =C2=A0 =C2=A0 =C2=A0vnc_async_encoding_start(job->vs,&vs); >> @@ -236,7 +248,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue= ) >> =C2=A0 =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 =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=A0 =C2=A0vnc_lock_output(job->vs); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vnc_worker_lock_output(job->v= s); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto disconnected; >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> >> @@ -255,7 +267,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue= ) >> =C2=A0 =C2=A0 =C2=A0vs.output.buffer[saved_offset + 1] =3D n_rectangles&= =C2=A00xFF; >> >> =C2=A0 =C2=A0 =C2=A0/* Switch back buffers */ >> - =C2=A0 =C2=A0vnc_lock_output(job->vs); >> + =C2=A0 =C2=A0vnc_worker_lock_output(job->vs); >> =C2=A0 =C2=A0 =C2=A0if (job->vs->csock =3D=3D -1) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto disconnected; >> =C2=A0 =C2=A0 =C2=A0} >> @@ -266,10 +278,12 @@ disconnected: >> =C2=A0 =C2=A0 =C2=A0/* Copy persistent encoding data */ >> =C2=A0 =C2=A0 =C2=A0vnc_async_encoding_end(job->vs,&vs); >> =C2=A0 =C2=A0 =C2=A0flush =3D (job->vs->csock !=3D -1&& =C2=A0job->vs->a= bort !=3D true); >> - =C2=A0 =C2=A0vnc_unlock_output(job->vs); >> + =C2=A0 =C2=A0vnc_worker_unlock_output(job->vs); >> >> =C2=A0 =C2=A0 =C2=A0if (flush) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_mutex_lock_iothread(); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vnc_flush(job->vs); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_mutex_unlock_iothread(); >> =C2=A0 =C2=A0 =C2=A0} >> >> =C2=A0 =C2=A0 =C2=A0vnc_lock_queue(queue); > > Acked-by: Paolo Bonzini for stable. Nacked-by: Corentin Chary > For 0.15, I believe an iohandler-list lock is a better solution. I believe that's the only solution. Looks at that deadlock: (gdb) bt #0 0x00007f70c067ad74 in __lll_lock_wait () from /lib/libpthread.so.0 #1 0x00007f70c0676219 in _L_lock_508 () from /lib/libpthread.so.0 #2 0x00007f70c067603b in pthread_mutex_lock () from /lib/libpthread.so.0 #3 0x00000000004c7559 in qemu_mutex_lock (mutex=3D0x10f4340) at qemu-threa= d.c:50 #4 0x00000000005644ef in main_loop_wait (nonblocking=3D) at /home/iksaif/dev/qemu/vl.c:1374 #5 0x00000000005653a8 in main_loop (argc=3D, argv=3D, envp=3D) at /home/iksaif/dev/qemu/vl.c:1437 #6 main (argc=3D, argv=3D, envp=3D) at /home/iksaif/dev/qemu/vl.c:3148 (gdb) t 2 [Switching to thread 2 (Thread 0x7f709262b710 (LWP 19546))]#0 0x00007f70c067ad74 in __lll_lock_wait () from /lib/libpthread.so.0 (gdb) bt #0 0x00007f70c067ad74 in __lll_lock_wait () from /lib/libpthread.so.0 #1 0x00007f70c0676219 in _L_lock_508 () from /lib/libpthread.so.0 #2 0x00007f70c067603b in pthread_mutex_lock () from /lib/libpthread.so.0 #3 0x00000000004c7559 in qemu_mutex_lock (mutex=3D0x10f4340) at qemu-threa= d.c:50 #4 0x00000000004c65de in vnc_worker_thread_loop (queue=3D0x33561d0) at ui/vnc-jobs-async.c:254 #5 0x00000000004c6730 in vnc_worker_thread (arg=3D0x33561d0) at ui/vnc-jobs-async.c:306 #6 0x00007f70c0673904 in start_thread () from /lib/libpthread.so.0 #7 0x00007f70be5cf1dd in clone () from /lib/libc.so.6 (gdb) t 3 [Switching to thread 3 (Thread 0x7f70b38ff710 (LWP 19545))]#0 0x00007f70c067829c in pthread_cond_wait () from /lib/libpthread.so.0 (gdb) bt #0 0x00007f70c067829c in pthread_cond_wait () from /lib/libpthread.so.0 #1 0x00000000004c7239 in qemu_cond_wait (cond=3D0x33561d4, mutex=3D0x80) at qemu-thread.c:133 #2 0x00000000004c617b in vnc_jobs_join (vs=3D0x35d9c10) at ui/vnc-jobs-async.c:155 #3 0x00000000004afefa in vnc_update_client_sync (ds=3D, src_x=3D, src_y=3D, dst_x=3D, dst_y=3D, w=3D, h=3D1) at ui/vnc.c:819 #4 vnc_dpy_copy (ds=3D, src_x=3D, src_y=3D, dst_x=3D, dst_y=3D, w=3D, h=3D1) at ui/vnc.c:692 #5 0x000000000046b5e1 in dpy_copy (ds=3D0x3329d40, src_x=3D, src_y=3D, dst_x=3D129, dst_y=3D377, w=3D2, h=3D1) at console.h:273 #6 qemu_console_copy (ds=3D0x3329d40, src_x=3D, src_y=3D, dst_x=3D129, dst_y=3D377, w=3D2, h=3D1) at console.c:1579 #7 0x00000000005d6159 in cirrus_do_copy (s=3D0x3316ea8) at /home/iksaif/dev/qemu/hw/cirrus_vga.c:729 #8 cirrus_bitblt_videotovideo_copy (s=3D0x3316ea8) at /home/iksaif/dev/qemu/hw/cirrus_vga.c:747 #9 cirrus_bitblt_videotovideo (s=3D0x3316ea8) at /home/iksaif/dev/qemu/hw/cirrus_vga.c:869 #10 cirrus_bitblt_start (s=3D0x3316ea8) at /home/iksaif/dev/qemu/hw/cirrus_vga.c:1010 #11 0x00000000005d8b67 in cirrus_mmio_writel (opaque=3D0x33561d4, addr=3D128, val=3D4294967042) at /home/iksaif/dev/qemu/hw/cirrus_vga.c:2819 #12 0x00000000004e04d8 in cpu_physical_memory_rw (addr=3D4060086592, buf=3D0x7f70b30fc028 "\002\377\377\377", len=3D4, is_write=3D1) at /home/iksaif/dev/qemu/exec.c:3670 #13 0x000000000042c845 in kvm_cpu_exec (env=3D0x30f8dd0) at /home/iksaif/dev/qemu/kvm-all.c:954 #14 0x000000000040c75e in qemu_kvm_cpu_thread_fn (arg=3D0x30f8dd0) at /home/iksaif/dev/qemu/cpus.c:832 #15 0x00007f70c0673904 in start_thread () from /lib/libpthread.so.0 #16 0x00007f70be5cf1dd in clone () from /lib/libc.so.6 (gdb) t 4 [Switching to thread 4 (Thread 0x7f70b4103710 (LWP 19544))]#0 0x00007f70c067ad74 in __lll_lock_wait () from /lib/libpthread.so.0 (gdb) bt #0 0x00007f70c067ad74 in __lll_lock_wait () from /lib/libpthread.so.0 #1 0x00007f70c0676219 in _L_lock_508 () from /lib/libpthread.so.0 #2 0x00007f70c067603b in pthread_mutex_lock () from /lib/libpthread.so.0 #3 0x00000000004c7559 in qemu_mutex_lock (mutex=3D0x10f4340) at qemu-threa= d.c:50 #4 0x000000000042c703 in kvm_cpu_exec (env=3D0x30e15f0) at /home/iksaif/dev/qemu/kvm-all.c:924 #5 0x000000000040c75e in qemu_kvm_cpu_thread_fn (arg=3D0x30e15f0) at /home/iksaif/dev/qemu/cpus.c:832 #6 0x00007f70c0673904 in start_thread () from /lib/libpthread.so.0 #7 0x00007f70be5cf1dd in clone () from /lib/libc.so.6 I currently don't see any solution for that one using iothread lock: - vnc_dpy_copy can be called with iothread locked - vnc_dpy_copy needs to wait for vnc-thread to finish is work before being able to do anything - vnc-thread need to lock iothread to finish its work --=20 Corentin Chary http://xf.iksaif.net