Re: [Qemu-devel] kvm crashes with spice while loading qxl

2011-04-26 Thread Gerd Hoffmann

  Hi,

[ ... back online now ... ]


/var/tmp/portage/app-emulation/qemu-kvm-0.14.0/work/qemu-kvm-0.14.0/qemu-kvm.c:1724:
kvm_mutex_unlock: Assertion `!cpu_single_env' failed.



That's a spice bug. In fact, there are a lot of
qemu_mutex_lock/unlock_iothread in that subsystem. I bet at least a few
of them can cause even more subtle problems.

Two general issues with dropping the global mutex like this:
  - The caller of mutex_unlock is responsible for maintaining
cpu_single_env across the unlocked phase (that's related to the
abort above).


This is true for qemu-kvm only, right?

qemu-kvm specific patches which add the cpu_single_env tracking (not 
polished yet) are here:


http://cgit.freedesktop.org/spice/qemu/log/?h=spice.kvm.v28


  - Dropping the lock in the middle of a callback is risky. That may
enable re-entrances of code sections that weren't designed for this


Hmm, indeed.


Spice requires a careful review regarding such issues. Or it should
pioneer with introducing its own lock so that we can handle at least
related I/O activities over the VCPUs without holding the global mutex
(but I bet it's not the simplest candidate for such a new scheme).


spice/qxl used to have its own locking scheme.  That didn't work out 
though.  spice server is threaded and calls back into qxl from spice 
thread context, and some of these callbacks need access to qemu data 
structures (display surface) and thus lock protection which covers more 
than just the spice subsystem.


I'll look hard again whenever I can find a way out of this (preferably 
drop the need for the global lock somehow).  For now I'm pretty busy 
with the email backlog though ...


cheers,
  Gerd



Re: [Qemu-devel] kvm crashes with spice while loading qxl

2011-04-26 Thread Jan Kiszka
On 2011-04-26 10:53, Gerd Hoffmann wrote:
   Hi,
 
 [ ... back online now ... ]
 
 /var/tmp/portage/app-emulation/qemu-kvm-0.14.0/work/qemu-kvm-0.14.0/qemu-kvm.c:1724:

 kvm_mutex_unlock: Assertion `!cpu_single_env' failed.
 
 That's a spice bug. In fact, there are a lot of
 qemu_mutex_lock/unlock_iothread in that subsystem. I bet at least a few
 of them can cause even more subtle problems.

 Two general issues with dropping the global mutex like this:
   - The caller of mutex_unlock is responsible for maintaining
 cpu_single_env across the unlocked phase (that's related to the
 abort above).
 
 This is true for qemu-kvm only, right?

Nope, this applies to both implementations.

 
 qemu-kvm specific patches which add the cpu_single_env tracking (not
 polished yet) are here:
 
 http://cgit.freedesktop.org/spice/qemu/log/?h=spice.kvm.v28

Cannot spot that quickly: In which way are they specific to qemu-kvm?

If they are, try to focus on upstream first. The qemu-kvm differences
are virtually deprecated, and I hope we can remove them really soon now
(my patches are all ready).

 
   - Dropping the lock in the middle of a callback is risky. That may
 enable re-entrances of code sections that weren't designed for this
 
 Hmm, indeed.
 
 Spice requires a careful review regarding such issues. Or it should
 pioneer with introducing its own lock so that we can handle at least
 related I/O activities over the VCPUs without holding the global mutex
 (but I bet it's not the simplest candidate for such a new scheme).
 
 spice/qxl used to have its own locking scheme.  That didn't work out
 though.  spice server is threaded and calls back into qxl from spice
 thread context, and some of these callbacks need access to qemu data
 structures (display surface) and thus lock protection which covers more
 than just the spice subsystem.
 
 I'll look hard again whenever I can find a way out of this (preferably
 drop the need for the global lock somehow).  For now I'm pretty busy
 with the email backlog though ...

Yeah, I can imagine...

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux



Re: [Qemu-devel] kvm crashes with spice while loading qxl

2011-04-26 Thread Alon Levy
On Tue, Apr 26, 2011 at 10:53:04AM +0200, Gerd Hoffmann wrote:
   Hi,
 
 [ ... back online now ... ]
 
 /var/tmp/portage/app-emulation/qemu-kvm-0.14.0/work/qemu-kvm-0.14.0/qemu-kvm.c:1724:
 kvm_mutex_unlock: Assertion `!cpu_single_env' failed.
 
 That's a spice bug. In fact, there are a lot of
 qemu_mutex_lock/unlock_iothread in that subsystem. I bet at least a few
 of them can cause even more subtle problems.
 
 Two general issues with dropping the global mutex like this:
   - The caller of mutex_unlock is responsible for maintaining
 cpu_single_env across the unlocked phase (that's related to the
 abort above).
 
 This is true for qemu-kvm only, right?
 
 qemu-kvm specific patches which add the cpu_single_env tracking (not
 polished yet) are here:
 
 http://cgit.freedesktop.org/spice/qemu/log/?h=spice.kvm.v28
 
   - Dropping the lock in the middle of a callback is risky. That may
 enable re-entrances of code sections that weren't designed for this
 
 Hmm, indeed.
 
 Spice requires a careful review regarding such issues. Or it should
 pioneer with introducing its own lock so that we can handle at least
 related I/O activities over the VCPUs without holding the global mutex
 (but I bet it's not the simplest candidate for such a new scheme).
 
 spice/qxl used to have its own locking scheme.  That didn't work out
 though.  spice server is threaded and calls back into qxl from spice
 thread context, and some of these callbacks need access to qemu data
 structures (display surface) and thus lock protection which covers
 more than just the spice subsystem.
 
 I'll look hard again whenever I can find a way out of this
 (preferably drop the need for the global lock somehow).  For now I'm
 pretty busy with the email backlog though ...
 

We (Hans, Uri, and Me) have already sent a fix for this, it seems to have
passed everyone's testing, and it basically does just that - drops the
use of the mutex. It's in 
http://cgit.freedesktop.org/spice/qemu/log/?h=spice.v32.kvm,
see the patches:

 qxl/spice-display: move pipe to ssd
 qxl: implement get_command in vga mode without locks
 qxl/spice: remove qemu_mutex_{un,}lock_iothread around dispatcher
 hw/qxl-render: drop cursor locks, replace with pipe

And specifically the comments too.

Alon

 cheers,
   Gerd
 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html



Re: [Qemu-devel] kvm crashes with spice while loading qxl

2011-04-26 Thread Gerd Hoffmann

On 04/26/11 11:06, Jan Kiszka wrote:

On 2011-04-26 10:53, Gerd Hoffmann wrote:

Two general issues with dropping the global mutex like this:
   - The caller of mutex_unlock is responsible for maintaining
 cpu_single_env across the unlocked phase (that's related to the
 abort above).


This is true for qemu-kvm only, right?


Nope, this applies to both implementations.


Oops.


qemu-kvm specific patches which add the cpu_single_env tracking (not
polished yet) are here:

http://cgit.freedesktop.org/spice/qemu/log/?h=spice.kvm.v28


Cannot spot that quickly: In which way are they specific to qemu-kvm?


cpu_single_env bookeeping.  But if upstream needs that too having 
specific patches is pretty pointless.  I'll go fix it up upstream then.


cheers,
  Gerd