Xen project Mailing List

On Thu, Nov 14, 2013 at 11:47 PM, Shriram Rajagopalan <rshriram@xxxxxxxxx> wrote:

# HG changeset patch
# User Shriram Rajagopalan <rshriram@xxxxxxxxx>
# 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@xxxxxxxxx>

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,

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

ping! If there is feedback, I can get going right away.

Meanwhile, I am trying to make the code use the nested-ao

stuff, but I can send it as a separate patch on top of this.

shriram

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