[libvirt] [PATCH 4/6] qemu: use virObject to manage reference-counting for qemu monitor

[Daniel P. Berrange]

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 :|