[Xen-changelog] [xen-unstable] libxl: Fix an ao completion bug; document locking policy

[Xen patchbot-unstable <patchbot@xxxxxxx>]

# HG changeset patch
# User Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
# Date 1336759144 -3600
# Node ID 2a8686b77897517b0aaf24b28ebf7a7f782c1317
# Parent  a87bde27be269a657d348c0d215a6d9e3d32d8a4
libxl: Fix an ao completion bug; document locking policy

Document the concurrent access policies for libxl__ao and libxl__egc,
and their corresponding gcs.

Fix a violation of the policy:

If an ao was submitted and a callback requested, and while the
initiating function was still running on the original thread, the ao
is completed on another thread, the completing thread would improperly
concurrently access the ao with the initiating thread.

Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
Committed-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
---


diff -r a87bde27be26 -r 2a8686b77897 tools/libxl/libxl_event.c
--- a/tools/libxl/libxl_event.c Fri May 11 18:59:04 2012 +0100
+++ b/tools/libxl/libxl_event.c Fri May 11 18:59:04 2012 +0100
@@ -901,6 +901,11 @@ void libxl__event_disaster(libxl__egc *e
 
 static void egc_run_callbacks(libxl__egc *egc)
 {
+    /*
+     * The callbacks must happen with the ctx unlocked.
+     * See the comment near #define EGC_GC in libxl_internal.h and
+     * those in the definitions of libxl__egc and libxl__ao.
+     */
     EGC_GC;
     libxl_event *ev, *ev_tmp;
 
@@ -914,9 +919,11 @@ static void egc_run_callbacks(libxl__egc
                              entry_for_callback, ao_tmp) {
         LIBXL_TAILQ_REMOVE(&egc->aos_for_callback, ao, entry_for_callback);
         ao->how.callback(CTX, ao->rc, ao->how.u.for_callback);
+        CTX_LOCK;
         ao->notified = 1;
         if (!ao->in_initiator)
             libxl__ao__destroy(CTX, ao);
+        CTX_UNLOCK;
     }
 }
 
diff -r a87bde27be26 -r 2a8686b77897 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h      Fri May 11 18:59:04 2012 +0100
+++ b/tools/libxl/libxl_internal.h      Fri May 11 18:59:04 2012 +0100
@@ -359,7 +359,8 @@ struct libxl__gc {
 };
 
 struct libxl__egc {
-    /* for event-generating functions only */
+    /* For event-generating functions only.
+     * The egc and its gc may be accessed only on the creating thread. */
     struct libxl__gc gc;
     struct libxl__event_list occurred_for_callback;
     LIBXL_TAILQ_HEAD(, libxl__ao) aos_for_callback;
@@ -369,6 +370,20 @@ struct libxl__egc {
 #define LIBXL__AO_MAGIC_DESTROYED    0xA0DEAD00ul
 
 struct libxl__ao {
+    /*
+     * An ao and its gc may be accessed only with the ctx lock held.
+     *
+     * Special exception: If an ao has been added to
+     * egc->aos_for_callback, the thread owning the egc may remove the
+     * ao from that list and make the callback without holding the
+     * lock.
+     *
+     * Corresponding restriction: An ao may be added only to one
+     * egc->aos_for_callback, once; rc and how must already have been
+     * set and may not be subsequently modified.  (This restriction is
+     * easily and obviously met since the ao is queued for callback
+     * only in libxl__ao_complete.)
+     */
     uint32_t magic;
     unsigned constructing:1, in_initiator:1, complete:1, notified:1;
     int rc;
@@ -1365,6 +1380,12 @@ libxl__device_model_version_running(libx
  * should in any case not find it necessary to call egc-creators from
  * within libxl.
  *
+ * The callbacks must all take place with the ctx unlocked because
+ * the application is entitled to reenter libxl from them.  This
+ * would be bad not because the lock is not recursive (it is) but
+ * because the application might make blocking libxl calls which
+ * would hold the lock unreasonably long.
+ *
  * For the same reason libxl__egc_cleanup (or EGC_FREE) must be called
  * with the ctx *unlocked*.  So the right pattern has the EGC_...
  * macro calls on the outside of the CTX_... ones.

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog