From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36832 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PxHnI-0001Ba-Sj for qemu-devel@nongnu.org; Wed, 09 Mar 2011 06:43:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PxHnH-0008MJ-JS for qemu-devel@nongnu.org; Wed, 09 Mar 2011 06:43:08 -0500 Received: from david.siemens.de ([192.35.17.14]:24211) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PxHnH-0008Lk-6h for qemu-devel@nongnu.org; Wed, 09 Mar 2011 06:43:07 -0500 Message-ID: <4D7767C0.6060609@siemens.com> Date: Wed, 09 Mar 2011 12:42:56 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <4D77539E.8090509@web.de> <1299667284-15157-1-git-send-email-corentin.chary@gmail.com> In-Reply-To: <1299667284-15157-1-git-send-email-corentin.chary@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corentin Chary Cc: Peter Lieven , qemu-devel , kvm@vger.kernel.org, Anthony Liguori On 2011-03-09 11:41, 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 > --- > configure | 9 +++++++++ > ui/vnc-jobs-async.c | 4 ++++ > 2 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/configure b/configure > index 5513d3e..c8c1ac1 100755 > --- a/configure > +++ b/configure > @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \ > roms="optionrom" > fi > > +# VNC Thread depends on IO Thread > +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then > + echo > + echo "ERROR: VNC thread depends on IO thread which isn't enabled." > + echo "Please use --enable-io-thread if you want to enable it." > + echo > + exit 1 > +fi > + > > echo "Install prefix $prefix" > echo "BIOS directory `eval echo $datadir`" > diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c > index f596247..093c0d4 100644 > --- a/ui/vnc-jobs-async.c > +++ b/ui/vnc-jobs-async.c > @@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue) > goto disconnected; > } > > + qemu_mutex_lock_iothread(); Doesn't this comes with a risk of an ABBA deadlock between output_mutex and the global lock? Here you take the global lock while holding output_mutex, but I bet there is also the other way around when vnc services are called from the main thread or a vcpu. > vnc_write(job->vs, vs.output.buffer, vs.output.offset); > + qemu_mutex_unlock_iothread(); > > disconnected: > /* Copy persistent encoding data */ > @@ -269,7 +271,9 @@ disconnected: > vnc_unlock_output(job->vs); > > if (flush) { > + qemu_mutex_lock_iothread(); > vnc_flush(job->vs); > + qemu_mutex_unlock_iothread(); > } > > vnc_lock_queue(queue); The second hunk looks safe. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux