[PATCH 7 of 7 V4] tools/libxl: refactor domain_suspend_callback code to be fully asynchronous

[Shriram Rajagopalan <rshriram@cs.ubc.ca>]

# HG changeset patch
# User Shriram Rajagopalan <rshriram@cs.ubc.ca>
# Date 1384492684 28800
# Node ID c9b550e435d8dce5302e77609849bf007b4f3c2e
# Parent  6a2a1bfffc666d5c7aaaa3532c709f2e38a86d05
tools/libxl: refactor domain_suspend_callback code to be fully asynchronous

libxl__domain_suspend_callback_common uses usleep calls,
while the caller libxl_domain_suspend_callback is asynchronous.
This patch refactors the libxl__domain_suspend__common code to use
AO facilities like libxl event loop timers instead of usleep calls.

Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

diff -r 6a2a1bfffc66 -r c9b550e435d8 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Thu Nov 14 21:17:58 2013 -0800
+++ b/tools/libxl/libxl_dom.c	Thu Nov 14 21:18:04 2013 -0800
@@ -1003,49 +1003,237 @@ int libxl__domain_resume_device_model(li
     return 0;
 }
 
-int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
+static int cancel_xenbus_suspend_request(libxl__gc *gc, uint32_t domid);
+static void wait_for_ack_timeout(libxl__egc *egc, libxl__ev_time *ev,
+                                 const struct timeval *requested_abs);
+static void wait_for_suspend_timeout(libxl__egc *egc, libxl__ev_time *ev,
+                                     const struct timeval *requested_abs);
+static void wait_for_suspend_req_ack(libxl__domain_suspend_state *dss);
+static void wait_for_guest_suspend(libxl__domain_suspend_state *dss);
+static void guest_suspended(libxl__domain_suspend_state *dss);
+
+/* Returns 0 if suspend request was cancelled and the guest did not
+ * respond during the cancellation process (see comments in function for
+ * explanation of race conditions).
+ * Returns 1 if guest had finally acked suspend request, during the
+ * cancellation process.
+ */
+static int cancel_xenbus_suspend_request(libxl__gc *gc, uint32_t domid)
 {
+    char *state = NULL;
+    xs_transaction_t t;
+
+    /*
+     * Guest appears to not be responding. Cancel the suspend
+     * request.
+     *
+     * We re-read the suspend node and clear it within a
+     * transaction in order to handle the case where we race
+     * against the guest catching up and acknowledging the request
+     * at the last minute.
+     */
+    LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
+ retry_transaction:
+    t = xs_transaction_start(CTX->xsh);
+
+    state = libxl__domain_pvcontrol_read(gc, t, domid);
+    if (!state) state = "";
+
+    if (!strcmp(state, "suspend"))
+        libxl__domain_pvcontrol_write(gc, t, domid, "");
+
+    if (!xs_transaction_end(CTX->xsh, t, 0))
+        if (errno == EAGAIN)
+            goto retry_transaction;
+
+    /*
+     * Final check for guest acknowledgement. The guest may have
+     * acknowledged while we were cancelling the request in which
+     * case we lost the race while cancelling and should continue.
+     */
+    if (!strcmp(state, "suspend")) {
+        LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
+        return 0;
+    }
+
+    return 1;
+}
+
+static void wait_for_ack_timeout(libxl__egc *egc, libxl__ev_time *ev,
+                                 const struct timeval *requested_abs)
+{
+    libxl__domain_suspend_state *dss = CONTAINER_OF(ev, *dss, timeout);
     STATE_AO_GC(dss->ao);
-    unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
+
+    libxl__ev_time_deregister(gc, &dss->timeout);
+    dss->watchdog--;
+    wait_for_suspend_req_ack(dss);
+}
+
+static void wait_for_suspend_req_ack(libxl__domain_suspend_state *dss)
+{
+    char *state = NULL;
     int ret;
-    char *state = "suspend";
-    int watchdog;
-    xs_transaction_t t;
+    int wait_time;
+    STATE_AO_GC(dss->ao);
+
+    state = libxl__domain_pvcontrol_read(gc, XBT_NULL, dss->domid);
+
+    if (!state) state = "";
+
+    if (!strcmp(state, "suspend") && dss->watchdog > 0) {
+        /* The first timeout is a short one (10ms), hoping that the
+         * guest would respond immediately. The subsequent timeouts
+         * are longer (40ms).
+         */
+        wait_time = dss->watchdog_starting ? 10 : 40;
+        dss->watchdog_starting = 0;
+        ret = libxl__ev_time_register_rel(gc, &dss->timeout,
+                                          wait_for_ack_timeout, wait_time);
+        if (ret) {
+            LOG(ERROR, "unable to register timeout event to wait for"
+                " guest to suspend ack. Cancelling suspend request");
+            goto cancel_req;
+        }
+        return;
+    }
+
+    if (strcmp(state, "suspend")) {
+    ack:
+        LOG(DEBUG, "guest acknowledged suspend request");
+        dss->guest_responded = 1;
+        dss->watchdog = 60;
+        dss->watchdog_starting = 1;
+        wait_for_guest_suspend(dss);
+        return;
+    }
+
+ cancel_req: //either timer reg. failed or watchdog loop ended
+    if (cancel_xenbus_suspend_request(gc, dss->domid))
+        goto ack;
+
+    libxl__xc_domain_saverestore_async_callback_done(dss->shs.egc, &dss->shs, 0);
+}
+
+static void wait_for_suspend_timeout(libxl__egc *egc, libxl__ev_time *ev,
+                                     const struct timeval *requested_abs)
+{
+    libxl__domain_suspend_state *dss = CONTAINER_OF(ev, *dss, timeout);
+    STATE_AO_GC(dss->ao);
+
+    libxl__ev_time_deregister(gc, &dss->timeout);
+    dss->watchdog--;
+    wait_for_guest_suspend(dss);
+}
+
+static void wait_for_guest_suspend(libxl__domain_suspend_state *dss)
+{
+    int ret;
+    int wait_time;
+    xc_domaininfo_t info;
+    STATE_AO_GC(dss->ao);
+
+    ret = xc_domain_getinfolist(CTX->xch, dss->domid, 1, &info);
+    if (ret == 1 && info.domain == dss->domid &&
+        (info.flags & XEN_DOMINF_shutdown)) {
+        int shutdown_reason;
+
+        shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
+            & XEN_DOMINF_shutdownmask;
+        if (shutdown_reason == SHUTDOWN_suspend) {
+            LOG(DEBUG, "guest has suspended");
+            guest_suspended(dss);
+            return;
+        }
+    }
+
+    if (dss->watchdog > 0) {
+        /* The first timeout is a short one (10ms), hoping that the
+         * guest would respond immediately. The subsequent timeouts
+         * are longer (40ms).
+         */
+        wait_time = dss->watchdog_starting ? 10 : 40;
+        dss->watchdog_starting = 0;
+        ret = libxl__ev_time_register_rel(gc, &dss->timeout,
+                                          wait_for_suspend_timeout, wait_time);
+        if (ret) {
+            LOG(ERROR, "unable to register timeout event to wait for"
+                " guest to suspend.");
+            goto err;
+        }
+        return;
+    }
+
+    LOG(ERROR, "guest did not suspend");
+ err:
+    libxl__xc_domain_saverestore_async_callback_done(dss->shs.egc,
+                                                     &dss->shs, 0);
+}
+
+static int libxl__remus_domain_suspend_callback(libxl__domain_suspend_state *dss);
+static void guest_suspended(libxl__domain_suspend_state *dss)
+{
+    int ret, ok = 1;
+    STATE_AO_GC(dss->ao);
+    if (dss->hvm) {
+        ret = libxl__domain_suspend_device_model(gc, dss);
+        if (ret) {
+            LOG(ERROR, "libxl__domain_suspend_device_model failed "
+                "ret=%d", ret);
+            ok = 0;
+            goto end;
+        }
+    }
+
+    if (dss->remus_ctx)
+        ok = libxl__remus_domain_suspend_callback(dss);
+
+ end:
+    libxl__xc_domain_saverestore_async_callback_done(dss->shs.egc,
+                                                     &dss->shs, ok);
+}
+
+static void libxl__domain_suspend_callback(void *data)
+{
+    libxl__save_helper_state *shs = data;
+    libxl__egc *egc = shs->egc;
+    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
+    int ret;
+    STATE_AO_GC(dss->ao);
 
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
 
-    if (dss->hvm) {
-        xc_get_hvm_param(CTX->xch, domid, HVM_PARAM_CALLBACK_IRQ, &hvm_pvdrv);
-        xc_get_hvm_param(CTX->xch, domid, HVM_PARAM_ACPI_S_STATE, &hvm_s_state);
-    }
-
-    if ((hvm_s_state == 0) && (dss->suspend_eventchn >= 0)) {
+    if ((dss->hvm_s_state == 0) && (dss->suspend_eventchn >= 0)) {
         LOG(DEBUG, "issuing %s suspend request via event channel",
             dss->hvm ? "PVHVM" : "PV");
         ret = xc_evtchn_notify(dss->xce, dss->suspend_eventchn);
         if (ret < 0) {
             LOG(ERROR, "xc_evtchn_notify failed ret=%d", ret);
-            return 0;
+            goto err;
         }
         ret = xc_await_suspend(CTX->xch, dss->xce, dss->suspend_eventchn);
         if (ret < 0) {
             LOG(ERROR, "xc_await_suspend failed ret=%d", ret);
-            return 0;
+            goto err;
         }
         dss->guest_responded = 1;
-        goto guest_suspended;
+        guest_suspended(dss);
+        return;
     }
 
-    if (dss->hvm && (!hvm_pvdrv || hvm_s_state)) {
+    dss->watchdog = 60;
+    dss->watchdog_starting = 1;
+    if (dss->hvm && (!dss->hvm_pvdrv || dss->hvm_s_state)) {
         LOG(DEBUG, "Calling xc_domain_shutdown on HVM domain");
         ret = xc_domain_shutdown(CTX->xch, domid, SHUTDOWN_suspend);
         if (ret < 0) {
             LOGE(ERROR, "xc_domain_shutdown failed");
-            return 0;
+            goto err;
         }
         /* The guest does not (need to) respond to this sort of request. */
         dss->guest_responded = 1;
+        wait_for_guest_suspend(dss);
     } else {
         LOG(DEBUG, "issuing %s suspend request via XenBus control node",
             dss->hvm ? "PVHVM" : "PV");
@@ -1053,90 +1241,12 @@ int libxl__domain_suspend_callback_commo
         libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
 
         LOG(DEBUG, "wait for the guest to acknowledge suspend request");
-        watchdog = 60;
-        while (!strcmp(state, "suspend") && watchdog > 0) {
-            usleep(100000);
+        wait_for_suspend_req_ack(dss);
+    }
+    return;
 
-            state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
-            if (!state) state = "";
-
-            watchdog--;
-        }
-
-        /*
-         * Guest appears to not be responding. Cancel the suspend
-         * request.
-         *
-         * We re-read the suspend node and clear it within a
-         * transaction in order to handle the case where we race
-         * against the guest catching up and acknowledging the request
-         * at the last minute.
-         */
-        if (!strcmp(state, "suspend")) {
-            LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
-        retry_transaction:
-            t = xs_transaction_start(CTX->xsh);
-
-            state = libxl__domain_pvcontrol_read(gc, t, domid);
-            if (!state) state = "";
-
-            if (!strcmp(state, "suspend"))
-                libxl__domain_pvcontrol_write(gc, t, domid, "");
-
-            if (!xs_transaction_end(CTX->xsh, t, 0))
-                if (errno == EAGAIN)
-                    goto retry_transaction;
-
-        }
-
-        /*
-         * Final check for guest acknowledgement. The guest may have
-         * acknowledged while we were cancelling the request in which
-         * case we lost the race while cancelling and should continue.
-         */
-        if (!strcmp(state, "suspend")) {
-            LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
-            return 0;
-        }
-
-        LOG(DEBUG, "guest acknowledged suspend request");
-        dss->guest_responded = 1;
-    }
-
-    LOG(DEBUG, "wait for the guest to suspend");
-    watchdog = 60;
-    while (watchdog > 0) {
-        xc_domaininfo_t info;
-
-        usleep(100000);
-        ret = xc_domain_getinfolist(CTX->xch, domid, 1, &info);
-        if (ret == 1 && info.domain == domid &&
-            (info.flags & XEN_DOMINF_shutdown)) {
-            int shutdown_reason;
-
-            shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
-                & XEN_DOMINF_shutdownmask;
-            if (shutdown_reason == SHUTDOWN_suspend) {
-                LOG(DEBUG, "guest has suspended");
-                goto guest_suspended;
-            }
-        }
-
-        watchdog--;
-    }
-
-    LOG(ERROR, "guest did not suspend");
-    return 0;
-
- guest_suspended:
-    if (dss->hvm) {
-        ret = libxl__domain_suspend_device_model(gc, dss);
-        if (ret) {
-            LOG(ERROR, "libxl__domain_suspend_device_model failed ret=%d", ret);
-            return 0;
-        }
-    }
-    return 1;
+ err:
+    libxl__xc_domain_saverestore_async_callback_done(egc, shs, 0);
 }
 
 static inline char *physmap_path(libxl__gc *gc, uint32_t domid,
@@ -1223,16 +1333,6 @@ int libxl__toolstack_save(uint32_t domid
     return 0;
 }
 
-static void libxl__domain_suspend_callback(void *data)
-{
-    libxl__save_helper_state *shs = data;
-    libxl__egc *egc = shs->egc;
-    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
-
-    int ok = libxl__domain_suspend_callback_common(dss);
-    libxl__xc_domain_saverestore_async_callback_done(egc, shs, ok);
-}
-
 /*----- remus setup/teardown code -----*/
 void libxl__remus_setup_initiate(libxl__egc *egc,
                                  libxl__domain_suspend_state *dss)
@@ -1280,18 +1380,14 @@ void libxl__remus_teardown_done(libxl__e
 
 /*----- remus callbacks -----*/
 
-static void libxl__remus_domain_suspend_callback(void *data)
+static int libxl__remus_domain_suspend_callback(libxl__domain_suspend_state *dss)
 {
-    libxl__save_helper_state *shs = data;
-    libxl__egc *egc = shs->egc;
-    libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
     libxl__remus_ctx *remus_ctx = dss->remus_ctx;
     STATE_AO_GC(dss->ao);
 
     /* REMUS TODO: Issue disk checkpoint reqs. */
-    int ok = libxl__domain_suspend_callback_common(dss);
-
-    if (!remus_ctx->netbuf_ctx || !ok) goto out;
+    if (!remus_ctx->netbuf_ctx)
+        return 1;
 
     /* The domain was suspended successfully. Start a new network
      * buffer for the next epoch. If this operation fails, then act
@@ -1300,9 +1396,9 @@ static void libxl__remus_domain_suspend_
      */
     if (libxl__remus_netbuf_start_new_epoch(gc, dss->domid,
                                             remus_ctx))
-        ok = 0;
- out:
-    libxl__xc_domain_saverestore_async_callback_done(egc, shs, ok);
+        return 0;
+
+    return 1;
 }
 
 static int libxl__remus_domain_resume_callback(void *data)
@@ -1411,6 +1507,7 @@ void libxl__domain_suspend(libxl__egc *e
         &dss->shs.callbacks.save.a;
 
     logdirty_init(&dss->logdirty);
+    libxl__ev_time_init(&dss->timeout);
 
     switch (type) {
     case LIBXL_DOMAIN_TYPE_HVM: {
@@ -1423,6 +1520,10 @@ void libxl__domain_suspend(libxl__egc *e
 
         vm_generationid_addr = (addr) ? strtoul(addr, NULL, 0) : 0;
         dss->hvm = 1;
+        xc_get_hvm_param(CTX->xch, domid, HVM_PARAM_CALLBACK_IRQ,
+                         &dss->hvm_pvdrv);
+        xc_get_hvm_param(CTX->xch, domid, HVM_PARAM_ACPI_S_STATE,
+                         &dss->hvm_s_state);
         break;
     }
     case LIBXL_DOMAIN_TYPE_PV:
@@ -1439,6 +1540,7 @@ void libxl__domain_suspend(libxl__egc *e
 
     dss->suspend_eventchn = -1;
     dss->guest_responded = 0;
+    dss->watchdog = 0;
     dss->dm_savefile = libxl__device_model_savefile(gc, domid);
 
     if (dss->remus_ctx && dss->remus_ctx->compression)
@@ -1461,12 +1563,12 @@ void libxl__domain_suspend(libxl__egc *e
     }
 
     memset(callbacks, 0, sizeof(*callbacks));
+
+    callbacks->suspend = libxl__domain_suspend_callback;
     if (dss->remus_ctx != NULL) {
-        callbacks->suspend = libxl__remus_domain_suspend_callback;
         callbacks->postcopy = libxl__remus_domain_resume_callback;
         callbacks->checkpoint = libxl__remus_domain_checkpoint_callback;
-    } else
-        callbacks->suspend = libxl__domain_suspend_callback;
+    }
 
     callbacks->switch_qemu_logdirty = libxl__domain_suspend_common_switch_qemu_logdirty;
     dss->shs.callbacks.save.toolstack_save = libxl__toolstack_save;
diff -r 6a2a1bfffc66 -r c9b550e435d8 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Thu Nov 14 21:17:58 2013 -0800
+++ b/tools/libxl/libxl_internal.h	Thu Nov 14 21:18:04 2013 -0800
@@ -2351,6 +2351,11 @@ struct libxl__domain_suspend_state {
     int hvm;
     int xcflags;
     int guest_responded;
+    int watchdog;
+    int watchdog_starting;
+    unsigned long hvm_s_state;
+    unsigned long hvm_pvdrv;
+    libxl__ev_time timeout;
     const char *dm_savefile;
     libxl__save_helper_state shs;
     libxl__logdirty_switch logdirty;
@@ -2640,7 +2645,6 @@ _hidden void libxl__xc_domain_save_done(
 void libxl__xc_domain_saverestore_async_callback_done(libxl__egc *egc,
                            libxl__save_helper_state *shs, int return_value);
 
-_hidden int libxl__domain_suspend_callback_common(libxl__domain_suspend_state*);
 _hidden void libxl__domain_suspend_common_switch_qemu_logdirty
                                (int domid, unsigned int enable, void *data);
 _hidden int libxl__toolstack_save(uint32_t domid, uint8_t **buf,