| Message ID | 1299667284-15157-1-git-send-email-corentin.chary@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 09.03.2011 um 11:41 schrieb Corentin Chary: > 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. qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix for this? thanks, peter > > Thanks to Jan Kiszka for helping me solve this issue. > > Cc: Jan Kiszka <jan.kiszka@web.de> > Signed-off-by: Corentin Chary <corentin.chary@gmail.com> > --- > 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(); > 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); > -- > 1.7.3.4 >
>> 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. > > qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix > for this? If IO Thread is not available, I'm afraid that --disable-vnc-thread is the only fix. Or, you can try to define some global mutex acting like iothread locks, but that doesn't sounds like an easy fix.
On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary <corentin.chary@gmail.com> 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. >> >> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix >> for this? > > If IO Thread is not available, I'm afraid that --disable-vnc-thread is > the only fix. > Or, you can try to define some global mutex acting like iothread > locks, but that doesn't sounds like an easy fix. Jan or Marcelo can help here but qemu-kvm has an iothread equivalent built in by default. It should be possible to use that. Stefan
On 2011-03-09 12:05, Stefan Hajnoczi wrote: > On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary > <corentin.chary@gmail.com> 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. >>> >>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix >>> for this? >> >> If IO Thread is not available, I'm afraid that --disable-vnc-thread is >> the only fix. >> Or, you can try to define some global mutex acting like iothread >> locks, but that doesn't sounds like an easy fix. > > Jan or Marcelo can help here but qemu-kvm has an iothread equivalent > built in by default. It should be possible to use that. qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even without --enable-io-thread. So that tree could temporarily disable the new configure check until we got rid of the special qemu-kvm bits. Corentin's patch is against upstream, that adjustment need to be made once the commit is merged into qemu-kvm. Jan
Am 09.03.2011 um 12:25 schrieb Jan Kiszka: > On 2011-03-09 12:05, Stefan Hajnoczi wrote: >> On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary >> <corentin.chary@gmail.com> 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. >>>> >>>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix >>>> for this? >>> >>> If IO Thread is not available, I'm afraid that --disable-vnc-thread is >>> the only fix. >>> Or, you can try to define some global mutex acting like iothread >>> locks, but that doesn't sounds like an easy fix. >> >> Jan or Marcelo can help here but qemu-kvm has an iothread equivalent >> built in by default. It should be possible to use that. > > qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even > without --enable-io-thread. So that tree could temporarily disable the > new configure check until we got rid of the special qemu-kvm bits. > Corentin's patch is against upstream, that adjustment need to be made > once the commit is merged into qemu-kvm. do i understand you right, that i should be able to use vnc-thread together with qemu-kvm just now if I add Corentin's patch without the io-thread dependency? if yes, i will do and try if I can force a crash again. br, peter > > Jan >
On 2011-03-09 12:32, Peter Lieven wrote: > > Am 09.03.2011 um 12:25 schrieb Jan Kiszka: > >> On 2011-03-09 12:05, Stefan Hajnoczi wrote: >>> On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary >>> <corentin.chary@gmail.com> 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. >>>>> >>>>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix >>>>> for this? >>>> >>>> If IO Thread is not available, I'm afraid that --disable-vnc-thread is >>>> the only fix. >>>> Or, you can try to define some global mutex acting like iothread >>>> locks, but that doesn't sounds like an easy fix. >>> >>> Jan or Marcelo can help here but qemu-kvm has an iothread equivalent >>> built in by default. It should be possible to use that. >> >> qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even >> without --enable-io-thread. So that tree could temporarily disable the >> new configure check until we got rid of the special qemu-kvm bits. >> Corentin's patch is against upstream, that adjustment need to be made >> once the commit is merged into qemu-kvm. > > do i understand you right, that i should be able to use vnc-thread together with qemu-kvm > just now if I add Corentin's patch without the io-thread dependency? Yep. > > if yes, i will do and try if I can force a crash again. > TIA, Jan
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 <jan.kiszka@web.de> > Signed-off-by: Corentin Chary <corentin.chary@gmail.com> > --- > 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
Am 09.03.2011 um 12:42 schrieb Jan Kiszka: > 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 <jan.kiszka@web.de> >> Signed-off-by: Corentin Chary <corentin.chary@gmail.com> >> --- >> 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. so after all i should stay with disabled vnc-thread in qemu-kvm until there is a solution? br, peter > >> 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
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(); 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 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 <jan.kiszka@web.de> Signed-off-by: Corentin Chary <corentin.chary@gmail.com> --- configure | 9 +++++++++ ui/vnc-jobs-async.c | 4 ++++ 2 files changed, 13 insertions(+), 0 deletions(-)