Ian Campbell
2011-Feb-08 13:23 UTC
[Xen-devel] [PATCH] libxl/xl: improve behaviour when guest fails to suspend itself
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1297171349 0
# Node ID ad6f318aac5dcdc5a7be03a4a80f27ed80729638
# Parent deaa7bc1a7ff6cfad7865394cb8b7205741004c9
libxl/xl: improve behaviour when guest fails to suspend itself.
The PV suspend protocol requires guest co-operating whereb the guest
must respond to a suspend request written to the xenstore control node
by clearing the node and then making a suspend hypercall.
Currently when a guest fails to do this libxl times out and returns
a generic failure code to the caller.
In response to this failure xl attempts to resume the guest. However
if the guest has not responded to the suspend request then the is no
guarantee that the guest has made the suspend hypercall (in fact it is
quite unlikely). Since the resume process attempts to modify the
return value of the hypercall (to indicate a cancelled suspend) this
results in the guest eax/rax register being corrupted!
Firstly ensure that libxl cancels the suspend request in a synchronous
manner, to avoid racing with a slow guest. Both pvops and classic-Xen
guests read and clear the control node in a single transaction so it
sufficient to do the same on the toolstack side.
Secondly add a mechanism to propagate guest timeouts from the
xc_domain_suspend ->suspend callback
(libxl__domain_suspend_common_callback in the case of libxl) to the
caller (libxl__domain_suspend_common).
Thirdly add a new libxl error code to indicate that the guest did not
respond in time.
Lastly in xl do not attempt to resume a guest if it has not responded
to the suspend request.
Tested by live migration of PVops kernels which either ignore the
suspend request, have already crashed and those which suspend/resume
correctly. In the first two cases the source domain is left alone (and
continues to function in the first case) and in the third the
migration is successful.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r deaa7bc1a7ff -r ad6f318aac5d tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Tue Feb 08 09:13:38 2011 +0000
+++ b/tools/libxl/libxl.h Tue Feb 08 13:22:29 2011 +0000
@@ -239,6 +239,7 @@ enum {
ERROR_NOMEM = -5,
ERROR_INVAL = -6,
ERROR_BADFAIL = -7,
+ ERROR_GUEST_TIMEDOUT = -8,
};
#define LIBXL_VERSION 0
diff -r deaa7bc1a7ff -r ad6f318aac5d tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Tue Feb 08 09:13:38 2011 +0000
+++ b/tools/libxl/libxl_dom.c Tue Feb 08 13:22:29 2011 +0000
@@ -319,7 +319,7 @@ struct suspendinfo {
int suspend_eventchn;
int domid;
int hvm;
- unsigned int flags;
+ int guest_responded;
};
static int libxl__domain_suspend_common_switch_qemu_logdirty(int domid,
unsigned int enable, void *data)
@@ -349,6 +349,7 @@ static int libxl__domain_suspend_common_
char *path, *state = "suspend";
int watchdog = 60;
libxl_ctx *ctx = libxl__gc_owner(si->gc);
+ xs_transaction_t t;
if (si->hvm)
xc_get_hvm_param(ctx->xch, si->domid, HVM_PARAM_ACPI_S_STATE,
&s_state);
@@ -363,6 +364,7 @@ static int libxl__domain_suspend_common_
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "xc_await_suspend failed
ret=%d", ret);
return 0;
}
+ si->guest_responded = 1;
return 1;
}
path = libxl__sprintf(si->gc, "%s/control/shutdown",
libxl__xs_get_dompath(si->gc, si->domid));
@@ -386,16 +388,40 @@ static int libxl__domain_suspend_common_
int shutdown_reason;
shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
& XEN_DOMINF_shutdownmask;
- if (shutdown_reason == SHUTDOWN_suspend)
+ if (shutdown_reason == SHUTDOWN_suspend) {
+ si->guest_responded = 1;
return 1;
+ }
}
state = libxl__xs_read(si->gc, XBT_NULL, path);
watchdog--;
}
+
+ /*
+ * Guest appear to not be responding, clear the suspend request
+ * synchronously using a transaction.
+ */
if (!strcmp(state, "suspend")) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t suspend
in time");
- libxl__xs_write(si->gc, XBT_NULL, path, "");
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t suspend
in time, clearing suspend request");
+ retry_transaction:
+ t = xs_transaction_start(ctx->xsh);
+
+ state = libxl__xs_read(si->gc, t, path);
+
+ libxl__xs_write(si->gc, t, path, "");
+
+ if (!xs_transaction_end(ctx->xsh, t, 0))
+ if (errno == EAGAIN)
+ goto retry_transaction;
}
+
+ /* Final check, guest may have suspended while we were clearing the
request. */
+ if (!strcmp(state, "suspend")) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "final check, guest
didn''t suspend");
+ return 0;
+ }
+
+ si->guest_responded = 1;
return 1;
}
@@ -414,10 +440,10 @@ int libxl__domain_suspend_common(libxl_c
| (hvm) ? XCFLAGS_HVM : 0;
si.domid = domid;
- si.flags = flags;
si.hvm = hvm;
si.gc = &gc;
si.suspend_eventchn = -1;
+ si.guest_responded = 0;
si.xce = xc_evtchn_open(NULL, 0);
if (si.xce == NULL)
@@ -441,8 +467,14 @@ int libxl__domain_suspend_common(libxl_c
rc = xc_domain_save(ctx->xch, fd, domid, 0, 0, flags, &callbacks,
hvm);
if ( rc ) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "saving domain");
- rc = ERROR_FAIL;
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "saving domain: %s",
+ si.guest_responded ?
+ "domain responded to suspend request" :
+ "domain did not respond to suspend
request");
+ if ( !si.guest_responded )
+ rc = ERROR_GUEST_TIMEDOUT;
+ else
+ rc = ERROR_FAIL;
}
if (si.suspend_eventchn > 0)
diff -r deaa7bc1a7ff -r ad6f318aac5d tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Tue Feb 08 09:13:38 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c Tue Feb 08 13:22:29 2011 +0000
@@ -2583,7 +2583,10 @@ static void migrate_domain(const char *d
if (rc) {
fprintf(stderr, "migration sender: libxl_domain_suspend
failed"
" (rc=%d)\n", rc);
- goto failed_resume;
+ if (rc == ERROR_GUEST_TIMEDOUT)
+ goto failed_suspend;
+ else
+ goto failed_resume;
}
//fprintf(stderr, "migration sender: Transfer complete.\n");
@@ -2661,6 +2664,12 @@ static void migrate_domain(const char *d
libxl_domain_destroy(&ctx, domid, 1); /* bang! */
fprintf(stderr, "Migration successful.\n");
exit(0);
+
+ failed_suspend:
+ close(send_fd);
+ migration_child_report(child, recv_fd);
+ fprintf(stderr, "Migration failed, failed to suspend at
sender.\n");
+ exit(-ERROR_FAIL);
failed_resume:
close(send_fd);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Feb-08 15:06 UTC
Re: [Xen-devel] [PATCH] libxl/xl: improve behaviour when guest fails to suspend itself
On Tue, 8 Feb 2011, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1297171349 0 > # Node ID ad6f318aac5dcdc5a7be03a4a80f27ed80729638 > # Parent deaa7bc1a7ff6cfad7865394cb8b7205741004c9 > libxl/xl: improve behaviour when guest fails to suspend itself. > > The PV suspend protocol requires guest co-operating whereb the guest > must respond to a suspend request written to the xenstore control node > by clearing the node and then making a suspend hypercall. > > Currently when a guest fails to do this libxl times out and returns > a generic failure code to the caller. > > In response to this failure xl attempts to resume the guest. However > if the guest has not responded to the suspend request then the is no > guarantee that the guest has made the suspend hypercall (in fact it is > quite unlikely). Since the resume process attempts to modify the > return value of the hypercall (to indicate a cancelled suspend) this > results in the guest eax/rax register being corrupted! > > Firstly ensure that libxl cancels the suspend request in a synchronous > manner, to avoid racing with a slow guest. Both pvops and classic-Xen > guests read and clear the control node in a single transaction so it > sufficient to do the same on the toolstack side. > > Secondly add a mechanism to propagate guest timeouts from the > xc_domain_suspend ->suspend callback > (libxl__domain_suspend_common_callback in the case of libxl) to the > caller (libxl__domain_suspend_common). > > Thirdly add a new libxl error code to indicate that the guest did not > respond in time. > > Lastly in xl do not attempt to resume a guest if it has not responded > to the suspend request. > > Tested by live migration of PVops kernels which either ignore the > suspend request, have already crashed and those which suspend/resume > correctly. In the first two cases the source domain is left alone (and > continues to function in the first case) and in the third the > migration is successful. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > diff -r deaa7bc1a7ff -r ad6f318aac5d tools/libxl/libxl.h > --- a/tools/libxl/libxl.h Tue Feb 08 09:13:38 2011 +0000 > +++ b/tools/libxl/libxl.h Tue Feb 08 13:22:29 2011 +0000 > @@ -239,6 +239,7 @@ enum { > ERROR_NOMEM = -5, > ERROR_INVAL = -6, > ERROR_BADFAIL = -7, > + ERROR_GUEST_TIMEDOUT = -8, > }; > > #define LIBXL_VERSION 0 > diff -r deaa7bc1a7ff -r ad6f318aac5d tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Tue Feb 08 09:13:38 2011 +0000 > +++ b/tools/libxl/libxl_dom.c Tue Feb 08 13:22:29 2011 +0000 > @@ -319,7 +319,7 @@ struct suspendinfo { > int suspend_eventchn; > int domid; > int hvm; > - unsigned int flags; > + int guest_responded; > }; >Even though they are not currently used, I would keep the flags field.> static int libxl__domain_suspend_common_switch_qemu_logdirty(int domid, unsigned int enable, void *data) > @@ -349,6 +349,7 @@ static int libxl__domain_suspend_common_ > char *path, *state = "suspend"; > int watchdog = 60; > libxl_ctx *ctx = libxl__gc_owner(si->gc); > + xs_transaction_t t; > > if (si->hvm) > xc_get_hvm_param(ctx->xch, si->domid, HVM_PARAM_ACPI_S_STATE, &s_state); > @@ -363,6 +364,7 @@ static int libxl__domain_suspend_common_ > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "xc_await_suspend failed ret=%d", ret); > return 0; > } > + si->guest_responded = 1; > return 1; > } > path = libxl__sprintf(si->gc, "%s/control/shutdown", libxl__xs_get_dompath(si->gc, si->domid)); > @@ -386,16 +388,40 @@ static int libxl__domain_suspend_common_ > int shutdown_reason; > > shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask; > - if (shutdown_reason == SHUTDOWN_suspend) > + if (shutdown_reason == SHUTDOWN_suspend) { > + si->guest_responded = 1; > return 1; > + } > } > state = libxl__xs_read(si->gc, XBT_NULL, path); > watchdog--; > } > + > + /* > + * Guest appear to not be responding, clear the suspend request > + * synchronously using a transaction. > + */ > if (!strcmp(state, "suspend")) { > - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t suspend in time"); > - libxl__xs_write(si->gc, XBT_NULL, path, ""); > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t suspend in time, clearing suspend request"); > + retry_transaction: > + t = xs_transaction_start(ctx->xsh); > + > + state = libxl__xs_read(si->gc, t, path); > + > + libxl__xs_write(si->gc, t, path, ""); > +Doesn''t it make sense to clear path only if state is still "suspend"? Clearing the path again even if the guest already cleared it may call a spurious xenstore watch event in the guest, right?> + > + /* Final check, guest may have suspended while we were clearing the request. */ > + if (!strcmp(state, "suspend")) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "final check, guest didn''t suspend"); > + return 0; > + } > + > + si->guest_responded = 1; > return 1; > }If "suspend" was gone before we cleared it, shouldn''t we check xc_domain_getinfolist again before asserting that the domain suspended correcty? Or maybe is better just to return 0 without the final check, assuming that the guest failed in any case.> @@ -414,10 +440,10 @@ int libxl__domain_suspend_common(libxl_c > | (hvm) ? XCFLAGS_HVM : 0; > > si.domid = domid; > - si.flags = flags; > si.hvm = hvm; > si.gc = &gc; > si.suspend_eventchn = -1; > + si.guest_responded = 0; > > si.xce = xc_evtchn_open(NULL, 0); > if (si.xce == NULL) > @@ -441,8 +467,14 @@ int libxl__domain_suspend_common(libxl_c > > rc = xc_domain_save(ctx->xch, fd, domid, 0, 0, flags, &callbacks, hvm); > if ( rc ) { > - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "saving domain"); > - rc = ERROR_FAIL; > + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "saving domain: %s", > + si.guest_responded ? > + "domain responded to suspend request" : > + "domain did not respond to suspend request"); > + if ( !si.guest_responded ) > + rc = ERROR_GUEST_TIMEDOUT; > + else > + rc = ERROR_FAIL; > } > > if (si.suspend_eventchn > 0) > diff -r deaa7bc1a7ff -r ad6f318aac5d tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c Tue Feb 08 09:13:38 2011 +0000 > +++ b/tools/libxl/xl_cmdimpl.c Tue Feb 08 13:22:29 2011 +0000 > @@ -2583,7 +2583,10 @@ static void migrate_domain(const char *d > if (rc) { > fprintf(stderr, "migration sender: libxl_domain_suspend failed" > " (rc=%d)\n", rc); > - goto failed_resume; > + if (rc == ERROR_GUEST_TIMEDOUT) > + goto failed_suspend; > + else > + goto failed_resume; > } > > //fprintf(stderr, "migration sender: Transfer complete.\n"); > @@ -2661,6 +2664,12 @@ static void migrate_domain(const char *d > libxl_domain_destroy(&ctx, domid, 1); /* bang! */ > fprintf(stderr, "Migration successful.\n"); > exit(0); > + > + failed_suspend: > + close(send_fd); > + migration_child_report(child, recv_fd); > + fprintf(stderr, "Migration failed, failed to suspend at sender.\n"); > + exit(-ERROR_FAIL); > > failed_resume: > close(send_fd); > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Feb-08 15:24 UTC
Re: [Xen-devel] [PATCH] libxl/xl: improve behaviour when guest fails to suspend itself
On Tue, 2011-02-08 at 15:06 +0000, Stefano Stabellini wrote:> On Tue, 8 Feb 2011, Ian Campbell wrote: > diff -r deaa7bc1a7ff -r ad6f318aac5d tools/libxl/libxl_dom.c > > --- a/tools/libxl/libxl_dom.c Tue Feb 08 09:13:38 2011 +0000 > > +++ b/tools/libxl/libxl_dom.c Tue Feb 08 13:22:29 2011 +0000 > > @@ -319,7 +319,7 @@ struct suspendinfo { > > int suspend_eventchn; > > int domid; > > int hvm; > > - unsigned int flags; > > + int guest_responded; > > }; > > > > Even though they are not currently used, I would keep the flags field.OK.> > + /* > > + * Guest appear to not be responding, clear the suspend request > > + * synchronously using a transaction. > > + */ > > if (!strcmp(state, "suspend")) { > > - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t suspend in time"); > > - libxl__xs_write(si->gc, XBT_NULL, path, ""); > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t suspend in time, clearing suspend request"); > > + retry_transaction: > > + t = xs_transaction_start(ctx->xsh); > > + > > + state = libxl__xs_read(si->gc, t, path); > > + > > + libxl__xs_write(si->gc, t, path, ""); > > + > > Doesn''t it make sense to clear path only if state is still "suspend"? > Clearing the path again even if the guest already cleared it may call a > spurious xenstore watch event in the guest, right?I guess it''s worth avoiding since its pretty trivial to do so, yes.> > + > > + /* Final check, guest may have suspended while we were clearing the request. */ > > + if (!strcmp(state, "suspend")) { > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "final check, guest didn''t suspend"); > > + return 0; > > + } > > + > > + si->guest_responded = 1; > > return 1; > > } > > If "suspend" was gone before we cleared it, shouldn''t we check > xc_domain_getinfolist again before asserting that the domain suspended > correcty?Hrm, yes. I think it works to pull that check out of the loop and only perform it once on the final exit from the function instead of checking every time around the loop. This will mean a second timeout loop after the guest acknowledges the suspend waiting for the actual suspend. I think this makes sense anyway rather than conflating the two in a single timeout.> Or maybe is better just to return 0 without the final check, assuming > that the guest failed in any case.I think if the guest cleared the xenstore node we need to assume it will have tried to suspend which is a different failure to not having tried. If the guest suspends without clearing the xenstore node then all bets are off anyway IMHO and we shouldn''t trust it to have suspended correctly and the toolstack can''t really be expected to recover so taking the "guest failed to respond" error path is as good as any other. I''ll make these changes and repost. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Feb-08 17:24 UTC
[Xen-devel] [PATCH] libxl/xl: improve behaviour when guest fails to suspend itself
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1297185819 0
# Node ID d631a4996cbc69a7fa8489f28d4a3313db12e77a
# Parent a46b91cd8202726aecd9ddefd8e75faff48144d6
libxl/xl: improve behaviour when guest fails to suspend itself.
The PV suspend protocol requires guest co-operating whereby the guest
must respond to a suspend request written to the xenstore control node
by clearing the node and then making a suspend hypercall.
Currently when a guest fails to do this libxl times out and returns
a generic failure code to the caller.
In response to this failure xl attempts to resume the guest. However
if the guest has not responded to the suspend request then the is no
guarantee that the guest has made the suspend hypercall (in fact it is
quite unlikely). Since the resume process attempts to modify the
return value of the hypercall (to indicate a cancelled suspend) this
results in the guest eax/rax register being corrupted!
To fix this change libxl to do the following:
* Wait for the guest to acknowledge the suspend request.
- on timeout cancel the suspend request.
- if cancellation is successful then return a new error code to
indicate that the guest is not responding.
- if the cancel does not succeed then we raced with the guest
which actually did acknowledge at the last minute, so
continue.
* Wait for the guest to suspend.
- on timeout return the standard error code as before
* Guest successfully suspended, return success.
Lastly in xl do not attempt to resume a guest if it has not responded
to the suspend request.
Tested by live migration of PVops kernels which either ignore the
suspend request, have already crashed and those which suspend/resume
correctly. In the first two cases the source domain is left alone (and
continues to function in the first case) and in the third the
migration is successful.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r a46b91cd8202 -r d631a4996cbc tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Tue Feb 08 16:26:07 2011 +0000
+++ b/tools/libxl/libxl.h Tue Feb 08 17:23:39 2011 +0000
@@ -239,6 +239,7 @@ enum {
ERROR_NOMEM = -5,
ERROR_INVAL = -6,
ERROR_BADFAIL = -7,
+ ERROR_GUEST_TIMEDOUT = -8,
};
#define LIBXL_VERSION 0
diff -r a46b91cd8202 -r d631a4996cbc tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Tue Feb 08 16:26:07 2011 +0000
+++ b/tools/libxl/libxl_dom.c Tue Feb 08 17:23:39 2011 +0000
@@ -320,6 +320,7 @@ struct suspendinfo {
int domid;
int hvm;
unsigned int flags;
+ int guest_responded;
};
static int libxl__domain_suspend_common_switch_qemu_logdirty(int domid,
unsigned int enable, void *data)
@@ -347,8 +348,9 @@ static int libxl__domain_suspend_common_
unsigned long s_state = 0;
int ret;
char *path, *state = "suspend";
- int watchdog = 60;
+ int watchdog;
libxl_ctx *ctx = libxl__gc_owner(si->gc);
+ xs_transaction_t t;
if (si->hvm)
xc_get_hvm_param(ctx->xch, si->domid, HVM_PARAM_ACPI_S_STATE,
&s_state);
@@ -363,6 +365,7 @@ static int libxl__domain_suspend_common_
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "xc_await_suspend failed
ret=%d", ret);
return 0;
}
+ si->guest_responded = 1;
return 1;
}
path = libxl__sprintf(si->gc, "%s/control/shutdown",
libxl__xs_get_dompath(si->gc, si->domid));
@@ -376,8 +379,56 @@ static int libxl__domain_suspend_common_
xc_domain_shutdown(ctx->xch, si->domid, SHUTDOWN_suspend);
}
}
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "wait for the guest to acknowledge
suspend request");
+ watchdog = 60;
+ while (!strcmp(state, "suspend") && watchdog > 0) {
+ usleep(100000);
+
+ state = libxl__xs_read(si->gc, XBT_NULL, path);
+
+ 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")) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t
acknowledge suspend, cancelling request");
+ retry_transaction:
+ t = xs_transaction_start(ctx->xsh);
+
+ state = libxl__xs_read(si->gc, t, path);
+
+ if (!strcmp(state, "suspend"))
+ libxl__xs_write(si->gc, t, path, "");
+
+ 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")) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t
acknowledge suspend, request cancelled");
+ return 0;
+ }
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "guest acknowledged suspend
request");
+ si->guest_responded = 1;
+
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "wait for the guest to
suspend");
- while (!strcmp(state, "suspend") && watchdog > 0) {
+ watchdog = 60;
+ while (watchdog > 0) {
xc_domaininfo_t info;
usleep(100000);
@@ -386,17 +437,17 @@ static int libxl__domain_suspend_common_
int shutdown_reason;
shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
& XEN_DOMINF_shutdownmask;
- if (shutdown_reason == SHUTDOWN_suspend)
+ if (shutdown_reason == SHUTDOWN_suspend) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "guest has
suspended");
return 1;
+ }
}
- state = libxl__xs_read(si->gc, XBT_NULL, path);
+
watchdog--;
}
- if (!strcmp(state, "suspend")) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t suspend
in time");
- libxl__xs_write(si->gc, XBT_NULL, path, "");
- }
- return 1;
+
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest did not suspend");
+ return 0;
}
int libxl__domain_suspend_common(libxl_ctx *ctx, uint32_t domid, int fd,
@@ -418,6 +469,7 @@ int libxl__domain_suspend_common(libxl_c
si.hvm = hvm;
si.gc = &gc;
si.suspend_eventchn = -1;
+ si.guest_responded = 0;
si.xce = xc_evtchn_open(NULL, 0);
if (si.xce == NULL)
@@ -441,8 +493,14 @@ int libxl__domain_suspend_common(libxl_c
rc = xc_domain_save(ctx->xch, fd, domid, 0, 0, flags, &callbacks,
hvm);
if ( rc ) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "saving domain");
- rc = ERROR_FAIL;
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "saving domain: %s",
+ si.guest_responded ?
+ "domain responded to suspend request" :
+ "domain did not respond to suspend
request");
+ if ( !si.guest_responded )
+ rc = ERROR_GUEST_TIMEDOUT;
+ else
+ rc = ERROR_FAIL;
}
if (si.suspend_eventchn > 0)
diff -r a46b91cd8202 -r d631a4996cbc tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Tue Feb 08 16:26:07 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c Tue Feb 08 17:23:39 2011 +0000
@@ -2583,7 +2583,10 @@ static void migrate_domain(const char *d
if (rc) {
fprintf(stderr, "migration sender: libxl_domain_suspend
failed"
" (rc=%d)\n", rc);
- goto failed_resume;
+ if (rc == ERROR_GUEST_TIMEDOUT)
+ goto failed_suspend;
+ else
+ goto failed_resume;
}
//fprintf(stderr, "migration sender: Transfer complete.\n");
@@ -2661,6 +2664,12 @@ static void migrate_domain(const char *d
libxl_domain_destroy(&ctx, domid, 1); /* bang! */
fprintf(stderr, "Migration successful.\n");
exit(0);
+
+ failed_suspend:
+ close(send_fd);
+ migration_child_report(child, recv_fd);
+ fprintf(stderr, "Migration failed, failed to suspend at
sender.\n");
+ exit(-ERROR_FAIL);
failed_resume:
close(send_fd);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Feb-08 17:25 UTC
[Xen-devel] Re: [PATCH] libxl/xl: improve behaviour when guest fails to suspend itself
BTW, this depends on "libxl: allow guest to write "control/shutdown" xenstore node" I''m vaguely hopeful that it will cause RHEL6 HVM suspend/migration to fail gracefully instead of collapsing in a heap. Ian. On Tue, 2011-02-08 at 17:24 +0000, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1297185819 0 > # Node ID d631a4996cbc69a7fa8489f28d4a3313db12e77a > # Parent a46b91cd8202726aecd9ddefd8e75faff48144d6 > libxl/xl: improve behaviour when guest fails to suspend itself. > > The PV suspend protocol requires guest co-operating whereby the guest > must respond to a suspend request written to the xenstore control node > by clearing the node and then making a suspend hypercall. > > Currently when a guest fails to do this libxl times out and returns > a generic failure code to the caller. > > In response to this failure xl attempts to resume the guest. However > if the guest has not responded to the suspend request then the is no > guarantee that the guest has made the suspend hypercall (in fact it is > quite unlikely). Since the resume process attempts to modify the > return value of the hypercall (to indicate a cancelled suspend) this > results in the guest eax/rax register being corrupted! > > To fix this change libxl to do the following: > * Wait for the guest to acknowledge the suspend request. > - on timeout cancel the suspend request. > - if cancellation is successful then return a new error code to > indicate that the guest is not responding. > - if the cancel does not succeed then we raced with the guest > which actually did acknowledge at the last minute, so > continue. > * Wait for the guest to suspend. > - on timeout return the standard error code as before > * Guest successfully suspended, return success. > > Lastly in xl do not attempt to resume a guest if it has not responded > to the suspend request. > > Tested by live migration of PVops kernels which either ignore the > suspend request, have already crashed and those which suspend/resume > correctly. In the first two cases the source domain is left alone (and > continues to function in the first case) and in the third the > migration is successful. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > diff -r a46b91cd8202 -r d631a4996cbc tools/libxl/libxl.h > --- a/tools/libxl/libxl.h Tue Feb 08 16:26:07 2011 +0000 > +++ b/tools/libxl/libxl.h Tue Feb 08 17:23:39 2011 +0000 > @@ -239,6 +239,7 @@ enum { > ERROR_NOMEM = -5, > ERROR_INVAL = -6, > ERROR_BADFAIL = -7, > + ERROR_GUEST_TIMEDOUT = -8, > }; > > #define LIBXL_VERSION 0 > diff -r a46b91cd8202 -r d631a4996cbc tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Tue Feb 08 16:26:07 2011 +0000 > +++ b/tools/libxl/libxl_dom.c Tue Feb 08 17:23:39 2011 +0000 > @@ -320,6 +320,7 @@ struct suspendinfo { > int domid; > int hvm; > unsigned int flags; > + int guest_responded; > }; > > static int libxl__domain_suspend_common_switch_qemu_logdirty(int domid, unsigned int enable, void *data) > @@ -347,8 +348,9 @@ static int libxl__domain_suspend_common_ > unsigned long s_state = 0; > int ret; > char *path, *state = "suspend"; > - int watchdog = 60; > + int watchdog; > libxl_ctx *ctx = libxl__gc_owner(si->gc); > + xs_transaction_t t; > > if (si->hvm) > xc_get_hvm_param(ctx->xch, si->domid, HVM_PARAM_ACPI_S_STATE, &s_state); > @@ -363,6 +365,7 @@ static int libxl__domain_suspend_common_ > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "xc_await_suspend failed ret=%d", ret); > return 0; > } > + si->guest_responded = 1; > return 1; > } > path = libxl__sprintf(si->gc, "%s/control/shutdown", libxl__xs_get_dompath(si->gc, si->domid)); > @@ -376,8 +379,56 @@ static int libxl__domain_suspend_common_ > xc_domain_shutdown(ctx->xch, si->domid, SHUTDOWN_suspend); > } > } > + > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "wait for the guest to acknowledge suspend request"); > + watchdog = 60; > + while (!strcmp(state, "suspend") && watchdog > 0) { > + usleep(100000); > + > + state = libxl__xs_read(si->gc, XBT_NULL, path); > + > + 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")) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t acknowledge suspend, cancelling request"); > + retry_transaction: > + t = xs_transaction_start(ctx->xsh); > + > + state = libxl__xs_read(si->gc, t, path); > + > + if (!strcmp(state, "suspend")) > + libxl__xs_write(si->gc, t, path, ""); > + > + 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")) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t acknowledge suspend, request cancelled"); > + return 0; > + } > + > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "guest acknowledged suspend request"); > + si->guest_responded = 1; > + > LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "wait for the guest to suspend"); > - while (!strcmp(state, "suspend") && watchdog > 0) { > + watchdog = 60; > + while (watchdog > 0) { > xc_domaininfo_t info; > > usleep(100000); > @@ -386,17 +437,17 @@ static int libxl__domain_suspend_common_ > int shutdown_reason; > > shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask; > - if (shutdown_reason == SHUTDOWN_suspend) > + if (shutdown_reason == SHUTDOWN_suspend) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "guest has suspended"); > return 1; > + } > } > - state = libxl__xs_read(si->gc, XBT_NULL, path); > + > watchdog--; > } > - if (!strcmp(state, "suspend")) { > - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest didn''t suspend in time"); > - libxl__xs_write(si->gc, XBT_NULL, path, ""); > - } > - return 1; > + > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest did not suspend"); > + return 0; > } > > int libxl__domain_suspend_common(libxl_ctx *ctx, uint32_t domid, int fd, > @@ -418,6 +469,7 @@ int libxl__domain_suspend_common(libxl_c > si.hvm = hvm; > si.gc = &gc; > si.suspend_eventchn = -1; > + si.guest_responded = 0; > > si.xce = xc_evtchn_open(NULL, 0); > if (si.xce == NULL) > @@ -441,8 +493,14 @@ int libxl__domain_suspend_common(libxl_c > > rc = xc_domain_save(ctx->xch, fd, domid, 0, 0, flags, &callbacks, hvm); > if ( rc ) { > - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "saving domain"); > - rc = ERROR_FAIL; > + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "saving domain: %s", > + si.guest_responded ? > + "domain responded to suspend request" : > + "domain did not respond to suspend request"); > + if ( !si.guest_responded ) > + rc = ERROR_GUEST_TIMEDOUT; > + else > + rc = ERROR_FAIL; > } > > if (si.suspend_eventchn > 0) > diff -r a46b91cd8202 -r d631a4996cbc tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c Tue Feb 08 16:26:07 2011 +0000 > +++ b/tools/libxl/xl_cmdimpl.c Tue Feb 08 17:23:39 2011 +0000 > @@ -2583,7 +2583,10 @@ static void migrate_domain(const char *d > if (rc) { > fprintf(stderr, "migration sender: libxl_domain_suspend failed" > " (rc=%d)\n", rc); > - goto failed_resume; > + if (rc == ERROR_GUEST_TIMEDOUT) > + goto failed_suspend; > + else > + goto failed_resume; > } > > //fprintf(stderr, "migration sender: Transfer complete.\n"); > @@ -2661,6 +2664,12 @@ static void migrate_domain(const char *d > libxl_domain_destroy(&ctx, domid, 1); /* bang! */ > fprintf(stderr, "Migration successful.\n"); > exit(0); > + > + failed_suspend: > + close(send_fd); > + migration_child_report(child, recv_fd); > + fprintf(stderr, "Migration failed, failed to suspend at sender.\n"); > + exit(-ERROR_FAIL); > > failed_resume: > close(send_fd);_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Feb-09 16:23 UTC
Re: [Xen-devel] [PATCH] libxl/xl: improve behaviour when guest fails to suspend itself
On Tue, 8 Feb 2011, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1297185819 0 > # Node ID d631a4996cbc69a7fa8489f28d4a3313db12e77a > # Parent a46b91cd8202726aecd9ddefd8e75faff48144d6 > libxl/xl: improve behaviour when guest fails to suspend itself. > > The PV suspend protocol requires guest co-operating whereby the guest > must respond to a suspend request written to the xenstore control node > by clearing the node and then making a suspend hypercall. > > Currently when a guest fails to do this libxl times out and returns > a generic failure code to the caller. > > In response to this failure xl attempts to resume the guest. However > if the guest has not responded to the suspend request then the is no > guarantee that the guest has made the suspend hypercall (in fact it is > quite unlikely). Since the resume process attempts to modify the > return value of the hypercall (to indicate a cancelled suspend) this > results in the guest eax/rax register being corrupted! > > To fix this change libxl to do the following: > * Wait for the guest to acknowledge the suspend request. > - on timeout cancel the suspend request. > - if cancellation is successful then return a new error code to > indicate that the guest is not responding. > - if the cancel does not succeed then we raced with the guest > which actually did acknowledge at the last minute, so > continue. > * Wait for the guest to suspend. > - on timeout return the standard error code as before > * Guest successfully suspended, return success. > > Lastly in xl do not attempt to resume a guest if it has not responded > to the suspend request. > > Tested by live migration of PVops kernels which either ignore the > suspend request, have already crashed and those which suspend/resume > correctly. In the first two cases the source domain is left alone (and > continues to function in the first case) and in the third the > migration is successful. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Feb-11 17:57 UTC
Re: [Xen-devel] [PATCH] libxl/xl: improve behaviour when guest fails to suspend itself
Ian Campbell writes ("[Xen-devel] [PATCH] libxl/xl: improve behaviour when
guest fails to suspend itself"):> libxl/xl: improve behaviour when guest fails to suspend itself.
Thanks, applied this, and the previous patch too.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel