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

[Hu Tao]

On Thu, Apr 07, 2011 at 10:38:34AM +0100, Daniel P. Berrange wrote:
> 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.

qemuDomainObjEnterMonitorWithDriver is now called without holding qemu
driver lock.

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

Now qemuMigrationWaitForCompletion should be called without holding qemu
driver lock.

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

Excepting for introducing virObject for reference-counting, this series
also simplifies the usage of lock: if you want to read/write qemu
driver data, it is enough to first acquire qemu driver lock only; if you
want to read/write virDomainObj data, it is enough to first acquire
virDomainObj lock only; same for others. And we'd better to avoid
acquiring two locks at the same time.

So yes, the code here is problematic, it should be ideally like this:

virDomainObjLock(vm);
if (!vm->persistent) {
  lock_hashtable(doms); /* hashtable's own lock to protect itself */
  virDomainRemoveInactive(doms, vm); 
  unlock_hashtable(doms);
}
virDomainObjUnlock(vm);

But it lacks hashtable lock, how about change the code like this:

virDomainObjLock(vm);
persistent = vm->persistent;
virDomainObjUnlock(vm);

/* chances that others change vm->persistent and we remove
   vm mistakenly :( */

if (!persistent) {
  qemuDriverLock(driver);
  virDomainRemoveInactive(doms, vm); 
  qemuDriverUnlock(driver);
}

Or is there a better way?


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