On Wed, Apr 06, 2011 at 03:19:55PM +0800, Hu Tao wrote:
--- src/qemu/qemu_domain.c | 30 ++----------- src/qemu/qemu_migration.c | 2 - src/qemu/qemu_monitor.c | 109 ++++++++++++++++++++++++-------------------- src/qemu/qemu_monitor.h | 4 +- src/qemu/qemu_process.c | 32 +++++++------- 5 files changed, 81 insertions(+), 96 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3a3c953..d11dc5f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -560,9 +560,8 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData;
- qemuMonitorLock(priv->mon); - qemuMonitorRef(priv->mon); virDomainObjUnlock(obj); + qemuMonitorLock(priv->mon); }
@@ -573,18 +572,9 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) void qemuDomainObjExitMonitor(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; - int refs; - - refs = qemuMonitorUnref(priv->mon); - - if (refs > 0) - qemuMonitorUnlock(priv->mon);
+ qemuMonitorUnlock(priv->mon); virDomainObjLock(obj); - - if (refs == 0) { - priv->mon = NULL; - } }
@@ -601,10 +591,8 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, { qemuDomainObjPrivatePtr priv = obj->privateData;
- qemuMonitorLock(priv->mon); - qemuMonitorRef(priv->mon); virDomainObjUnlock(obj); - qemuDriverUnlock(driver); + qemuMonitorLock(priv->mon); }
@@ -617,19 +605,9 @@ void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; - int refs; - - refs = qemuMonitorUnref(priv->mon); - - if (refs > 0) - qemuMonitorUnlock(priv->mon);
- qemuDriverLock(driver); + qemuMonitorUnlock(priv->mon); virDomainObjLock(obj); - - if (refs == 0) { - priv->mon = NULL; - } }
This means that the 'driver' lock is now whenever any QEMU monitor command is runing, which blocks the entire driver.
void qemuDomainObjEnterRemoteWithDriver(struct qemud_driver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 462e6be..6af2e24 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -224,11 +224,9 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm) }
virDomainObjUnlock(vm); - qemuDriverUnlock(driver);
nanosleep(&ts, NULL);
- qemuDriverLock(driver); virDomainObjLock(vm); }
Holding the 'driver' lock while sleeping blocks the entire QEMU driver.
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 244b22a..4b9087f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -107,7 +107,6 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
VIR_DEBUG("Received EOF on %p '%s'", vm, vm->def->name);
- qemuDriverLock(driver); virDomainObjLock(vm);
if (!virDomainObjIsActive(vm)) { @@ -133,15 +132,17 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuProcessStop(driver, vm, 0); qemuAuditDomainStop(vm, hasError ? "failed" : "shutdown");
- if (!vm->persistent) + if (!vm->persistent) { + qemuDriverLock(driver); virDomainRemoveInactive(&driver->domains, vm); - else - virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + } + + virDomainObjUnlock(vm);
if (event) { qemuDomainEventQueue(driver, event); } - qemuDriverUnlock(driver); }
This violates the lock ordering rules. The 'driver' lock *must* be obtained *before* any 'vm' lock is held. Now we have some places in the code which do lock(vm) lock(driver) and other places which do lock(driver) lock(vm) so 2 threads can trivially deadlock waiting for each other Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|