[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCH] Clean some comments about locking



Since the BQDL has been broken, this patch tries to cleanup some
information about it in various comments.
---
To be applied after Dan's patch [1] which removes unnecessary
qemuDriverLocks.

[1] https://www.redhat.com/archives/libvir-list/2013-February/msg00641.html

 src/qemu/THREADS.txt      |  9 ++-------
 src/qemu/qemu_conf.c      |  8 +++++---
 src/qemu/qemu_conf.h      |  5 +----
 src/qemu/qemu_domain.c    | 11 +++++------
 src/qemu/qemu_domain.h    |  3 +--
 src/qemu/qemu_driver.c    |  5 +----
 src/qemu/qemu_migration.c |  4 ++--
 src/qemu/qemu_process.c   | 40 ++++++++++++++--------------------------
 8 files changed, 31 insertions(+), 54 deletions(-)

diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
index 785be99..50a0cf9 100644
--- a/src/qemu/THREADS.txt
+++ b/src/qemu/THREADS.txt
@@ -128,7 +128,7 @@ To acquire the normal job condition

 To acquire the asynchronous job condition

-  qemuDomainObjBeginAsyncJob()            (if driver is unlocked)
+  qemuDomainObjBeginAsyncJob()
     - Increments ref count on virDomainObjPtr
     - Waits until no async job is running
     - Waits for job.cond condition 'job.active != 0' using virDomainObjPtr
@@ -155,8 +155,6 @@ To acquire the QEMU monitor lock
     - Releases the qemuMonitorObjPtr lock
     - Acquires the virDomainObjPtr lock

-  NB: caller must take care to drop the driver lock if necessary
-
   These functions must not be used by an asynchronous job.


@@ -166,12 +164,10 @@ To acquire the QEMU monitor lock as part of an asynchronous job
     - Validates that the right async job is still running
     - Acquires the qemuMonitorObjPtr lock
     - Releases the virDomainObjPtr lock
-    - Releases the driver lock
     - Validates that the VM is still active

   qemuDomainObjExitMonitor()
     - Releases the qemuMonitorObjPtr lock
-    - Acquires the driver lock
     - Acquires the virDomainObjPtr lock

   These functions are for use inside an asynchronous job; the caller
@@ -180,8 +176,7 @@ To acquire the QEMU monitor lock as part of an asynchronous job
   used from a sync job (such as when first starting a domain).


-To keep a domain alive while waiting on a remote command, starting
-with the driver lock held
+To keep a domain alive while waiting on a remote command

   qemuDomainObjEnterRemote()
     - Increments ref count on virDomainObjPtr
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 5ac2004..4ff912d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1,7 +1,7 @@
 /*
  * qemu_conf.c: QEMU configuration management
  *
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2013 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -79,11 +79,13 @@ struct _qemuDriverCloseDef {
     qemuDriverCloseCallback cb;
 };

-void qemuDriverLock(virQEMUDriverPtr driver)
+static void
+qemuDriverLock(virQEMUDriverPtr driver)
 {
     virMutexLock(&driver->lock);
 }
-void qemuDriverUnlock(virQEMUDriverPtr driver)
+static void
+qemuDriverUnlock(virQEMUDriverPtr driver)
 {
     virMutexUnlock(&driver->lock);
 }
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 09eacce..14f1427 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -1,7 +1,7 @@
 /*
  * qemu_conf.h: QEMU configuration management
  *
- * Copyright (C) 2006-2007, 2009-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2007, 2009-2013 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -236,9 +236,6 @@ struct _qemuDomainCmdlineDef {
 # define QEMUD_MIGRATION_NUM_PORTS 64


-void qemuDriverLock(virQEMUDriverPtr driver);
-void qemuDriverUnlock(virQEMUDriverPtr driver);
-
 virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);

 int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ec4c8f8..482f64a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1,7 +1,7 @@
 /*
  * qemu_domain.h: QEMU domain private state
  *
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2013 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -116,7 +116,6 @@ qemuDomainAsyncJobPhaseFromString(enum qemuDomainAsyncJob job,
 }


-/* driver must be locked before calling */
 void qemuDomainEventQueue(virQEMUDriverPtr driver,
                           virDomainEventPtr event)
 {
@@ -760,7 +759,7 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, enum qemuDomainJob job)
 #define QEMU_JOB_WAIT_TIME (1000ull * 30)

 /*
- * obj must be locked before calling;
+ * obj must be locked before calling
  */
 static int ATTRIBUTE_NONNULL(1)
 qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
@@ -857,7 +856,7 @@ error:
 }

 /*
- * obj must be locked before calling, driver must NOT be locked
+ * obj must be locked before calling
  *
  * This must be called by anything that will change the VM state
  * in any way, or anything that will use the QEMU monitor.
@@ -883,7 +882,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,


 /*
- * obj must be locked before calling, driver does not matter
+ * obj must be locked before calling
  *
  * To be called after completing the work associated with the
  * earlier qemuDomainBeginJob() call
@@ -1734,7 +1733,7 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
 }

 /*
- * The caller must hold a lock on both driver and vm, and there must
+ * The caller must hold a lock the vm and there must
  * be no remaining references to vm.
  */
 void
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index cfc952d..905b099 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1,7 +1,7 @@
 /*
  * qemu_domain.h: QEMU domain private state
  *
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2013 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -175,7 +175,6 @@ int qemuDomainAsyncJobPhaseFromString(enum qemuDomainAsyncJob job,

 void qemuDomainEventFlush(int timer, void *opaque);

-/* driver must be locked before calling */
 void qemuDomainEventQueue(virQEMUDriverPtr driver,
                           virDomainEventPtr event);

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4d756f3..2ad21dc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2746,8 +2746,7 @@ cleanup:
     return ret;
 }

-/* This internal function expects the driver lock to already be held on
- * entry and the vm must be active + locked. Vm will be unlocked and
+/* The vm must be active + locked. Vm will be unlocked and
  * potentially free'd after this returns (eg transient VMs are freed
  * shutdown). So 'vm' must not be referenced by the caller after
  * this returns (whether returning success or failure).
@@ -10349,8 +10348,6 @@ cleanup:
 }


-
-/* this function expects the driver lock to be held by the caller */
 static int
 qemuDomainSnapshotFSFreeze(virDomainObjPtr vm) {
     qemuDomainObjPrivatePtr priv = vm->privateData;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 60cd47a..36e55d2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2,7 +2,7 @@
 /*
  * qemu_migration.c: QEMU migration handling
  *
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -3620,7 +3620,7 @@ cleanup:
 }


-/* Helper function called while driver lock is held and vm is active.  */
+/* Helper function called while vm is active.  */
 int
 qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
                     int fd, off_t offset, const char *path,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 306f686..e5eadc0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1,7 +1,7 @@
 /*
  * qemu_process.h: QEMU process management
  *
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -599,8 +599,7 @@ endjob:
 cleanup:
     if (vm) {
         if (ret == -1) {
-            ignore_value(qemuProcessKill(vm,
-                                         VIR_QEMU_PROCESS_KILL_FORCE));
+            ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_FORCE));
         }
         if (virObjectUnref(vm))
             virObjectUnlock(vm);
@@ -625,13 +624,11 @@ qemuProcessShutdownOrReboot(virQEMUDriverPtr driver,
                             qemuProcessFakeReboot,
                             vm) < 0) {
             VIR_ERROR(_("Failed to create reboot thread, killing domain"));
-            ignore_value(qemuProcessKill(vm,
-                                         VIR_QEMU_PROCESS_KILL_NOWAIT));
+            ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_NOWAIT));
             virObjectUnref(vm);
         }
     } else {
-        ignore_value(qemuProcessKill(vm,
-                                     VIR_QEMU_PROCESS_KILL_NOWAIT));
+        ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_NOWAIT));
     }
 }

@@ -2792,9 +2789,8 @@ qemuProcessPrepareMonitorChr(virQEMUDriverConfigPtr cfg,


 /*
- * Precondition: Both driver and vm must be locked,
- * and a job must be active. This method will call
- * {Enter,Exit}Monitor
+ * Precondition: vm must be locked, and a job must be active.
+ * This method will call {Enter,Exit}Monitor
  */
 int
 qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm,
@@ -3374,23 +3370,15 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
     memcpy(data, src, sizeof(*data));
     data->payload = obj;

-    /* This iterator is called with driver being locked.
+    /*
      * We create a separate thread to run qemuProcessReconnect in it.
      * However, qemuProcessReconnect needs to:
-     * 1. lock driver
-     * 2. just before monitor reconnect do lightweight MonitorEnter
+     * 1. just before monitor reconnect do lightweight MonitorEnter
      *    (increase VM refcount, unlock VM & driver)
-     * 3. reconnect to monitor
-     * 4. do lightweight MonitorExit (lock driver & VM)
-     * 5. continue reconnect process
-     * 6. EndJob
-     * 7. unlock driver
-     *
-     * It is necessary to NOT hold driver lock for the entire run
-     * of reconnect, otherwise we will get blocked if there is
-     * unresponsive qemu.
-     * However, iterating over hash table MUST be done on locked
-     * driver.
+     * 2. reconnect to monitor
+     * 3. do lightweight MonitorExit (lock VM)
+     * 4. continue reconnect process
+     * 5. EndJob
      *
      * NB, we can't do normal MonitorEnter & MonitorExit because
      * these two lock the monitor lock, which does not exists in
@@ -4184,8 +4172,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
     }

     /*
-     * We may unlock the driver and vm in qemuProcessKill(), and another thread
-     * can lock driver and vm, and then call qemuProcessStop(). So we should
+     * We may unlock the vm in qemuProcessKill(), and another thread
+     * can lock the vm, and then call qemuProcessStop(). So we should
      * set vm->def->id to -1 here to avoid qemuProcessStop() to be called twice.
      */
     vm->def->id = -1;
--
1.8.1.2


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]