Re: [Qemu-devel] kvm crashes with spice while loading qxl
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
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
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
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