* [PATCH 01/19] libxl: init: Provide a gc later in libxl_ctx_alloc
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 16:20 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 02/19] libxl: init: libxl__poller_init and _get take gc Ian Jackson
` (18 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Provide libxl__gc *gc for the second half of libxl_ctx_alloc.
(For the first half of the function, gc is in scope but set to NULL.)
This makes it possible to make gc-requiring calls. For example, it
makes error logging more convenient.
Make use of this by changing the logging calls to use the LOG*
convenience macros.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 730f6e1..4b5abc8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -25,6 +25,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
unsigned flags, xentoollog_logger * lg)
{
libxl_ctx *ctx = NULL;
+ libxl__gc gc_buf, *gc = NULL;
int rc;
if (version != LIBXL_VERSION) { rc = ERROR_VERSION; goto out; }
@@ -79,6 +80,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
}
/* Now ctx is safe for ctx_free; failures simply set rc and "goto out" */
+ LIBXL_INIT_GC(gc_buf,ctx);
+ gc = &gc_buf;
+ /* Now gc is useable */
rc = libxl__atfork_init(ctx);
if (rc) goto out;
@@ -88,8 +92,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
ctx->xch = xc_interface_open(lg,lg,0);
if (!ctx->xch) {
- LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
- "cannot open libxc handle");
+ LOGEV(ERROR, errno, "cannot open libxc handle");
rc = ERROR_FAIL; goto out;
}
@@ -97,8 +100,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
if (!ctx->xsh)
ctx->xsh = xs_domain_open();
if (!ctx->xsh) {
- LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
- "cannot connect to xenstore");
+ LOGEV(ERROR, errno, "cannot connect to xenstore");
rc = ERROR_FAIL; goto out;
}
@@ -106,6 +108,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
return 0;
out:
+ if (gc) libxl__free_all(gc);
libxl_ctx_free(ctx);
*pctx = NULL;
return rc;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 01/19] libxl: init: Provide a gc later in libxl_ctx_alloc
2014-03-04 14:56 ` [PATCH 01/19] libxl: init: Provide a gc later in libxl_ctx_alloc Ian Jackson
@ 2014-03-13 16:20 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 16:20 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Provide libxl__gc *gc for the second half of libxl_ctx_alloc.
> (For the first half of the function, gc is in scope but set to NULL.)
>
> This makes it possible to make gc-requiring calls. For example, it
> makes error logging more convenient.
>
> Make use of this by changing the logging calls to use the LOG*
> convenience macros.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 02/19] libxl: init: libxl__poller_init and _get take gc
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
2014-03-04 14:56 ` [PATCH 01/19] libxl: init: Provide a gc later in libxl_ctx_alloc Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 16:21 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 03/19] libxl: events: const-correct *_inuse, *_isregistered Ian Jackson
` (17 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Change libxl__poller_init and libxl__poller__get to take a libxl__gc*
rather than a libxl_ctx*. The gc is not used for memory allocation
but simply to provide the standard local variable "gc" expected by the
convenience macros. Doing this makes the error logging more
convenient.
Hence, convert the logging calls to use the LOG* convenience macros.
And consequently, change the call sites, and the function bodies to
use CTX rather than ctx.
Also convert a call to malloc() (with error check) in
libxl__poller_get, to libxl__zalloc (no error check needed).
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 2 +-
tools/libxl/libxl_event.c | 21 ++++++++-------------
tools/libxl/libxl_internal.h | 4 ++--
3 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4b5abc8..5415ebd 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -87,7 +87,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
rc = libxl__atfork_init(ctx);
if (rc) goto out;
- rc = libxl__poller_init(ctx, &ctx->poller_app);
+ rc = libxl__poller_init(gc, &ctx->poller_app);
if (rc) goto out;
ctx->xch = xc_interface_open(lg,lg,0);
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index ea8c744..3e465af 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1347,13 +1347,13 @@ int libxl__self_pipe_eatall(int fd)
* Manipulation of pollers
*/
-int libxl__poller_init(libxl_ctx *ctx, libxl__poller *p)
+int libxl__poller_init(libxl__gc *gc, libxl__poller *p)
{
int rc;
p->fd_polls = 0;
p->fd_rindices = 0;
- rc = libxl__pipe_nonblock(ctx, p->wakeup_pipe);
+ rc = libxl__pipe_nonblock(CTX, p->wakeup_pipe);
if (rc) goto out;
return 0;
@@ -1370,25 +1370,20 @@ void libxl__poller_dispose(libxl__poller *p)
free(p->fd_rindices);
}
-libxl__poller *libxl__poller_get(libxl_ctx *ctx)
+libxl__poller *libxl__poller_get(libxl__gc *gc)
{
/* must be called with ctx locked */
int rc;
- libxl__poller *p = LIBXL_LIST_FIRST(&ctx->pollers_idle);
+ libxl__poller *p = LIBXL_LIST_FIRST(&CTX->pollers_idle);
if (p) {
LIBXL_LIST_REMOVE(p, entry);
return p;
}
- p = malloc(sizeof(*p));
- if (!p) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot allocate poller");
- return 0;
- }
- memset(p, 0, sizeof(*p));
+ p = libxl__zalloc(NOGC, sizeof(*p));
- rc = libxl__poller_init(ctx, p);
+ rc = libxl__poller_init(gc, p);
if (rc) {
free(p);
return NULL;
@@ -1477,7 +1472,7 @@ int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_r,
EGC_INIT(ctx);
CTX_LOCK;
- poller = libxl__poller_get(ctx);
+ poller = libxl__poller_get(gc);
if (!poller) { rc = ERROR_FAIL; goto out; }
for (;;) {
@@ -1653,7 +1648,7 @@ libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t domid,
if (how) {
ao->how = *how;
} else {
- ao->poller = libxl__poller_get(ctx);
+ ao->poller = libxl__poller_get(&ao->gc);
if (!ao->poller) goto out;
}
libxl__log(ctx,XTL_DEBUG,-1,file,line,func,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9d17586..b67ed79 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -834,13 +834,13 @@ _hidden void libxl__event_disaster(libxl__egc*, const char *msg, int errnoval,
/* Fills in, or disposes of, the resources held by, a poller whose
* space the caller has allocated. ctx must be locked. */
-_hidden int libxl__poller_init(libxl_ctx *ctx, libxl__poller *p);
+_hidden int libxl__poller_init(libxl__gc *gc, libxl__poller *p);
_hidden void libxl__poller_dispose(libxl__poller *p);
/* Obtain a fresh poller from malloc or the idle list, and put it
* away again afterwards. _get can fail, returning NULL.
* ctx must be locked. */
-_hidden libxl__poller *libxl__poller_get(libxl_ctx *ctx);
+_hidden libxl__poller *libxl__poller_get(libxl__gc *gc);
_hidden void libxl__poller_put(libxl_ctx*, libxl__poller *p /* may be NULL */);
/* Notifies whoever is polling using p that they should wake up.
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 02/19] libxl: init: libxl__poller_init and _get take gc
2014-03-04 14:56 ` [PATCH 02/19] libxl: init: libxl__poller_init and _get take gc Ian Jackson
@ 2014-03-13 16:21 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 16:21 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Change libxl__poller_init and libxl__poller__get to take a libxl__gc*
> rather than a libxl_ctx*. The gc is not used for memory allocation
> but simply to provide the standard local variable "gc" expected by the
> convenience macros. Doing this makes the error logging more
> convenient.
>
> Hence, convert the logging calls to use the LOG* convenience macros.
>
> And consequently, change the call sites, and the function bodies to
> use CTX rather than ctx.
>
> Also convert a call to malloc() (with error check) in
> libxl__poller_get, to libxl__zalloc (no error check needed).
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 03/19] libxl: events: const-correct *_inuse, *_isregistered
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
2014-03-04 14:56 ` [PATCH 01/19] libxl: init: Provide a gc later in libxl_ctx_alloc Ian Jackson
2014-03-04 14:56 ` [PATCH 02/19] libxl: init: libxl__poller_init and _get take gc Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-04 14:56 ` [PATCH 04/19] libxl: events: Provide libxl__xswait_* Ian Jackson
` (16 subsequent siblings)
19 siblings, 0 replies; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
The comments for libxl__ev_time_isregistered and the corresponding
watch function even say that these should be const. Make it so.
Also fix libxl__ev_child_inuse and libxl__ev_spawn_inuse.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_internal.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b67ed79..9e02861 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -727,7 +727,7 @@ _hidden int libxl__ev_fd_modify(libxl__gc*, libxl__ev_fd *ev,
_hidden void libxl__ev_fd_deregister(libxl__gc*, libxl__ev_fd *ev);
static inline void libxl__ev_fd_init(libxl__ev_fd *efd)
{ efd->fd = -1; }
-static inline int libxl__ev_fd_isregistered(libxl__ev_fd *efd)
+static inline int libxl__ev_fd_isregistered(const libxl__ev_fd *efd)
{ return efd->fd >= 0; }
_hidden int libxl__ev_time_register_rel(libxl__gc*, libxl__ev_time *ev_out,
@@ -743,7 +743,7 @@ _hidden int libxl__ev_time_modify_abs(libxl__gc*, libxl__ev_time *ev,
_hidden void libxl__ev_time_deregister(libxl__gc*, libxl__ev_time *ev);
static inline void libxl__ev_time_init(libxl__ev_time *ev)
{ ev->func = 0; }
-static inline int libxl__ev_time_isregistered(libxl__ev_time *ev)
+static inline int libxl__ev_time_isregistered(const libxl__ev_time *ev)
{ return !!ev->func; }
@@ -783,7 +783,7 @@ _hidden pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *childw_out,
libxl__ev_child_callback *death);
static inline void libxl__ev_child_init(libxl__ev_child *childw_out)
{ childw_out->pid = -1; }
-static inline int libxl__ev_child_inuse(libxl__ev_child *childw_out)
+static inline int libxl__ev_child_inuse(const libxl__ev_child *childw_out)
{ return childw_out->pid >= 0; }
/* Useable (only) in the child to once more make the ctx useable for
@@ -1225,7 +1225,7 @@ struct libxl__spawn_state {
libxl__ev_xswatch xswatch;
};
-static inline int libxl__spawn_inuse(libxl__spawn_state *ss)
+static inline int libxl__spawn_inuse(const libxl__spawn_state *ss)
{ return libxl__ev_child_inuse(&ss->mid); }
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 04/19] libxl: events: Provide libxl__xswait_*
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (2 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 03/19] libxl: events: const-correct *_inuse, *_isregistered Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 16:33 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code Ian Jackson
` (15 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
This is an ao utility for for conveniently doing a timed wait on
xenstore. It handles setting up and cancelling the timeout, and also
conveniently reads the key for you.
No callers yet in this patch.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
v2 fix doc comments to refer to correct rc values
---
tools/libxl/libxl_aoutils.c | 77 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 52 ++++++++++++++++++++++++++++
2 files changed, 129 insertions(+)
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index b4eb6e5..477717b 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -16,6 +16,83 @@
#include "libxl_internal.h"
+/*----- xswait -----*/
+
+static libxl__ev_xswatch_callback xswait_xswatch_callback;
+static libxl__ev_time_callback xswait_timeout_callback;
+static void xswait_report_error(libxl__egc*, libxl__xswait_state*, int rc);
+
+void libxl__xswait_init(libxl__xswait_state *xswa)
+{
+ libxl__ev_time_init(&xswa->time_ev);
+ libxl__ev_xswatch_init(&xswa->watch_ev);
+}
+
+void libxl__xswait_stop(libxl__gc *gc, libxl__xswait_state *xswa)
+{
+ libxl__ev_time_deregister(gc, &xswa->time_ev);
+ libxl__ev_xswatch_deregister(gc, &xswa->watch_ev);
+}
+
+bool libxl__xswait_inuse(const libxl__xswait_state *xswa)
+{
+ bool time_inuse = libxl__ev_time_isregistered(&xswa->time_ev);
+ bool watch_inuse = libxl__ev_xswatch_isregistered(&xswa->watch_ev);
+ assert(time_inuse == watch_inuse);
+ return time_inuse;
+}
+
+int libxl__xswait_start(libxl__gc *gc, libxl__xswait_state *xswa)
+{
+ int rc;
+
+ rc = libxl__ev_time_register_rel(gc, &xswa->time_ev,
+ xswait_timeout_callback, xswa->timeout_ms);
+ if (rc) goto err;
+
+ rc = libxl__ev_xswatch_register(gc, &xswa->watch_ev,
+ xswait_xswatch_callback, xswa->path);
+ if (rc) goto err;
+
+ return 0;
+
+ err:
+ libxl__xswait_stop(gc, xswa);
+ return rc;
+}
+
+void xswait_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *xsw,
+ const char *watch_path, const char *event_path)
+{
+ EGC_GC;
+ libxl__xswait_state *xswa = CONTAINER_OF(xsw, *xswa, watch_ev);
+ int rc;
+ const char *data;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL, xswa->path, &data);
+ if (rc) { xswait_report_error(egc, xswa, rc); return; }
+
+ xswa->callback(egc, xswa, 0, data);
+}
+
+void xswait_timeout_callback(libxl__egc *egc, libxl__ev_time *ev,
+ const struct timeval *requested_abs)
+{
+ EGC_GC;
+ libxl__xswait_state *xswa = CONTAINER_OF(ev, *xswa, time_ev);
+ LOG(DEBUG, "%s: xswait timeout (path=%s)", xswa->what, xswa->path);
+ xswait_report_error(egc, xswa, ERROR_TIMEDOUT);
+}
+
+static void xswait_report_error(libxl__egc *egc, libxl__xswait_state *xswa,
+ int rc)
+{
+ EGC_GC;
+ libxl__xswait_stop(gc, xswa);
+ xswa->callback(egc, xswa, rc, 0);
+}
+
+
/*----- data copier -----*/
void libxl__datacopier_init(libxl__datacopier_state *dc)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9e02861..644fcee 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1080,6 +1080,58 @@ _hidden int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, int num);
_hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);
+/*----- xswait: wait for a xenstore node to be suitable -----*/
+
+typedef struct libxl__xswait_state libxl__xswait_state;
+
+/*
+ * rc describes the circumstances of this callback:
+ *
+ * rc==0
+ *
+ * The xenstore path (may have) changed. It has been read for
+ * you. The result is in data (allocated from the ao gc).
+ * data may be NULL, which means that the xenstore read gave
+ * ENOENT.
+ *
+ * If you are satisfied, you MUST call libxl__xswait_stop.
+ * Otherwise, xswait will continue waiting and watching and
+ * will call you back later.
+ *
+ * rc==ERROR_TIMEDOUT
+ *
+ * The specified timeout was reached.
+ * This has NOT been logged (except to the debug log).
+ * xswait will not continue (but calling libxl__xswait_stop is OK).
+ *
+ * rc!=0, !=ERROR_TIMEDOUT
+ *
+ * Some other error occurred.
+ * This HAS been logged.
+ * xswait will not continue (but calling libxl__xswait_stop is OK).
+ *
+ */
+typedef void libxl__xswait_callback(libxl__egc *egc,
+ libxl__xswait_state *xswa, int rc, const char *data);
+
+struct libxl__xswait_state {
+ /* caller must fill these in, and they must all remain valid */
+ libxl__ao *ao;
+ const char *what; /* for error msgs: noun phrase, what we're waiting for */
+ const char *path;
+ int timeout_ms; /* as for poll(2) */
+ libxl__xswait_callback *callback;
+ /* remaining fields are private to xswait */
+ libxl__ev_time time_ev;
+ libxl__ev_xswatch watch_ev;
+};
+
+void libxl__xswait_init(libxl__xswait_state*);
+void libxl__xswait_stop(libxl__gc*, libxl__xswait_state*); /*idempotent*/
+bool libxl__xswait_inuse(const libxl__xswait_state *ss);
+
+int libxl__xswait_start(libxl__gc*, libxl__xswait_state*);
+
/*
*----- spawn -----
*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 04/19] libxl: events: Provide libxl__xswait_*
2014-03-04 14:56 ` [PATCH 04/19] libxl: events: Provide libxl__xswait_* Ian Jackson
@ 2014-03-13 16:33 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 16:33 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> This is an ao utility for for conveniently doing a timed wait on
> xenstore. It handles setting up and cancelling the timeout, and also
> conveniently reads the key for you.
>
> No callers yet in this patch.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (3 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 04/19] libxl: events: Provide libxl__xswait_* Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-10 3:35 ` Lai Jiangshan
2014-03-04 14:56 ` [PATCH 06/19] libxl: events: Provide libxl__ev_evtchn* Ian Jackson
` (14 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Replace open-coded use of ev_time and ev_xswatch with xswait.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_exec.c | 49 +++++++++++++++---------------------------
tools/libxl/libxl_internal.h | 3 +--
2 files changed, 18 insertions(+), 34 deletions(-)
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index b6efa0f..4b012dc 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -257,10 +257,8 @@ err:
*/
/* Event callbacks. */
-static void spawn_watch_event(libxl__egc *egc, libxl__ev_xswatch *xsw,
- const char *watch_path, const char *event_path);
-static void spawn_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs);
+static void spawn_watch_event(libxl__egc *egc, libxl__xswait_state *xswa,
+ int rc, const char *xsdata);
static void spawn_middle_death(libxl__egc *egc, libxl__ev_child *childw,
pid_t pid, int status);
@@ -274,8 +272,7 @@ static void spawn_fail(libxl__egc *egc, libxl__spawn_state *ss);
void libxl__spawn_init(libxl__spawn_state *ss)
{
libxl__ev_child_init(&ss->mid);
- libxl__ev_time_init(&ss->timeout);
- libxl__ev_xswatch_init(&ss->xswatch);
+ libxl__xswait_init(&ss->xswait);
}
int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
@@ -288,12 +285,12 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
libxl__spawn_init(ss);
ss->failed = ss->detaching = 0;
- rc = libxl__ev_time_register_rel(gc, &ss->timeout,
- spawn_timeout, ss->timeout_ms);
- if (rc) goto out_err;
-
- rc = libxl__ev_xswatch_register(gc, &ss->xswatch,
- spawn_watch_event, ss->xspath);
+ ss->xswait.ao = ao;
+ ss->xswait.what = GCSPRINTF("%s startup", ss->what);
+ ss->xswait.path = ss->xspath;
+ ss->xswait.timeout_ms = ss->timeout_ms;
+ ss->xswait.callback = spawn_watch_event;
+ rc = libxl__xswait_start(gc, &ss->xswait);
if (rc) goto out_err;
pid_t middle = libxl__ev_child_fork(gc, &ss->mid, spawn_middle_death);
@@ -350,8 +347,7 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
static void spawn_cleanup(libxl__gc *gc, libxl__spawn_state *ss)
{
assert(!libxl__ev_child_inuse(&ss->mid));
- libxl__ev_time_deregister(gc, &ss->timeout);
- libxl__ev_xswatch_deregister(gc, &ss->xswatch);
+ libxl__xswait_stop(gc, &ss->xswait);
}
static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
@@ -362,8 +358,7 @@ static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
int r;
assert(libxl__ev_child_inuse(&ss->mid));
- libxl__ev_time_deregister(gc, &ss->timeout);
- libxl__ev_xswatch_deregister(gc, &ss->xswatch);
+ libxl__xswait_stop(gc, &ss->xswait);
pid_t child = ss->mid.pid;
r = kill(child, SIGKILL);
@@ -387,25 +382,15 @@ static void spawn_fail(libxl__egc *egc, libxl__spawn_state *ss)
spawn_detach(gc, ss);
}
-static void spawn_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
-{
- /* Before event, was Attached. */
- EGC_GC;
- libxl__spawn_state *ss = CONTAINER_OF(ev, *ss, timeout);
- LOG(ERROR, "%s: startup timed out", ss->what);
- spawn_fail(egc, ss); /* must be last */
-}
-
-static void spawn_watch_event(libxl__egc *egc, libxl__ev_xswatch *xsw,
- const char *watch_path, const char *event_path)
+static void spawn_watch_event(libxl__egc *egc, libxl__xswait_state *xswa,
+ int rc, const char *p)
{
/* On entry, is Attached. */
EGC_GC;
- libxl__spawn_state *ss = CONTAINER_OF(xsw, *ss, xswatch);
- char *p = libxl__xs_read(gc, 0, ss->xspath);
- if (!p && errno != ENOENT) {
- LOG(ERROR, "%s: xenstore read of %s failed", ss->what, ss->xspath);
+ libxl__spawn_state *ss = CONTAINER_OF(xswa, *ss, xswait);
+ if (rc) {
+ if (rc == ERROR_TIMEDOUT)
+ LOG(ERROR, "%s: startup timed out", ss->what);
spawn_fail(egc, ss); /* must be last */
return;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 644fcee..b4e4e0f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1273,8 +1273,7 @@ struct libxl__spawn_state {
int detaching; /* we are in Detaching */
int failed; /* might be true whenever we are not Idle */
libxl__ev_child mid; /* always in use whenever we are not Idle */
- libxl__ev_time timeout;
- libxl__ev_xswatch xswatch;
+ libxl__xswait_state xswait;
};
static inline int libxl__spawn_inuse(const libxl__spawn_state *ss)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code
2014-03-04 14:56 ` [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code Ian Jackson
@ 2014-03-10 3:35 ` Lai Jiangshan
2014-03-10 10:26 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Lai Jiangshan @ 2014-03-10 3:35 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Ian Campbell, Stefano Stabellini
On 03/04/2014 10:56 PM, Ian Jackson wrote:
> Replace open-coded use of ev_time and ev_xswatch with xswait.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_exec.c | 49 +++++++++++++++---------------------------
> tools/libxl/libxl_internal.h | 3 +--
> 2 files changed, 18 insertions(+), 34 deletions(-)
>
> diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
> index b6efa0f..4b012dc 100644
> --- a/tools/libxl/libxl_exec.c
> +++ b/tools/libxl/libxl_exec.c
> @@ -257,10 +257,8 @@ err:
> */
>
> /* Event callbacks. */
> -static void spawn_watch_event(libxl__egc *egc, libxl__ev_xswatch *xsw,
> - const char *watch_path, const char *event_path);
> -static void spawn_timeout(libxl__egc *egc, libxl__ev_time *ev,
> - const struct timeval *requested_abs);
> +static void spawn_watch_event(libxl__egc *egc, libxl__xswait_state *xswa,
> + int rc, const char *xsdata);
> static void spawn_middle_death(libxl__egc *egc, libxl__ev_child *childw,
> pid_t pid, int status);
>
> @@ -274,8 +272,7 @@ static void spawn_fail(libxl__egc *egc, libxl__spawn_state *ss);
> void libxl__spawn_init(libxl__spawn_state *ss)
> {
> libxl__ev_child_init(&ss->mid);
> - libxl__ev_time_init(&ss->timeout);
> - libxl__ev_xswatch_init(&ss->xswatch);
> + libxl__xswait_init(&ss->xswait);
> }
>
> int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
> @@ -288,12 +285,12 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
> libxl__spawn_init(ss);
> ss->failed = ss->detaching = 0;
>
> - rc = libxl__ev_time_register_rel(gc, &ss->timeout,
> - spawn_timeout, ss->timeout_ms);
> - if (rc) goto out_err;
> -
> - rc = libxl__ev_xswatch_register(gc, &ss->xswatch,
> - spawn_watch_event, ss->xspath);
> + ss->xswait.ao = ao;
> + ss->xswait.what = GCSPRINTF("%s startup", ss->what);
> + ss->xswait.path = ss->xspath;
> + ss->xswait.timeout_ms = ss->timeout_ms;
> + ss->xswait.callback = spawn_watch_event;
> + rc = libxl__xswait_start(gc, &ss->xswait);
This is another kind of open code, I guess it should be wrapped into a function.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code
2014-03-10 3:35 ` Lai Jiangshan
@ 2014-03-10 10:26 ` Ian Jackson
2014-03-13 16:33 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-10 10:26 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Shriram Rajagopalan, xen-devel, Ian Campbell, Stefano Stabellini
Lai Jiangshan writes ("Re: [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code"):
> On 03/04/2014 10:56 PM, Ian Jackson wrote:
> > Replace open-coded use of ev_time and ev_xswatch with xswait.
...
> > + ss->xswait.ao = ao;
> > + ss->xswait.what = GCSPRINTF("%s startup", ss->what);
> > + ss->xswait.path = ss->xspath;
> > + ss->xswait.timeout_ms = ss->timeout_ms;
> > + ss->xswait.callback = spawn_watch_event;
> > + rc = libxl__xswait_start(gc, &ss->xswait);
>
> This is another kind of open code, I guess it should be wrapped into
> a function.
I'm not sure I follow. These are, effectively, the arguments to
libxl__xswait_start. libxl__xswait_start takes its "arguments" inside
the xswait structure. So the LHS are formulaic but the RHS are all
specific to the call site.
I guess we could use GCC's compound literal syntax, with something
like this:
ss->xswait = (libxl__xswait_state){
.ao = ao,
.what = GCSPRINTF("%s startup", ss->what),
.path = ss->xspath,
.timeout_ms = ss->timeout_ms,
.callback = spawn_watch_event,
};
But that's not an improvement IMO.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code
2014-03-10 10:26 ` Ian Jackson
@ 2014-03-13 16:33 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 16:33 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Mon, 2014-03-10 at 10:26 +0000, Ian Jackson wrote:
> Lai Jiangshan writes ("Re: [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code"):
> > On 03/04/2014 10:56 PM, Ian Jackson wrote:
> > > Replace open-coded use of ev_time and ev_xswatch with xswait.
> ...
> > > + ss->xswait.ao = ao;
> > > + ss->xswait.what = GCSPRINTF("%s startup", ss->what);
> > > + ss->xswait.path = ss->xspath;
> > > + ss->xswait.timeout_ms = ss->timeout_ms;
> > > + ss->xswait.callback = spawn_watch_event;
> > > + rc = libxl__xswait_start(gc, &ss->xswait);
> >
> > This is another kind of open code, I guess it should be wrapped into
> > a function.
>
> I'm not sure I follow. These are, effectively, the arguments to
> libxl__xswait_start. libxl__xswait_start takes its "arguments" inside
> the xswait structure. So the LHS are formulaic but the RHS are all
> specific to the call site.
>
> I guess we could use GCC's compound literal syntax, with something
> like this:
>
> ss->xswait = (libxl__xswait_state){
> .ao = ao,
> .what = GCSPRINTF("%s startup", ss->what),
> .path = ss->xspath,
> .timeout_ms = ss->timeout_ms,
> .callback = spawn_watch_event,
> };
>
> But that's not an improvement IMO.
Nor mine.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 06/19] libxl: events: Provide libxl__ev_evtchn*
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (4 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 05/19] libxl: events: Use libxl__xswait_* in spawn code Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 16:36 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init Ian Jackson
` (13 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
v2: Fix commit message not to refer to libxl_ctx_alloc's gc, done earlier
Change type of port in evtchn_fd_callback to evtchn_port_or_error_t
Clarify comment about use of ctx->xce.
Fix typo in comment.
---
tools/libxl/libxl.c | 9 +++
tools/libxl/libxl_event.c | 153 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 48 +++++++++++++
3 files changed, 210 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5415ebd..f91a398 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -60,6 +60,10 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
LIBXL_SLIST_INIT(&ctx->watch_freeslots);
libxl__ev_fd_init(&ctx->watch_efd);
+ ctx->xce = 0;
+ LIBXL_LIST_INIT(&ctx->evtchns_waiting);
+ libxl__ev_fd_init(&ctx->evtchn_efd);
+
LIBXL_TAILQ_INIT(&ctx->death_list);
libxl__ev_xswatch_init(&ctx->death_watch);
@@ -104,6 +108,8 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
rc = ERROR_FAIL; goto out;
}
+ rc = libxl__ctx_evtchn_init(gc);
+
*pctx = ctx;
return 0;
@@ -147,16 +153,19 @@ int libxl_ctx_free(libxl_ctx *ctx)
for (i = 0; i < ctx->watch_nslots; i++)
assert(!libxl__watch_slot_contents(gc, i));
libxl__ev_fd_deregister(gc, &ctx->watch_efd);
+ libxl__ev_fd_deregister(gc, &ctx->evtchn_efd);
libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
/* Now there should be no more events requested from the application: */
assert(LIBXL_LIST_EMPTY(&ctx->efds));
assert(LIBXL_TAILQ_EMPTY(&ctx->etimes));
+ assert(LIBXL_LIST_EMPTY(&ctx->evtchns_waiting));
if (ctx->xch) xc_interface_close(ctx->xch);
libxl_version_info_dispose(&ctx->version_info);
if (ctx->xsh) xs_daemon_close(ctx->xsh);
+ if (ctx->xce) xc_evtchn_close(ctx->xce);
libxl__poller_dispose(&ctx->poller_app);
assert(LIBXL_LIST_EMPTY(&ctx->pollers_event));
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 3e465af..22b1227 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -625,6 +625,159 @@ void libxl__ev_xswatch_deregister(libxl__gc *gc, libxl__ev_xswatch *w)
}
/*
+ * evtchn
+ */
+
+static int evtchn_revents_check(libxl__egc *egc, int revents)
+{
+ EGC_GC;
+
+ if (revents & ~POLLIN) {
+ LOG(ERROR, "unexpected poll event on event channel fd: %x", revents);
+ LIBXL__EVENT_DISASTER(egc,
+ "unexpected poll event on event channel fd", 0, 0);
+ libxl__ev_fd_deregister(gc, &CTX->evtchn_efd);
+ return ERROR_FAIL;
+ }
+
+ assert(revents & POLLIN);
+
+ return 0;
+}
+
+static void evtchn_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
+ int fd, short events, short revents)
+{
+ EGC_GC;
+ libxl__ev_evtchn *evev;
+ int r, rc;
+ evtchn_port_or_error_t port;
+ struct pollfd recheck;
+
+ rc = evtchn_revents_check(egc, revents);
+ if (rc) return;
+
+ for (;;) {
+ /* Check the fd again. The incoming revent may no longer be
+ * true, because the libxl ctx lock has not necessarily been
+ * held continuously since someone noticed the fd. Normally
+ * this wouldn't be a problem but evtchn devices don't always
+ * honour O_NONBLOCK (see xenctrl.h). */
+
+ recheck.fd = fd;
+ recheck.events = POLLIN;
+ recheck.revents = 0;
+ r = poll(&recheck, 1, 0);
+ DBG("ev_evtchn recheck r=%d revents=%#x", r, recheck.revents);
+ if (r < 0) {
+ LIBXL__EVENT_DISASTER(egc,
+ "unexpected failure polling event channel fd for recheck",
+ errno, 0);
+ return;
+ }
+ if (r == 0)
+ break;
+ rc = evtchn_revents_check(egc, recheck.revents);
+ if (rc) return;
+
+ /* OK, that's that workaround done. We can actually check for
+ * work for us to do: */
+
+ port = xc_evtchn_pending(CTX->xce);
+ if (port < 0) {
+ if (errno == EAGAIN)
+ break;
+ LIBXL__EVENT_DISASTER(egc,
+ "unexpected failure fetching occurring event port number from evtchn",
+ errno, 0);
+ return;
+ }
+
+ LIBXL_LIST_FOREACH(evev, &CTX->evtchns_waiting, entry)
+ if (port == evev->port)
+ goto found;
+ /* not found */
+ DBG("ev_evtchn port=%d no-one cared", port);
+ continue;
+
+ found:
+ DBG("ev_evtchn=%p port=%d signaled", evev, port);
+ evev->waiting = 0;
+ LIBXL_LIST_REMOVE(evev, entry);
+ evev->callback(egc, evev);
+ }
+}
+
+int libxl__ctx_evtchn_init(libxl__gc *gc) {
+ xc_evtchn *xce;
+ int rc, fd;
+
+ if (CTX->xce)
+ return 0;
+
+ xce = xc_evtchn_open(CTX->lg, 0);
+ if (!xce) {
+ LOGE(ERROR,"cannot open libxc evtchn handle");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ fd = xc_evtchn_fd(xce);
+ assert(fd >= 0);
+
+ rc = libxl_fd_set_nonblock(CTX, fd, 1);
+ if (rc) goto out;
+
+ rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd,
+ evtchn_fd_callback, fd, POLLIN);
+ if (rc) goto out;
+
+ CTX->xce = xce;
+ return 0;
+
+ out:
+ xc_evtchn_close(xce);
+ return rc;
+}
+
+int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev)
+{
+ int r, rc;
+
+ DBG("ev_evtchn=%p port=%d wait (was waiting=%d)",
+ evev, evev->port, evev->waiting);
+
+ if (evev->waiting)
+ return 0;
+
+ r = xc_evtchn_unmask(CTX->xce, evev->port);
+ if (r) {
+ LOGE(ERROR,"cannot unmask event channel %d",evev->port);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ evev->waiting = 1;
+ LIBXL_LIST_INSERT_HEAD(&CTX->evtchns_waiting, evev, entry);
+ return 0;
+
+ out:
+ return rc;
+}
+
+void libxl__ev_evtchn_cancel(libxl__gc *gc, libxl__ev_evtchn *evev)
+{
+ DBG("ev_evtchn=%p port=%d cancel (was waiting=%d)",
+ evev, evev->port, evev->waiting);
+
+ if (!evev->waiting)
+ return;
+
+ evev->waiting = 0;
+ LIBXL_LIST_REMOVE(evev, entry);
+}
+
+/*
* waiting for device state
*/
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b4e4e0f..979b266 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -197,6 +197,17 @@ struct libxl__ev_xswatch {
uint32_t counterval;
};
+typedef struct libxl__ev_evtchn libxl__ev_evtchn;
+typedef void libxl__ev_evtchn_callback(libxl__egc *egc, libxl__ev_evtchn*);
+struct libxl__ev_evtchn {
+ /* caller must fill these in, and they must all remain valid */
+ libxl__ev_evtchn_callback *callback;
+ int port;
+ /* remainder is private for libxl__ev_evtchn_... */
+ bool waiting;
+ LIBXL_LIST_ENTRY(libxl__ev_evtchn) entry;
+};
+
/*
* An entry in the watch_slots table is either:
* 1. an entry in the free list, ie NULL or pointer to next free list entry
@@ -343,6 +354,10 @@ struct libxl__ctx {
uint32_t watch_counter; /* helps disambiguate slot reuse */
libxl__ev_fd watch_efd;
+ xc_evtchn *xce; /* waiting must be done only with libxl__ev_evtchn* */
+ LIBXL_LIST_HEAD(, libxl__ev_evtchn) evtchns_waiting;
+ libxl__ev_fd evtchn_efd;
+
LIBXL_TAILQ_HEAD(libxl__evgen_domain_death_list, libxl_evgen_domain_death)
death_list /* sorted by domid */,
death_reported;
@@ -759,6 +774,39 @@ static inline int libxl__ev_xswatch_isregistered(const libxl__ev_xswatch *xw)
/*
+ * The evtchn facility is one-shot per call to libxl__ev_evtchn_wait.
+ * You should call some suitable xc bind function on (or to obtain)
+ * the port, then libxl__ev_evtchn_wait.
+ *
+ * When the event is signaled then the callback will be made, once.
+ * Then you must call libxl__ev_evtchn_wait again, if desired.
+ *
+ * You must NOT call xc_evtchn_unmask. wait will do that for you.
+ *
+ * Calling libxl__ev_evtchn_cancel will arrange for libxl to disregard
+ * future occurrences of event. Both libxl__ev_evtchn_wait and
+ * libxl__ev_evtchn_cancel are idempotent.
+ *
+ * (Note of course that an event channel becomes signaled when it is
+ * first bound, so you will get one call to libxl__ev_evtchn_wait
+ * "right away"; unless you have won a very fast race, the condition
+ * you were waiting for won't exist yet so when you check for it
+ * you'll find you need to call wait again.)
+ *
+ * You must not wait on the same port twice at once (that is, with
+ * two separate libxl__ev_evtchn's).
+ */
+_hidden int libxl__ev_evtchn_wait(libxl__gc*, libxl__ev_evtchn *evev);
+_hidden void libxl__ev_evtchn_cancel(libxl__gc *gc, libxl__ev_evtchn *evev);
+
+static inline void libxl__ev_evtchn_init(libxl__ev_evtchn *evev)
+ { evev->waiting = 0; }
+static inline bool libxl__ev_evtchn_iswaiting(const libxl__ev_evtchn *evev)
+ { return evev->waiting; }
+
+_hidden int libxl__ctx_evtchn_init(libxl__gc *gc); /* for libxl_ctx_alloc */
+
+/*
* For making subprocesses. This is the only permitted mechanism for
* code in libxl to do so.
*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (5 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 06/19] libxl: events: Provide libxl__ev_evtchn* Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-04 15:10 ` Andrew Cooper
2014-03-13 16:38 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 08/19] libxc: suspend: Fix suspend event channel locking Ian Jackson
` (12 subsequent siblings)
19 siblings, 2 replies; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
xc_suspend_evtchn_init expects to eat the first event on the xce. If
the xce is used for any other purpose then this can break. Document
this fact and rename the function to xc_suspend_evtchn_init_exclusive.
(I haven't checked the call sites for improper shared use of the xce.)
Provide a corresponding xc_suspend_evtchn_init_sane which doesn't try
to eat an event, and instead leaves the caller the ability to
demultiplex.
Also document that xc_await_suspend needs exclusive use of the xce.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
tools/libxc/xc_suspend.c | 21 ++++++++++++++++----
tools/libxc/xenguest.h | 13 +++++++++++-
tools/libxl/libxl_dom.c | 2 +-
tools/misc/xen-hptool.c | 2 +-
.../python/xen/lowlevel/checkpoint/libcheckpoint.c | 2 +-
tools/xcutils/xc_save.c | 2 +-
6 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
index 1ace411..7968a44 100644
--- a/tools/libxc/xc_suspend.c
+++ b/tools/libxc/xc_suspend.c
@@ -14,6 +14,8 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*/
+#include <assert.h>
+
#include "xc_private.h"
#include "xenguest.h"
@@ -102,7 +104,7 @@ int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int
return unlock_suspend_event(xch, domid);
}
-int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port)
+int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port)
{
int rc, suspend_evtchn = -1;
@@ -121,9 +123,6 @@ int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int por
goto cleanup;
}
- /* event channel is pending immediately after binding */
- xc_await_suspend(xch, xce, suspend_evtchn);
-
return suspend_evtchn;
cleanup:
@@ -132,3 +131,17 @@ cleanup:
return -1;
}
+
+int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port)
+{
+ int suspend_evtchn;
+
+ suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port);
+ if (suspend_evtchn < 0)
+ return suspend_evtchn;
+
+ /* event channel is pending immediately after binding */
+ xc_await_suspend(xch, xce, suspend_evtchn);
+
+ return suspend_evtchn;
+}
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
index a0e30e1..ce5456c 100644
--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -256,10 +256,21 @@ int xc_hvm_build_target_mem(xc_interface *xch,
int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn);
-int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+/**
+ * This function eats the initial notification.
+ * xce must not be used for anything else
+ */
+int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+/* xce must not be used for anything else */
int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
+/**
+ * The port will be signaled immediately after this call
+ * The caller should check the domain status and look for the next event
+ */
+int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+
int xc_get_bit_size(xc_interface *xch,
const char *image_name, const char *cmdline,
const char *features, int *type);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 55f74b2..4b42856 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1358,7 +1358,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
if (port >= 0) {
dss->suspend_eventchn =
- xc_suspend_evtchn_init(CTX->xch, dss->xce, dss->domid, port);
+ xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce, dss->domid, port);
if (dss->suspend_eventchn < 0)
LOG(WARN, "Suspend event channel initialization failed");
diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
index 24c3e95..db76f79 100644
--- a/tools/misc/xen-hptool.c
+++ b/tools/misc/xen-hptool.c
@@ -112,7 +112,7 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
fprintf(stderr, "DOM%d: No suspend port, try live migration\n", domid);
goto failed;
}
- suspend_evtchn = xc_suspend_evtchn_init(xch, xce, domid, port);
+ suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid, port);
if (suspend_evtchn < 0)
{
fprintf(stderr, "Suspend evtchn initialization failed\n");
diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
index 01c0d47..817d272 100644
--- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
+++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
@@ -360,7 +360,7 @@ static int setup_suspend_evtchn(checkpoint_state* s)
return -1;
}
- s->suspend_evtchn = xc_suspend_evtchn_init(s->xch, s->xce, s->domid, port);
+ s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce, s->domid, port);
if (s->suspend_evtchn < 0) {
s->errstr = "failed to bind suspend event channel";
return -1;
diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
index e34bd2c..974f706 100644
--- a/tools/xcutils/xc_save.c
+++ b/tools/xcutils/xc_save.c
@@ -202,7 +202,7 @@ main(int argc, char **argv)
else
{
si.suspend_evtchn =
- xc_suspend_evtchn_init(si.xch, si.xce, si.domid, port);
+ xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid, port);
if (si.suspend_evtchn < 0)
warnx("suspend event channel initialization failed, "
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init
2014-03-04 14:56 ` [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init Ian Jackson
@ 2014-03-04 15:10 ` Andrew Cooper
2014-03-04 15:30 ` Ian Jackson
2014-03-13 16:38 ` Ian Campbell
1 sibling, 1 reply; 70+ messages in thread
From: Andrew Cooper @ 2014-03-04 15:10 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, Lai Jiangshan, xen-devel, Ian Campbell,
Stefano Stabellini
On 04/03/14 14:56, Ian Jackson wrote:
> xc_suspend_evtchn_init expects to eat the first event on the xce. If
> the xce is used for any other purpose then this can break. Document
> this fact and rename the function to xc_suspend_evtchn_init_exclusive.
> (I haven't checked the call sites for improper shared use of the xce.)
>
> Provide a corresponding xc_suspend_evtchn_init_sane which doesn't try
> to eat an event, and instead leaves the caller the ability to
> demultiplex.
>
> Also document that xc_await_suspend needs exclusive use of the xce.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
> tools/libxc/xc_suspend.c | 21 ++++++++++++++++----
> tools/libxc/xenguest.h | 13 +++++++++++-
> tools/libxl/libxl_dom.c | 2 +-
> tools/misc/xen-hptool.c | 2 +-
> .../python/xen/lowlevel/checkpoint/libcheckpoint.c | 2 +-
> tools/xcutils/xc_save.c | 2 +-
> 6 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
> index 1ace411..7968a44 100644
> --- a/tools/libxc/xc_suspend.c
> +++ b/tools/libxc/xc_suspend.c
> @@ -14,6 +14,8 @@
> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> */
>
> +#include <assert.h>
> +
This appears unused - is it debugging which slipped in?
~Andrew
> #include "xc_private.h"
> #include "xenguest.h"
>
> @@ -102,7 +104,7 @@ int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int
> return unlock_suspend_event(xch, domid);
> }
>
> -int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port)
> +int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port)
> {
> int rc, suspend_evtchn = -1;
>
> @@ -121,9 +123,6 @@ int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int por
> goto cleanup;
> }
>
> - /* event channel is pending immediately after binding */
> - xc_await_suspend(xch, xce, suspend_evtchn);
> -
> return suspend_evtchn;
>
> cleanup:
> @@ -132,3 +131,17 @@ cleanup:
>
> return -1;
> }
> +
> +int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port)
> +{
> + int suspend_evtchn;
> +
> + suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port);
> + if (suspend_evtchn < 0)
> + return suspend_evtchn;
> +
> + /* event channel is pending immediately after binding */
> + xc_await_suspend(xch, xce, suspend_evtchn);
> +
> + return suspend_evtchn;
> +}
> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
> index a0e30e1..ce5456c 100644
> --- a/tools/libxc/xenguest.h
> +++ b/tools/libxc/xenguest.h
> @@ -256,10 +256,21 @@ int xc_hvm_build_target_mem(xc_interface *xch,
>
> int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn);
>
> -int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port);
> +/**
> + * This function eats the initial notification.
> + * xce must not be used for anything else
> + */
> +int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port);
>
> +/* xce must not be used for anything else */
> int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
>
> +/**
> + * The port will be signaled immediately after this call
> + * The caller should check the domain status and look for the next event
> + */
> +int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port);
> +
> int xc_get_bit_size(xc_interface *xch,
> const char *image_name, const char *cmdline,
> const char *features, int *type);
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 55f74b2..4b42856 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1358,7 +1358,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
>
> if (port >= 0) {
> dss->suspend_eventchn =
> - xc_suspend_evtchn_init(CTX->xch, dss->xce, dss->domid, port);
> + xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce, dss->domid, port);
>
> if (dss->suspend_eventchn < 0)
> LOG(WARN, "Suspend event channel initialization failed");
> diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
> index 24c3e95..db76f79 100644
> --- a/tools/misc/xen-hptool.c
> +++ b/tools/misc/xen-hptool.c
> @@ -112,7 +112,7 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
> fprintf(stderr, "DOM%d: No suspend port, try live migration\n", domid);
> goto failed;
> }
> - suspend_evtchn = xc_suspend_evtchn_init(xch, xce, domid, port);
> + suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid, port);
> if (suspend_evtchn < 0)
> {
> fprintf(stderr, "Suspend evtchn initialization failed\n");
> diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> index 01c0d47..817d272 100644
> --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> @@ -360,7 +360,7 @@ static int setup_suspend_evtchn(checkpoint_state* s)
> return -1;
> }
>
> - s->suspend_evtchn = xc_suspend_evtchn_init(s->xch, s->xce, s->domid, port);
> + s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce, s->domid, port);
> if (s->suspend_evtchn < 0) {
> s->errstr = "failed to bind suspend event channel";
> return -1;
> diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
> index e34bd2c..974f706 100644
> --- a/tools/xcutils/xc_save.c
> +++ b/tools/xcutils/xc_save.c
> @@ -202,7 +202,7 @@ main(int argc, char **argv)
> else
> {
> si.suspend_evtchn =
> - xc_suspend_evtchn_init(si.xch, si.xce, si.domid, port);
> + xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid, port);
>
> if (si.suspend_evtchn < 0)
> warnx("suspend event channel initialization failed, "
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init
2014-03-04 15:10 ` Andrew Cooper
@ 2014-03-04 15:30 ` Ian Jackson
0 siblings, 0 replies; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 15:30 UTC (permalink / raw)
To: Andrew Cooper
Cc: Shriram Rajagopalan, Lai Jiangshan, xen-devel, Ian Campbell,
Stefano Stabellini
Andrew Cooper writes ("Re: [Xen-devel] [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init"):
> On 04/03/14 14:56, Ian Jackson wrote:
> > +#include <assert.h>
>
> This appears unused - is it debugging which slipped in?
I think so. I'll drop this hunk.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init
2014-03-04 14:56 ` [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init Ian Jackson
2014-03-04 15:10 ` Andrew Cooper
@ 2014-03-13 16:38 ` Ian Campbell
1 sibling, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 16:38 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> xc_suspend_evtchn_init expects to eat the first event on the xce. If
> the xce is used for any other purpose then this can break. Document
> this fact and rename the function to xc_suspend_evtchn_init_exclusive.
> (I haven't checked the call sites for improper shared use of the xce.)
>
> Provide a corresponding xc_suspend_evtchn_init_sane which doesn't try
> to eat an event, and instead leaves the caller the ability to
> demultiplex.
>
> Also document that xc_await_suspend needs exclusive use of the xce.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(just to clarify: I acked this on v1 a few minutes ago before I realised
there was this resend)
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 08/19] libxc: suspend: Fix suspend event channel locking
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (6 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 16:47 ` Ian Campbell
2014-03-16 4:53 ` Shriram Rajagopalan
2014-03-04 14:56 ` [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback Ian Jackson
` (11 subsequent siblings)
19 siblings, 2 replies; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Use fcntl F_SETLK, rather than writing our pid into a "lock" file.
That way if we crash we don't leave the lockfile lying about. Callers
now need to keep the fd for our lockfile. (We don't use flock because
we don't want anyone who inherits this fd across fork to end up with a
handle onto the lock.)
While we are here:
* Move the lockfile to /var/run/xen
* De-duplicate the calculation of the pathname
* Compute the buffer size for the pathname so that it will definitely
not overrun (and use the computed value everywhere)
* Fix various error handling bugs
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
tools/libxc/xc_suspend.c | 159 +++++++++++++-------
tools/libxc/xenguest.h | 16 +-
tools/libxl/libxl_dom.c | 6 +-
tools/libxl/libxl_internal.h | 1 +
tools/misc/xen-hptool.c | 19 ++-
tools/python/xen/lowlevel/checkpoint/checkpoint.h | 2 +-
.../python/xen/lowlevel/checkpoint/libcheckpoint.c | 7 +-
tools/xcutils/xc_save.c | 8 +-
8 files changed, 149 insertions(+), 69 deletions(-)
diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
index 7968a44..f0f70c1 100644
--- a/tools/libxc/xc_suspend.c
+++ b/tools/libxc/xc_suspend.c
@@ -15,66 +15,115 @@
*/
#include <assert.h>
+#include <unistd.h>
+#include <fcntl.h>
#include "xc_private.h"
#include "xenguest.h"
-#define SUSPEND_LOCK_FILE "/var/lib/xen/suspend_evtchn"
-static int lock_suspend_event(xc_interface *xch, int domid)
+#define SUSPEND_LOCK_FILE "/var/run/xen/suspend-evtchn-%d.lock"
+
+/*
+ * locking
+ */
+
+#define ERR(x) do{ \
+ ERROR("Can't " #x " lock file for suspend event channel %s: %s\n", \
+ suspend_file, strerror(errno)); \
+ goto err; \
+}while(0)
+
+#define SUSPEND_FILE_BUFLEN (sizeof(SUSPEND_LOCK_FILE) + 10)
+
+static void get_suspend_file(char buf[SUSPEND_FILE_BUFLEN], int domid)
{
- int fd, rc;
- mode_t mask;
- char buf[128];
- char suspend_file[256];
-
- snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
- SUSPEND_LOCK_FILE, domid);
- mask = umask(022);
- fd = open(suspend_file, O_CREAT | O_EXCL | O_RDWR, 0666);
- if (fd < 0)
- {
- ERROR("Can't create lock file for suspend event channel %s\n",
- suspend_file);
- return -EINVAL;
+ snprintf(buf, sizeof(buf), SUSPEND_LOCK_FILE, domid);
+}
+
+static int lock_suspend_event(xc_interface *xch, int domid, int *lockfd)
+{
+ int fd = -1, r;
+ char suspend_file[SUSPEND_FILE_BUFLEN];
+ struct stat ours, theirs;
+ struct flock fl;
+
+ get_suspend_file(suspend_file, domid);
+
+ *lockfd = -1;
+
+ for (;;) {
+ if (fd >= 0)
+ close (fd);
+
+ fd = open(suspend_file, O_CREAT | O_RDWR, 0600);
+ if (fd < 0)
+ ERR("create");
+
+ r = fcntl(fd, F_SETFD, FD_CLOEXEC);
+ if (r)
+ ERR("fcntl F_SETFD FD_CLOEXEC");
+
+ memset(&fl, 0, sizeof(fl));
+ fl.l_type = F_WRLCK;
+ fl.l_whence = SEEK_SET;
+ fl.l_len = 1;
+ r = fcntl(fd, F_SETLK, &fl);
+ if (r)
+ ERR("fcntl F_SETLK");
+
+ r = fstat(fd, &ours);
+ if (r)
+ ERR("fstat");
+
+ r = stat(suspend_file, &theirs);
+ if (r) {
+ if (errno == ENOENT)
+ /* try again */
+ continue;
+ ERR("stat");
+ }
+
+ if (ours.st_ino != theirs.st_ino)
+ /* someone else must have removed it while we were locking it */
+ continue;
+
+ break;
}
- umask(mask);
- snprintf(buf, sizeof(buf), "%10ld", (long)getpid());
- rc = write_exact(fd, buf, strlen(buf));
- close(fd);
+ *lockfd = fd;
+ return 0;
- return rc;
+ err:
+ if (fd >= 0)
+ close(fd);
+
+ return -1;
}
-static int unlock_suspend_event(xc_interface *xch, int domid)
+static int unlock_suspend_event(xc_interface *xch, int domid, int *lockfd)
{
- int fd, pid, n;
- char buf[128];
- char suspend_file[256];
+ int r;
+ char suspend_file[SUSPEND_FILE_BUFLEN];
- snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
- SUSPEND_LOCK_FILE, domid);
- fd = open(suspend_file, O_RDWR);
+ if (*lockfd < 0)
+ return 0;
- if (fd < 0)
- return -EINVAL;
+ get_suspend_file(suspend_file, domid);
- n = read(fd, buf, 127);
+ r = unlink(suspend_file);
+ if (r)
+ ERR("unlink");
- close(fd);
+ r = close(*lockfd);
+ *lockfd = -1;
+ if (r)
+ ERR("close");
- if (n > 0)
- {
- sscanf(buf, "%d", &pid);
- /* We are the owner, so we can simply delete the file */
- if (pid == getpid())
- {
- unlink(suspend_file);
- return 0;
- }
- }
+ err:
+ if (*lockfd >= 0)
+ close(*lockfd);
- return -EPERM;
+ return -1;
}
int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn)
@@ -96,20 +145,26 @@ int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn)
return 0;
}
-int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn)
+/* Internal callers are allowed to call this with suspend_evtchn<0
+ * but *lockfd>0. */
+int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce,
+ int domid, int suspend_evtchn, int *lockfd)
{
if (suspend_evtchn >= 0)
xc_evtchn_unbind(xce, suspend_evtchn);
- return unlock_suspend_event(xch, domid);
+ return unlock_suspend_event(xch, domid, lockfd);
}
-int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port)
+int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce,
+ int domid, int port, int *lockfd)
{
int rc, suspend_evtchn = -1;
- if (lock_suspend_event(xch, domid))
- return -EINVAL;
+ if (lock_suspend_event(xch, domid, lockfd)) {
+ errno = EINVAL;
+ goto cleanup;
+ }
suspend_evtchn = xc_evtchn_bind_interdomain(xce, domid, port);
if (suspend_evtchn < 0) {
@@ -126,17 +181,17 @@ int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, in
return suspend_evtchn;
cleanup:
- if (suspend_evtchn != -1)
- xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
+ xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn, lockfd);
return -1;
}
-int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port)
+int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce,
+ int domid, int port, int *lockfd)
{
int suspend_evtchn;
- suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port);
+ suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port, lockfd);
if (suspend_evtchn < 0)
return suspend_evtchn;
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
index ce5456c..1f216cd 100644
--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -254,13 +254,19 @@ int xc_hvm_build_target_mem(xc_interface *xch,
int target,
const char *image_name);
-int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn);
+/*
+ * Sets *lockfd to -1.
+ * Has deallocated everything even on error.
+ */
+int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn, int *lockfd);
/**
* This function eats the initial notification.
* xce must not be used for anything else
+ * See xc_suspend_evtchn_init_sane re lockfd.
*/
-int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce,
+ int domid, int port, int *lockfd);
/* xce must not be used for anything else */
int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
@@ -268,8 +274,12 @@ int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
/**
* The port will be signaled immediately after this call
* The caller should check the domain status and look for the next event
+ * On success, *lockfd will be set to >=0 and *lockfd must be preserved
+ * and fed to xc_suspend_evtchn_release. (On error *lockfd is
+ * undefined and xc_suspend_evtchn_release is not allowed.)
*/
-int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port);
+int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce,
+ int domid, int port, int *lockfd);
int xc_get_bit_size(xc_interface *xch,
const char *image_name, const char *cmdline,
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 4b42856..48a4b8e 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1340,6 +1340,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
| (dss->hvm ? XCFLAGS_HVM : 0);
dss->suspend_eventchn = -1;
+ dss->guest_evtchn_lockfd = -1;
dss->guest_responded = 0;
dss->dm_savefile = libxl__device_model_savefile(gc, domid);
@@ -1358,7 +1359,8 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
if (port >= 0) {
dss->suspend_eventchn =
- xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce, dss->domid, port);
+ xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce,
+ dss->domid, port, &dss->guest_evtchn_lockfd);
if (dss->suspend_eventchn < 0)
LOG(WARN, "Suspend event channel initialization failed");
@@ -1522,7 +1524,7 @@ static void domain_suspend_done(libxl__egc *egc,
if (dss->suspend_eventchn > 0)
xc_suspend_evtchn_release(CTX->xch, dss->xce, domid,
- dss->suspend_eventchn);
+ dss->suspend_eventchn, &dss->guest_evtchn_lockfd);
if (dss->xce != NULL)
xc_evtchn_close(dss->xce);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 979b266..ebbfa09 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2417,6 +2417,7 @@ struct libxl__domain_suspend_state {
/* private */
xc_evtchn *xce; /* event channel handle */
int suspend_eventchn;
+ int guest_evtchn_lockfd;
int hvm;
int xcflags;
int guest_responded;
diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
index db76f79..bd9d936 100644
--- a/tools/misc/xen-hptool.c
+++ b/tools/misc/xen-hptool.c
@@ -99,10 +99,13 @@ static int hp_mem_query_func(int argc, char *argv[])
extern int xs_suspend_evtchn_port(int domid);
-static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtchn)
+static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid,
+ int *evtchn, int *lockfd)
{
int port, rc, suspend_evtchn = -1;
+ *lockfd = -1;
+
if (!evtchn)
return -1;
@@ -112,7 +115,8 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
fprintf(stderr, "DOM%d: No suspend port, try live migration\n", domid);
goto failed;
}
- suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid, port);
+ suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid,
+ port, lockfd);
if (suspend_evtchn < 0)
{
fprintf(stderr, "Suspend evtchn initialization failed\n");
@@ -135,7 +139,8 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
failed:
if (suspend_evtchn != -1)
- xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
+ xc_suspend_evtchn_release(xch, xce, domid,
+ suspend_evtchn, lockfd);
return -1;
}
@@ -193,7 +198,7 @@ static int hp_mem_offline_func(int argc, char *argv[])
}
else if (status & PG_OFFLINE_OWNED)
{
- int result, suspend_evtchn = -1;
+ int result, suspend_evtchn = -1, suspend_lockfd = -1;
xc_evtchn *xce;
xce = xc_evtchn_open(NULL, 0);
@@ -205,7 +210,8 @@ static int hp_mem_offline_func(int argc, char *argv[])
}
domid = status >> PG_OFFLINE_OWNER_SHIFT;
- if (suspend_guest(xch, xce, domid, &suspend_evtchn))
+ if (suspend_guest(xch, xce, domid,
+ &suspend_evtchn, &suspend_lockfd))
{
fprintf(stderr, "Failed to suspend guest %d for"
" mfn %lx\n", domid, mfn);
@@ -231,7 +237,8 @@ static int hp_mem_offline_func(int argc, char *argv[])
mfn, domid);
}
xc_domain_resume(xch, domid, 1);
- xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
+ xc_suspend_evtchn_release(xch, xce, domid,
+ suspend_evtchn, &suspend_lockfd);
xc_evtchn_close(xce);
}
break;
diff --git a/tools/python/xen/lowlevel/checkpoint/checkpoint.h b/tools/python/xen/lowlevel/checkpoint/checkpoint.h
index 187d9d7..2414956 100644
--- a/tools/python/xen/lowlevel/checkpoint/checkpoint.h
+++ b/tools/python/xen/lowlevel/checkpoint/checkpoint.h
@@ -27,7 +27,7 @@ typedef struct {
checkpoint_domtype domtype;
int fd;
- int suspend_evtchn;
+ int suspend_evtchn, suspend_lockfd;
char* errstr;
diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
index 817d272..74ca062 100644
--- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
+++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
@@ -57,6 +57,7 @@ void checkpoint_init(checkpoint_state* s)
s->fd = -1;
s->suspend_evtchn = -1;
+ s->suspend_lockfd = -1;
s->errstr = NULL;
@@ -360,7 +361,8 @@ static int setup_suspend_evtchn(checkpoint_state* s)
return -1;
}
- s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce, s->domid, port);
+ s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce,
+ s->domid, port, &s->suspend_lockfd);
if (s->suspend_evtchn < 0) {
s->errstr = "failed to bind suspend event channel";
return -1;
@@ -377,7 +379,8 @@ static void release_suspend_evtchn(checkpoint_state *s)
{
/* TODO: teach xen to clean up if port is unbound */
if (s->xce != NULL && s->suspend_evtchn >= 0) {
- xc_suspend_evtchn_release(s->xch, s->xce, s->domid, s->suspend_evtchn);
+ xc_suspend_evtchn_release(s->xch, s->xce, s->domid,
+ s->suspend_evtchn, &s->suspend_lockfd);
s->suspend_evtchn = -1;
}
}
diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
index 974f706..bf74e46 100644
--- a/tools/xcutils/xc_save.c
+++ b/tools/xcutils/xc_save.c
@@ -167,7 +167,7 @@ int
main(int argc, char **argv)
{
unsigned int maxit, max_f, lflags;
- int io_fd, ret, port;
+ int io_fd, ret, port, suspend_lockfd = -1;
struct save_callbacks callbacks;
xentoollog_level lvl;
xentoollog_logger *l;
@@ -202,7 +202,8 @@ main(int argc, char **argv)
else
{
si.suspend_evtchn =
- xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid, port);
+ xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid,
+ port, &suspend_lockfd);
if (si.suspend_evtchn < 0)
warnx("suspend event channel initialization failed, "
@@ -216,7 +217,8 @@ main(int argc, char **argv)
&callbacks, !!(si.flags & XCFLAGS_HVM), 0);
if (si.suspend_evtchn > 0)
- xc_suspend_evtchn_release(si.xch, si.xce, si.domid, si.suspend_evtchn);
+ xc_suspend_evtchn_release(si.xch, si.xce, si.domid,
+ si.suspend_evtchn, &suspend_lockfd);
if (si.xce > 0)
xc_evtchn_close(si.xce);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking
2014-03-04 14:56 ` [PATCH 08/19] libxc: suspend: Fix suspend event channel locking Ian Jackson
@ 2014-03-13 16:47 ` Ian Campbell
2014-03-13 18:46 ` Ian Jackson
2014-03-16 4:53 ` Shriram Rajagopalan
1 sibling, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 16:47 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Use fcntl F_SETLK, rather than writing our pid into a "lock" file.
> That way if we crash we don't leave the lockfile lying about. Callers
> now need to keep the fd for our lockfile. (We don't use flock because
> we don't want anyone who inherits this fd across fork to end up with a
> handle onto the lock.)
>
> While we are here:
> * Move the lockfile to /var/run/xen
There isn't some autoconf'y path we should use is there? SUBSYS_DIR,
localstatedir etc?
Probably we don't really use those elsewhere so it is consistent to
use /var directly here too.
> * De-duplicate the calculation of the pathname
> * Compute the buffer size for the pathname so that it will definitely
> not overrun (and use the computed value everywhere)
> * Fix various error handling bugs
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
> tools/libxc/xc_suspend.c | 159 +++++++++++++-------
> tools/libxc/xenguest.h | 16 +-
> tools/libxl/libxl_dom.c | 6 +-
> tools/libxl/libxl_internal.h | 1 +
> tools/misc/xen-hptool.c | 19 ++-
> tools/python/xen/lowlevel/checkpoint/checkpoint.h | 2 +-
> .../python/xen/lowlevel/checkpoint/libcheckpoint.c | 7 +-
> tools/xcutils/xc_save.c | 8 +-
> 8 files changed, 149 insertions(+), 69 deletions(-)
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking
2014-03-13 16:47 ` Ian Campbell
@ 2014-03-13 18:46 ` Ian Jackson
2014-03-14 9:55 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-13 18:46 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking"):
> On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > Use fcntl F_SETLK, rather than writing our pid into a "lock" file.
> > That way if we crash we don't leave the lockfile lying about. Callers
> > now need to keep the fd for our lockfile. (We don't use flock because
> > we don't want anyone who inherits this fd across fork to end up with a
> > handle onto the lock.)
> >
> > While we are here:
> > * Move the lockfile to /var/run/xen
>
> There isn't some autoconf'y path we should use is there? SUBSYS_DIR,
> localstatedir etc?
There does seem to be, but I'm not really convinced that it's the
right thing.
> Probably we don't really use those elsewhere so it is consistent to
> use /var directly here too.
I did a grep for uses of /var/run in the whole tree, and of /var in
libxl. libxl has /var/lib and /var/log hardcoded but not /var/run
yet.
But xenstored, xenmon, and many of our scripts, have it hardcoded in
one way or another.
So I think this is OK. Systems which are using /run will have a
symlink for /var/run.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking
2014-03-13 18:46 ` Ian Jackson
@ 2014-03-14 9:55 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 9:55 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Thu, 2014-03-13 at 18:46 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking"):
> > On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > > Use fcntl F_SETLK, rather than writing our pid into a "lock" file.
> > > That way if we crash we don't leave the lockfile lying about. Callers
> > > now need to keep the fd for our lockfile. (We don't use flock because
> > > we don't want anyone who inherits this fd across fork to end up with a
> > > handle onto the lock.)
> > >
> > > While we are here:
> > > * Move the lockfile to /var/run/xen
> >
> > There isn't some autoconf'y path we should use is there? SUBSYS_DIR,
> > localstatedir etc?
>
> There does seem to be, but I'm not really convinced that it's the
> right thing.
>
> > Probably we don't really use those elsewhere so it is consistent to
> > use /var directly here too.
>
> I did a grep for uses of /var/run in the whole tree, and of /var in
> libxl. libxl has /var/lib and /var/log hardcoded but not /var/run
> yet.
>
> But xenstored, xenmon, and many of our scripts, have it hardcoded in
> one way or another.
This rings a bell -- I think when we switch prefix from /usr
to /usr/local we inadvertently ended up moving a load of stuff
to /usr/local/var which was just broken -- so we ended up overriding to
use FHS /var instead of GNU/autoconf $PREFIX/var.
So I think you've done the right thing.
My ack stands.
>
> So I think this is OK. Systems which are using /run will have a
> symlink for /var/run.
>
> Thanks,
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking
2014-03-04 14:56 ` [PATCH 08/19] libxc: suspend: Fix suspend event channel locking Ian Jackson
2014-03-13 16:47 ` Ian Campbell
@ 2014-03-16 4:53 ` Shriram Rajagopalan
2014-03-17 11:35 ` Ian Jackson
1 sibling, 1 reply; 70+ messages in thread
From: Shriram Rajagopalan @ 2014-03-16 4:53 UTC (permalink / raw)
To: Ian Jackson
Cc: xen-devel@lists.xensource.com, Ian Campbell, Stefano Stabellini,
Lai Jiangshan
[-- Attachment #1.1: Type: text/plain, Size: 18382 bytes --]
On Tue, Mar 4, 2014 at 6:56 AM, Ian Jackson <ian.jackson@eu.citrix.com>wrote:
> Use fcntl F_SETLK, rather than writing our pid into a "lock" file.
> That way if we crash we don't leave the lockfile lying about. Callers
> now need to keep the fd for our lockfile. (We don't use flock because
> we don't want anyone who inherits this fd across fork to end up with a
> handle onto the lock.)
>
> While we are here:
> * Move the lockfile to /var/run/xen
> * De-duplicate the calculation of the pathname
> * Compute the buffer size for the pathname so that it will definitely
> not overrun (and use the computed value everywhere)
> * Fix various error handling bugs
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
> tools/libxc/xc_suspend.c | 159
> +++++++++++++-------
> tools/libxc/xenguest.h | 16 +-
> tools/libxl/libxl_dom.c | 6 +-
> tools/libxl/libxl_internal.h | 1 +
> tools/misc/xen-hptool.c | 19 ++-
> tools/python/xen/lowlevel/checkpoint/checkpoint.h | 2 +-
> .../python/xen/lowlevel/checkpoint/libcheckpoint.c | 7 +-
> tools/xcutils/xc_save.c | 8 +-
> 8 files changed, 149 insertions(+), 69 deletions(-)
>
> diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
> index 7968a44..f0f70c1 100644
> --- a/tools/libxc/xc_suspend.c
> +++ b/tools/libxc/xc_suspend.c
> @@ -15,66 +15,115 @@
> */
>
> #include <assert.h>
> +#include <unistd.h>
> +#include <fcntl.h>
>
> #include "xc_private.h"
> #include "xenguest.h"
>
> -#define SUSPEND_LOCK_FILE "/var/lib/xen/suspend_evtchn"
> -static int lock_suspend_event(xc_interface *xch, int domid)
> +#define SUSPEND_LOCK_FILE "/var/run/xen/suspend-evtchn-%d.lock"
> +
> +/*
> + * locking
> + */
> +
> +#define ERR(x) do{ \
> + ERROR("Can't " #x " lock file for suspend event channel %s: %s\n", \
> + suspend_file, strerror(errno)); \
> + goto err; \
> +}while(0)
> +
> +#define SUSPEND_FILE_BUFLEN (sizeof(SUSPEND_LOCK_FILE) + 10)
> +
> +static void get_suspend_file(char buf[SUSPEND_FILE_BUFLEN], int domid)
> {
> - int fd, rc;
> - mode_t mask;
> - char buf[128];
> - char suspend_file[256];
> -
> - snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
> - SUSPEND_LOCK_FILE, domid);
> - mask = umask(022);
> - fd = open(suspend_file, O_CREAT | O_EXCL | O_RDWR, 0666);
> - if (fd < 0)
> - {
> - ERROR("Can't create lock file for suspend event channel %s\n",
> - suspend_file);
> - return -EINVAL;
> + snprintf(buf, sizeof(buf), SUSPEND_LOCK_FILE, domid);
> +}
> +
> +static int lock_suspend_event(xc_interface *xch, int domid, int *lockfd)
> +{
> + int fd = -1, r;
> + char suspend_file[SUSPEND_FILE_BUFLEN];
> + struct stat ours, theirs;
> + struct flock fl;
> +
> + get_suspend_file(suspend_file, domid);
> +
> + *lockfd = -1;
> +
> + for (;;) {
> + if (fd >= 0)
> + close (fd);
> +
> + fd = open(suspend_file, O_CREAT | O_RDWR, 0600);
> + if (fd < 0)
> + ERR("create");
> +
> + r = fcntl(fd, F_SETFD, FD_CLOEXEC);
> + if (r)
> + ERR("fcntl F_SETFD FD_CLOEXEC");
> +
> + memset(&fl, 0, sizeof(fl));
> + fl.l_type = F_WRLCK;
> + fl.l_whence = SEEK_SET;
> + fl.l_len = 1;
> + r = fcntl(fd, F_SETLK, &fl);
> + if (r)
> + ERR("fcntl F_SETLK");
> +
> + r = fstat(fd, &ours);
> + if (r)
> + ERR("fstat");
> +
> + r = stat(suspend_file, &theirs);
> + if (r) {
> + if (errno == ENOENT)
> + /* try again */
> + continue;
> + ERR("stat");
> + }
> +
> + if (ours.st_ino != theirs.st_ino)
> + /* someone else must have removed it while we were locking it
> */
> + continue;
> +
> + break;
> }
> - umask(mask);
> - snprintf(buf, sizeof(buf), "%10ld", (long)getpid());
>
> - rc = write_exact(fd, buf, strlen(buf));
> - close(fd);
> + *lockfd = fd;
> + return 0;
>
> - return rc;
> + err:
> + if (fd >= 0)
> + close(fd);
> +
> + return -1;
> }
>
> -static int unlock_suspend_event(xc_interface *xch, int domid)
> +static int unlock_suspend_event(xc_interface *xch, int domid, int *lockfd)
> {
> - int fd, pid, n;
> - char buf[128];
> - char suspend_file[256];
> + int r;
> + char suspend_file[SUSPEND_FILE_BUFLEN];
>
> - snprintf(suspend_file, sizeof(suspend_file), "%s_%d_lock.d",
> - SUSPEND_LOCK_FILE, domid);
> - fd = open(suspend_file, O_RDWR);
> + if (*lockfd < 0)
> + return 0;
>
> - if (fd < 0)
> - return -EINVAL;
> + get_suspend_file(suspend_file, domid);
>
> - n = read(fd, buf, 127);
> + r = unlink(suspend_file);
> + if (r)
> + ERR("unlink");
>
> - close(fd);
> + r = close(*lockfd);
> + *lockfd = -1;
> + if (r)
> + ERR("close");
>
> - if (n > 0)
> - {
> - sscanf(buf, "%d", &pid);
> - /* We are the owner, so we can simply delete the file */
> - if (pid == getpid())
> - {
> - unlink(suspend_file);
> - return 0;
> - }
> - }
> + err:
> + if (*lockfd >= 0)
> + close(*lockfd);
>
> - return -EPERM;
> + return -1;
> }
>
> int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int
> suspend_evtchn)
> @@ -96,20 +145,26 @@ int xc_await_suspend(xc_interface *xch, xc_evtchn
> *xce, int suspend_evtchn)
> return 0;
> }
>
> -int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int
> domid, int suspend_evtchn)
> +/* Internal callers are allowed to call this with suspend_evtchn<0
> + * but *lockfd>0. */
> +int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce,
> + int domid, int suspend_evtchn, int *lockfd)
> {
> if (suspend_evtchn >= 0)
> xc_evtchn_unbind(xce, suspend_evtchn);
>
> - return unlock_suspend_event(xch, domid);
> + return unlock_suspend_event(xch, domid, lockfd);
> }
>
> -int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int
> domid, int port)
> +int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce,
> + int domid, int port, int *lockfd)
> {
> int rc, suspend_evtchn = -1;
>
> - if (lock_suspend_event(xch, domid))
> - return -EINVAL;
> + if (lock_suspend_event(xch, domid, lockfd)) {
> + errno = EINVAL;
> + goto cleanup;
> + }
>
> suspend_evtchn = xc_evtchn_bind_interdomain(xce, domid, port);
> if (suspend_evtchn < 0) {
> @@ -126,17 +181,17 @@ int xc_suspend_evtchn_init_sane(xc_interface *xch,
> xc_evtchn *xce, int domid, in
> return suspend_evtchn;
>
> cleanup:
> - if (suspend_evtchn != -1)
> - xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
> + xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn, lockfd);
>
> return -1;
> }
>
> -int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce,
> int domid, int port)
> +int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce,
> + int domid, int port, int *lockfd)
> {
> int suspend_evtchn;
>
> - suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port);
> + suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port,
> lockfd);
> if (suspend_evtchn < 0)
> return suspend_evtchn;
>
> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
> index ce5456c..1f216cd 100644
> --- a/tools/libxc/xenguest.h
> +++ b/tools/libxc/xenguest.h
> @@ -254,13 +254,19 @@ int xc_hvm_build_target_mem(xc_interface *xch,
> int target,
> const char *image_name);
>
> -int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int
> domid, int suspend_evtchn);
> +/*
> + * Sets *lockfd to -1.
> + * Has deallocated everything even on error.
> + */
> +int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int
> domid, int suspend_evtchn, int *lockfd);
>
> /**
> * This function eats the initial notification.
> * xce must not be used for anything else
> + * See xc_suspend_evtchn_init_sane re lockfd.
> */
> -int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce,
> int domid, int port);
> +int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce,
> + int domid, int port, int *lockfd);
>
> /* xce must not be used for anything else */
> int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int
> suspend_evtchn);
> @@ -268,8 +274,12 @@ int xc_await_suspend(xc_interface *xch, xc_evtchn
> *xce, int suspend_evtchn);
> /**
> * The port will be signaled immediately after this call
> * The caller should check the domain status and look for the next event
> + * On success, *lockfd will be set to >=0 and *lockfd must be preserved
> + * and fed to xc_suspend_evtchn_release. (On error *lockfd is
> + * undefined and xc_suspend_evtchn_release is not allowed.)
> */
> -int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int
> domid, int port);
> +int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce,
> + int domid, int port, int *lockfd);
>
> int xc_get_bit_size(xc_interface *xch,
> const char *image_name, const char *cmdline,
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 4b42856..48a4b8e 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1340,6 +1340,7 @@ void libxl__domain_suspend(libxl__egc *egc,
> libxl__domain_suspend_state *dss)
> | (dss->hvm ? XCFLAGS_HVM : 0);
>
> dss->suspend_eventchn = -1;
> + dss->guest_evtchn_lockfd = -1;
> dss->guest_responded = 0;
> dss->dm_savefile = libxl__device_model_savefile(gc, domid);
>
> @@ -1358,7 +1359,8 @@ void libxl__domain_suspend(libxl__egc *egc,
> libxl__domain_suspend_state *dss)
>
> if (port >= 0) {
> dss->suspend_eventchn =
> - xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce,
> dss->domid, port);
> + xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce,
> + dss->domid, port,
> &dss->guest_evtchn_lockfd);
>
> if (dss->suspend_eventchn < 0)
> LOG(WARN, "Suspend event channel initialization failed");
> @@ -1522,7 +1524,7 @@ static void domain_suspend_done(libxl__egc *egc,
>
> if (dss->suspend_eventchn > 0)
> xc_suspend_evtchn_release(CTX->xch, dss->xce, domid,
> - dss->suspend_eventchn);
> + dss->suspend_eventchn,
> &dss->guest_evtchn_lockfd);
> if (dss->xce != NULL)
> xc_evtchn_close(dss->xce);
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 979b266..ebbfa09 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2417,6 +2417,7 @@ struct libxl__domain_suspend_state {
> /* private */
> xc_evtchn *xce; /* event channel handle */
> int suspend_eventchn;
> + int guest_evtchn_lockfd;
> int hvm;
> int xcflags;
> int guest_responded;
> diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
> index db76f79..bd9d936 100644
> --- a/tools/misc/xen-hptool.c
> +++ b/tools/misc/xen-hptool.c
> @@ -99,10 +99,13 @@ static int hp_mem_query_func(int argc, char *argv[])
>
> extern int xs_suspend_evtchn_port(int domid);
>
> -static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid,
> int *evtchn)
> +static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid,
> + int *evtchn, int *lockfd)
> {
> int port, rc, suspend_evtchn = -1;
>
> + *lockfd = -1;
> +
> if (!evtchn)
> return -1;
>
> @@ -112,7 +115,8 @@ static int suspend_guest(xc_interface *xch, xc_evtchn
> *xce, int domid, int *evtc
> fprintf(stderr, "DOM%d: No suspend port, try live migration\n",
> domid);
> goto failed;
> }
> - suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid,
> port);
> + suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid,
> + port, lockfd);
> if (suspend_evtchn < 0)
> {
> fprintf(stderr, "Suspend evtchn initialization failed\n");
> @@ -135,7 +139,8 @@ static int suspend_guest(xc_interface *xch, xc_evtchn
> *xce, int domid, int *evtc
>
> failed:
> if (suspend_evtchn != -1)
> - xc_suspend_evtchn_release(xch, xce, domid, suspend_evtchn);
> + xc_suspend_evtchn_release(xch, xce, domid,
> + suspend_evtchn, lockfd);
>
> return -1;
> }
> @@ -193,7 +198,7 @@ static int hp_mem_offline_func(int argc, char *argv[])
> }
> else if (status & PG_OFFLINE_OWNED)
> {
> - int result, suspend_evtchn = -1;
> + int result, suspend_evtchn = -1, suspend_lockfd = -1;
> xc_evtchn *xce;
> xce = xc_evtchn_open(NULL, 0);
>
> @@ -205,7 +210,8 @@ static int hp_mem_offline_func(int argc, char *argv[])
> }
>
> domid = status >> PG_OFFLINE_OWNER_SHIFT;
> - if (suspend_guest(xch, xce, domid, &suspend_evtchn))
> + if (suspend_guest(xch, xce, domid,
> + &suspend_evtchn, &suspend_lockfd))
> {
> fprintf(stderr, "Failed to suspend guest %d for"
> " mfn %lx\n", domid, mfn);
> @@ -231,7 +237,8 @@ static int hp_mem_offline_func(int argc, char *argv[])
> mfn, domid);
> }
> xc_domain_resume(xch, domid, 1);
> - xc_suspend_evtchn_release(xch, xce, domid,
> suspend_evtchn);
> + xc_suspend_evtchn_release(xch, xce, domid,
> + suspend_evtchn,
> &suspend_lockfd);
> xc_evtchn_close(xce);
> }
> break;
> diff --git a/tools/python/xen/lowlevel/checkpoint/checkpoint.h
> b/tools/python/xen/lowlevel/checkpoint/checkpoint.h
> index 187d9d7..2414956 100644
> --- a/tools/python/xen/lowlevel/checkpoint/checkpoint.h
> +++ b/tools/python/xen/lowlevel/checkpoint/checkpoint.h
> @@ -27,7 +27,7 @@ typedef struct {
> checkpoint_domtype domtype;
> int fd;
>
> - int suspend_evtchn;
> + int suspend_evtchn, suspend_lockfd;
>
> char* errstr;
>
> diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> index 817d272..74ca062 100644
> --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> @@ -57,6 +57,7 @@ void checkpoint_init(checkpoint_state* s)
> s->fd = -1;
>
> s->suspend_evtchn = -1;
> + s->suspend_lockfd = -1;
>
> s->errstr = NULL;
>
> @@ -360,7 +361,8 @@ static int setup_suspend_evtchn(checkpoint_state* s)
> return -1;
> }
>
> - s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce,
> s->domid, port);
> + s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce,
> + s->domid, port, &s->suspend_lockfd);
> if (s->suspend_evtchn < 0) {
> s->errstr = "failed to bind suspend event channel";
> return -1;
> @@ -377,7 +379,8 @@ static void release_suspend_evtchn(checkpoint_state *s)
> {
> /* TODO: teach xen to clean up if port is unbound */
> if (s->xce != NULL && s->suspend_evtchn >= 0) {
> - xc_suspend_evtchn_release(s->xch, s->xce, s->domid,
> s->suspend_evtchn);
> + xc_suspend_evtchn_release(s->xch, s->xce, s->domid,
> + s->suspend_evtchn, &s->suspend_lockfd);
> s->suspend_evtchn = -1;
> }
> }
> diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
> index 974f706..bf74e46 100644
> --- a/tools/xcutils/xc_save.c
> +++ b/tools/xcutils/xc_save.c
> @@ -167,7 +167,7 @@ int
> main(int argc, char **argv)
> {
> unsigned int maxit, max_f, lflags;
> - int io_fd, ret, port;
> + int io_fd, ret, port, suspend_lockfd = -1;
> struct save_callbacks callbacks;
> xentoollog_level lvl;
> xentoollog_logger *l;
> @@ -202,7 +202,8 @@ main(int argc, char **argv)
> else
> {
> si.suspend_evtchn =
> - xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid,
> port);
> + xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid,
> + port, &suspend_lockfd);
>
> if (si.suspend_evtchn < 0)
> warnx("suspend event channel initialization failed, "
> @@ -216,7 +217,8 @@ main(int argc, char **argv)
> &callbacks, !!(si.flags & XCFLAGS_HVM), 0);
>
> if (si.suspend_evtchn > 0)
> - xc_suspend_evtchn_release(si.xch, si.xce, si.domid,
> si.suspend_evtchn);
> + xc_suspend_evtchn_release(si.xch, si.xce, si.domid,
> + si.suspend_evtchn, &suspend_lockfd);
>
> if (si.xce > 0)
> xc_evtchn_close(si.xce);
> --
> 1.7.10.4
>
>
Thanks for this.
Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
Slightly off-track:
If you got a chance to test this patch, were you able to live migrate
more than one
domain simultaneously? I don't see any reason why it shouldn't be possible
but I sort of vaguely remember some bug fix in the past, where we couldn't
start more than one live migration at the same time
Shriram
[-- Attachment #1.2: Type: text/html, Size: 21233 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking
2014-03-16 4:53 ` Shriram Rajagopalan
@ 2014-03-17 11:35 ` Ian Jackson
2014-03-17 13:00 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-17 11:35 UTC (permalink / raw)
To: rshriram
Cc: xen-devel@lists.xensource.com, Ian Campbell, Stefano Stabellini,
Lai Jiangshan
Shriram Rajagopalan writes ("Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking"):
> On Tue, Mar 4, 2014 at 6:56 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
>
> Use fcntl F_SETLK, rather than writing our pid into a "lock" file.
> That way if we crash we don't leave the lockfile lying about. Callers
> now need to keep the fd for our lockfile. (We don't use flock because
> we don't want anyone who inherits this fd across fork to end up with a
> handle onto the lock.)
...
> Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
Thanks.
> Slightly off-track:
> If you got a chance to test this patch, were you able to live migrate more
> than one
> domain simultaneously? I don't see any reason why it shouldn't be possible
> but I sort of vaguely remember some bug fix in the past, where we couldn't
> start more than one live migration at the same time
I will test that.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking
2014-03-17 11:35 ` Ian Jackson
@ 2014-03-17 13:00 ` Ian Jackson
0 siblings, 0 replies; 70+ messages in thread
From: Ian Jackson @ 2014-03-17 13:00 UTC (permalink / raw)
To: rshriram, xen-devel@lists.xensource.com, Ian Campbell,
Lai Jiangshan, Stefano Stabellini
Ian Jackson writes ("Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking"):
> Shriram Rajagopalan writes ("Re: [PATCH 08/19] libxc: suspend: Fix suspend event channel locking"):
> > Slightly off-track:
> > If you got a chance to test this patch, were you able to live migrate more
> > than one
> > domain simultaneously? I don't see any reason why it shouldn't be possible
> > but I sort of vaguely remember some bug fix in the past, where we couldn't
> > start more than one live migration at the same time
>
> I will test that.
I am not able to do this. This is not because of my series - I have
verified that the same problem appears without it. It is because of
acquire_lock() in xl_cmdimpl.c, which is related to memory allocation
races. This really needs to be sorted out :-/, but at least it isn't
a blocker for my series.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (7 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 08/19] libxc: suspend: Fix suspend event channel locking Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 16:58 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 10/19] libxl: suspend: Async domain_suspend_callback_common Ian Jackson
` (10 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Mark the suspend callback libxl__domain_suspend_callback as
asynchronous in the helper stub generator (libxl_save_msgs_gen.pl).
We are going to want to provide an asynchronous version of this
function to get rid of the usleeps and waiting loops in the suspend
code.
libxl__domain_suspend_common_callback, the common synchronous core,
which used to be provided directly as the callback function for the
helper machinery, becomes libxl__domain_suspend_callback_common. It
can now take a typesafe parameter.
For now, provide two very similar asynchronous wrappers for it. Each
is a simple function which contains only boilerplate, calls the common
synchronous core, and returns the asynchronous response.
Essentially, we have just moved (in the case of suspend callbacks) the
call site of libxl__srm_callout_callback_complete. It was in the
switch statement in the autogenerated _libxl_save_msgs_callout.c, and
is now in the handwritten libxl_dom.c.
No functional change.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
v2: Commit message mentions usleeps, not Remus, as motivation.
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_dom.c | 25 +++++++++++++++++++------
tools/libxl/libxl_internal.h | 2 +-
tools/libxl/libxl_save_msgs_gen.pl | 2 +-
3 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 48a4b8e..1c0f04f 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1020,10 +1020,8 @@ int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
return 0;
}
-int libxl__domain_suspend_common_callback(void *user)
+int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
{
- libxl__save_helper_state *shs = user;
- libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
STATE_AO_GC(dss->ao);
unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
int ret;
@@ -1242,12 +1240,27 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
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 callbacks -----*/
-static int libxl__remus_domain_suspend_callback(void *data)
+static void libxl__remus_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);
+
/* REMUS TODO: Issue disk and network checkpoint reqs. */
- return libxl__domain_suspend_common_callback(data);
+ int ok = libxl__domain_suspend_callback_common(dss);
+ libxl__xc_domain_saverestore_async_callback_done(egc, shs, ok);
}
static int libxl__remus_domain_resume_callback(void *data)
@@ -1373,7 +1386,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
callbacks->postcopy = libxl__remus_domain_resume_callback;
callbacks->checkpoint = libxl__remus_domain_checkpoint_callback;
} else
- callbacks->suspend = libxl__domain_suspend_common_callback;
+ 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 --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ebbfa09..a32d75f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2717,7 +2717,7 @@ _hidden void libxl__xc_domain_save_done(libxl__egc*, void *dss_void,
void libxl__xc_domain_saverestore_async_callback_done(libxl__egc *egc,
libxl__save_helper_state *shs, int return_value);
-_hidden int libxl__domain_suspend_common_callback(void *data);
+_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,
diff --git a/tools/libxl/libxl_save_msgs_gen.pl b/tools/libxl/libxl_save_msgs_gen.pl
index ee126c7..3c6bd57 100755
--- a/tools/libxl/libxl_save_msgs_gen.pl
+++ b/tools/libxl/libxl_save_msgs_gen.pl
@@ -23,7 +23,7 @@ our @msgs = (
STRING doing_what),
'unsigned long', 'done',
'unsigned long', 'total'] ],
- [ 3, 'scxW', "suspend", [] ],
+ [ 3, 'scxA', "suspend", [] ],
[ 4, 'scxW', "postcopy", [] ],
[ 5, 'scxA', "checkpoint", [] ],
[ 6, 'scxA', "switch_qemu_logdirty", [qw(int domid
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback
2014-03-04 14:56 ` [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback Ian Jackson
@ 2014-03-13 16:58 ` Ian Campbell
2014-03-13 18:19 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 16:58 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Mark the suspend callback libxl__domain_suspend_callback as
> asynchronous in the helper stub generator (libxl_save_msgs_gen.pl).
> We are going to want to provide an asynchronous version of this
> function to get rid of the usleeps and waiting loops in the suspend
> code.
>
> libxl__domain_suspend_common_callback, the common synchronous core,
> which used to be provided directly as the callback function for the
> helper machinery, becomes libxl__domain_suspend_callback_common. It
> can now take a typesafe parameter.
>
> For now, provide two
two == remus & regular?
> very similar asynchronous wrappers for it. Each
> is a simple function which contains only boilerplate, calls the common
> synchronous core, and returns the asynchronous response.
>
> Essentially, we have just moved (in the case of suspend callbacks) the
> call site of libxl__srm_callout_callback_complete.
Do you mean libxl__srm_callout_sendreply here?
> It was in the
> switch statement in the autogenerated _libxl_save_msgs_callout.c, and
> is now in the handwritten libxl_dom.c.
>
> No functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback
2014-03-13 16:58 ` Ian Campbell
@ 2014-03-13 18:19 ` Ian Jackson
2014-03-14 9:54 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-13 18:19 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback"):
> On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > For now, provide two
>
> two == remus & regular?
Yes.
> > Essentially, we have just moved (in the case of suspend callbacks) the
> > call site of libxl__srm_callout_callback_complete.
>
> Do you mean libxl__srm_callout_sendreply here?
Yes.
I have updated the commit message in my series.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback
2014-03-13 18:19 ` Ian Jackson
@ 2014-03-14 9:54 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 9:54 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Thu, 2014-03-13 at 18:19 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback"):
> > On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > > For now, provide two
> >
> > two == remus & regular?
>
> Yes.
>
> > > Essentially, we have just moved (in the case of suspend callbacks) the
> > > call site of libxl__srm_callout_callback_complete.
> >
> > Do you mean libxl__srm_callout_sendreply here?
>
> Yes.
>
> I have updated the commit message in my series.
In which case: Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 10/19] libxl: suspend: Async domain_suspend_callback_common
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (8 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 09/19] libxl: suspend: Async libxl__domain_suspend_callback Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 16:59 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 11/19] libxl: suspend: Reorg domain_suspend_callback_common Ian Jackson
` (9 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Make domain_suspend_callback_common do its work and then call
dss->callback_common_done, rather than simply returning its answer.
This is preparatory to abolishing the usleeps in this function and
replacing them with use of the event machinery.
No functional change.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 46 ++++++++++++++++++++++++++++++------------
tools/libxl/libxl_internal.h | 4 +++-
2 files changed, 36 insertions(+), 14 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 1c0f04f..39f06da 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -755,6 +755,10 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
static void domain_suspend_done(libxl__egc *egc,
libxl__domain_suspend_state *dss, int rc);
+static void domain_suspend_callback_common_done(libxl__egc *egc,
+ libxl__domain_suspend_state *dss, int ok);
+static void remus_domain_suspend_callback_common_done(libxl__egc *egc,
+ libxl__domain_suspend_state *dss, int ok);
/*----- complicated callback, called by xc_domain_save -----*/
@@ -1020,7 +1024,9 @@ int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
return 0;
}
-int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
+/* calls dss->callback_common_done when done */
+static void domain_suspend_callback_common(libxl__egc *egc,
+ libxl__domain_suspend_state *dss)
{
STATE_AO_GC(dss->ao);
unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
@@ -1043,12 +1049,12 @@ int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
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;
@@ -1059,7 +1065,7 @@ int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
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;
@@ -1113,7 +1119,7 @@ int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
*/
if (!strcmp(state, "suspend")) {
LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
- return 0;
+ goto err;
}
LOG(DEBUG, "guest acknowledged suspend request");
@@ -1143,17 +1149,19 @@ int libxl__domain_suspend_callback_common(libxl__domain_suspend_state *dss)
}
LOG(ERROR, "guest did not suspend");
- return 0;
+ err:
+ dss->callback_common_done(egc, dss, 0);
+ return;
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;
+ goto err;
}
}
- return 1;
+ dss->callback_common_done(egc, dss, 1);
}
static inline char *physmap_path(libxl__gc *gc, uint32_t domid,
@@ -1246,10 +1254,16 @@ static void libxl__domain_suspend_callback(void *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);
+ dss->callback_common_done = domain_suspend_callback_common_done;
+ domain_suspend_callback_common(egc, dss);
}
+static void domain_suspend_callback_common_done(libxl__egc *egc,
+ libxl__domain_suspend_state *dss, int ok)
+{
+ libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, ok);
+}
+
/*----- remus callbacks -----*/
static void libxl__remus_domain_suspend_callback(void *data)
@@ -1258,11 +1272,17 @@ static void libxl__remus_domain_suspend_callback(void *data)
libxl__egc *egc = shs->egc;
libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
- /* REMUS TODO: Issue disk and network checkpoint reqs. */
- int ok = libxl__domain_suspend_callback_common(dss);
- libxl__xc_domain_saverestore_async_callback_done(egc, shs, ok);
+ dss->callback_common_done = remus_domain_suspend_callback_common_done;
+ domain_suspend_callback_common(egc, dss);
}
+static void remus_domain_suspend_callback_common_done(libxl__egc *egc,
+ libxl__domain_suspend_state *dss, int ok)
+{
+ /* REMUS TODO: Issue disk and network checkpoint reqs. */
+ libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, ok);
+}
+
static int libxl__remus_domain_resume_callback(void *data)
{
libxl__save_helper_state *shs = data;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a32d75f..369a1b3 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2425,6 +2425,8 @@ struct libxl__domain_suspend_state {
int interval; /* checkpoint interval (for Remus) */
libxl__save_helper_state shs;
libxl__logdirty_switch logdirty;
+ void (*callback_common_done)(libxl__egc*,
+ struct libxl__domain_suspend_state*, int ok);
/* private for libxl__domain_save_device_model */
libxl__save_device_model_cb *save_dm_callback;
libxl__datacopier_state save_dm_datacopier;
@@ -2717,7 +2719,7 @@ _hidden void libxl__xc_domain_save_done(libxl__egc*, void *dss_void,
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,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 10/19] libxl: suspend: Async domain_suspend_callback_common
2014-03-04 14:56 ` [PATCH 10/19] libxl: suspend: Async domain_suspend_callback_common Ian Jackson
@ 2014-03-13 16:59 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 16:59 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Make domain_suspend_callback_common do its work and then call
> dss->callback_common_done, rather than simply returning its answer.
>
> This is preparatory to abolishing the usleeps in this function and
> replacing them with use of the event machinery.
>
> No functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 11/19] libxl: suspend: Reorg domain_suspend_callback_common
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (9 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 10/19] libxl: suspend: Async domain_suspend_callback_common Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:02 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 12/19] libxl: suspend: New libxl__domain_pvcontrol_xspath Ian Jackson
` (8 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Make domain_suspend_callback_common more callback-oriented:
* Turn the functionality behind the goto labels "err" and
"guest_suspended" into functions which can be called just before
"return".
* Deindent the "issuing %s suspend request via XenBus control node"
branch; it is going to be split up into various functions as the
xenstore work becomes callback-based.
No functional change.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 158 ++++++++++++++++++++++++++++++-----------------
1 file changed, 103 insertions(+), 55 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 39f06da..59f6ce3 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1024,6 +1024,16 @@ int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
return 0;
}
+static void domain_suspend_common_wait_guest(libxl__egc *egc,
+ libxl__domain_suspend_state *dss);
+static void domain_suspend_common_guest_suspended(libxl__egc *egc,
+ libxl__domain_suspend_state *dss);
+static void domain_suspend_common_failed(libxl__egc *egc,
+ libxl__domain_suspend_state *dss);
+static void domain_suspend_common_done(libxl__egc *egc,
+ libxl__domain_suspend_state *dss,
+ bool ok);
+
/* calls dss->callback_common_done when done */
static void domain_suspend_callback_common(libxl__egc *egc,
libxl__domain_suspend_state *dss)
@@ -1057,7 +1067,8 @@ static void domain_suspend_callback_common(libxl__egc *egc,
goto err;
}
dss->guest_responded = 1;
- goto guest_suspended;
+ domain_suspend_common_guest_suspended(egc, dss);
+ return;
}
if (dss->hvm && (!hvm_pvdrv || hvm_s_state)) {
@@ -1069,63 +1080,81 @@ static void domain_suspend_callback_common(libxl__egc *egc,
}
/* The guest does not (need to) respond to this sort of request. */
dss->guest_responded = 1;
- } else {
- LOG(DEBUG, "issuing %s suspend request via XenBus control node",
- dss->hvm ? "PVHVM" : "PV");
+ domain_suspend_common_wait_guest(egc, dss);
+ return;
+ }
- libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
+ LOG(DEBUG, "issuing %s suspend request via XenBus control node",
+ dss->hvm ? "PVHVM" : "PV");
- LOG(DEBUG, "wait for the guest to acknowledge suspend request");
- watchdog = 60;
- while (!strcmp(state, "suspend") && watchdog > 0) {
- usleep(100000);
+ libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
- state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
- if (!state) state = "";
+ LOG(DEBUG, "wait for the guest to acknowledge suspend request");
+ watchdog = 60;
+ while (!strcmp(state, "suspend") && watchdog > 0) {
+ usleep(100000);
- watchdog--;
- }
+ state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
+ if (!state) state = "";
- /*
- * 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;
+ 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);
- /*
- * 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");
- goto err;
- }
+ 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;
- LOG(DEBUG, "guest acknowledged suspend request");
- dss->guest_responded = 1;
}
+ /*
+ * 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");
+ goto err;
+ }
+
+ LOG(DEBUG, "guest acknowledged suspend request");
+ dss->guest_responded = 1;
+ domain_suspend_common_wait_guest(egc,dss);
+ return;
+
+ err:
+ domain_suspend_common_failed(egc, dss);
+}
+
+static void domain_suspend_common_wait_guest(libxl__egc *egc,
+ libxl__domain_suspend_state *dss)
+{
+ STATE_AO_GC(dss->ao);
+ int ret;
+ int watchdog;
+
+ /* Convenience aliases */
+ const uint32_t domid = dss->domid;
+
LOG(DEBUG, "wait for the guest to suspend");
watchdog = 60;
while (watchdog > 0) {
@@ -1141,7 +1170,8 @@ static void domain_suspend_callback_common(libxl__egc *egc,
& XEN_DOMINF_shutdownmask;
if (shutdown_reason == SHUTDOWN_suspend) {
LOG(DEBUG, "guest has suspended");
- goto guest_suspended;
+ domain_suspend_common_guest_suspended(egc, dss);
+ return;
}
}
@@ -1149,19 +1179,37 @@ static void domain_suspend_callback_common(libxl__egc *egc,
}
LOG(ERROR, "guest did not suspend");
- err:
- dss->callback_common_done(egc, dss, 0);
- return;
+ domain_suspend_common_failed(egc, dss);
+}
+
+static void domain_suspend_common_guest_suspended(libxl__egc *egc,
+ libxl__domain_suspend_state *dss)
+{
+ STATE_AO_GC(dss->ao);
+ int ret;
- 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);
- goto err;
+ domain_suspend_common_failed(egc, dss);
+ return;
}
}
- dss->callback_common_done(egc, dss, 1);
+ domain_suspend_common_done(egc, dss, 1);
+}
+
+static void domain_suspend_common_failed(libxl__egc *egc,
+ libxl__domain_suspend_state *dss)
+{
+ domain_suspend_common_done(egc, dss, 0);
+}
+
+static void domain_suspend_common_done(libxl__egc *egc,
+ libxl__domain_suspend_state *dss,
+ bool ok)
+{
+ dss->callback_common_done(egc, dss, ok);
}
static inline char *physmap_path(libxl__gc *gc, uint32_t domid,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 11/19] libxl: suspend: Reorg domain_suspend_callback_common
2014-03-04 14:56 ` [PATCH 11/19] libxl: suspend: Reorg domain_suspend_callback_common Ian Jackson
@ 2014-03-13 17:02 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:02 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Make domain_suspend_callback_common more callback-oriented:
>
> * Turn the functionality behind the goto labels "err" and
> "guest_suspended" into functions which can be called just before
> "return".
>
> * Deindent the "issuing %s suspend request via XenBus control node"
> branch; it is going to be split up into various functions as the
> xenstore work becomes callback-based.
>
> No functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 12/19] libxl: suspend: New libxl__domain_pvcontrol_xspath
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (10 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 11/19] libxl: suspend: Reorg domain_suspend_callback_common Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:03 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 13/19] libxl: suspend: New domain_suspend_pvcontrol_acked Ian Jackson
` (7 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Factor out the pv control node xenstore path calculation into
libxl__domain_pvcontrol_xspath.
This xs path calculation was open coded in
libxl__domain_pvcontrol_read and _write. This is undesirable because
it duplicates the code and because it makes the path inaccessible to
other parts of libxl (which are soon going to want it).
No functional change.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 21 +++++++++++----------
tools/libxl/libxl_internal.h | 1 +
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f91a398..395d18e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -894,17 +894,23 @@ int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid)
return !!pvdriver;
}
-char * libxl__domain_pvcontrol_read(libxl__gc *gc, xs_transaction_t t,
- uint32_t domid)
+const char *libxl__domain_pvcontrol_xspath(libxl__gc *gc, uint32_t domid)
{
- const char *shutdown_path;
const char *dom_path;
dom_path = libxl__xs_get_dompath(gc, domid);
if (!dom_path)
return NULL;
- shutdown_path = libxl__sprintf(gc, "%s/control/shutdown", dom_path);
+ return GCSPRINTF("%s/control/shutdown", dom_path);
+}
+
+char * libxl__domain_pvcontrol_read(libxl__gc *gc, xs_transaction_t t,
+ uint32_t domid)
+{
+ const char *shutdown_path;
+
+ shutdown_path = libxl__domain_pvcontrol_xspath(gc, domid);
if (!shutdown_path)
return NULL;
@@ -915,13 +921,8 @@ int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
uint32_t domid, const char *cmd)
{
const char *shutdown_path;
- const char *dom_path;
-
- dom_path = libxl__xs_get_dompath(gc, domid);
- if (!dom_path)
- return ERROR_FAIL;
- shutdown_path = libxl__sprintf(gc, "%s/control/shutdown", dom_path);
+ shutdown_path = libxl__domain_pvcontrol_xspath(gc, domid);
if (!shutdown_path)
return ERROR_FAIL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 369a1b3..97dbd8e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -979,6 +979,7 @@ _hidden int libxl__domain_resume(libxl__gc *gc, uint32_t domid,
/* returns 0 or 1, or a libxl error code */
_hidden int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid);
+_hidden const char *libxl__domain_pvcontrol_xspath(libxl__gc*, uint32_t domid);
_hidden char * libxl__domain_pvcontrol_read(libxl__gc *gc,
xs_transaction_t t, uint32_t domid);
_hidden int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 12/19] libxl: suspend: New libxl__domain_pvcontrol_xspath
2014-03-04 14:56 ` [PATCH 12/19] libxl: suspend: New libxl__domain_pvcontrol_xspath Ian Jackson
@ 2014-03-13 17:03 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:03 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Factor out the pv control node xenstore path calculation into
> libxl__domain_pvcontrol_xspath.
>
> This xs path calculation was open coded in
> libxl__domain_pvcontrol_read and _write. This is undesirable because
> it duplicates the code and because it makes the path inaccessible to
> other parts of libxl (which are soon going to want it).
>
> No functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 13/19] libxl: suspend: New domain_suspend_pvcontrol_acked
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (11 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 12/19] libxl: suspend: New libxl__domain_pvcontrol_xspath Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:05 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 14/19] libxl: suspend: domain_suspend_callback_common xs errs Ian Jackson
` (6 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Factor out domain_suspend_pvcontrol_acked.
This replaces a bunch of open-coded strcmp()s and makes the code
clearer. It also eliminates the need to check for state==NULL each
time it's read, because we can check for NULL once before the strcmp.
No functional change.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 59f6ce3..a0b3a57 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1034,6 +1034,12 @@ static void domain_suspend_common_done(libxl__egc *egc,
libxl__domain_suspend_state *dss,
bool ok);
+static bool domain_suspend_pvcontrol_acked(const char *state) {
+ /* any value other than "suspend", including ENOENT, is OK */
+ if (!state) return 1;
+ return strcmp(state,"suspend");
+}
+
/* calls dss->callback_common_done when done */
static void domain_suspend_callback_common(libxl__egc *egc,
libxl__domain_suspend_state *dss)
@@ -1091,11 +1097,10 @@ static void domain_suspend_callback_common(libxl__egc *egc,
LOG(DEBUG, "wait for the guest to acknowledge suspend request");
watchdog = 60;
- while (!strcmp(state, "suspend") && watchdog > 0) {
+ while (!domain_suspend_pvcontrol_acked(state) && watchdog > 0) {
usleep(100000);
state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
- if (!state) state = "";
watchdog--;
}
@@ -1109,21 +1114,19 @@ static void domain_suspend_callback_common(libxl__egc *egc,
* against the guest catching up and acknowledging the request
* at the last minute.
*/
- if (!strcmp(state, "suspend")) {
+ if (!domain_suspend_pvcontrol_acked(state)) {
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"))
+ if (!domain_suspend_pvcontrol_acked(state))
libxl__domain_pvcontrol_write(gc, t, domid, "");
if (!xs_transaction_end(CTX->xsh, t, 0))
if (errno == EAGAIN)
goto retry_transaction;
-
}
/*
@@ -1131,7 +1134,7 @@ static void domain_suspend_callback_common(libxl__egc *egc,
* acknowledged while we were cancelling the request in which
* case we lost the race while cancelling and should continue.
*/
- if (!strcmp(state, "suspend")) {
+ if (!domain_suspend_pvcontrol_acked(state)) {
LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
goto err;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 13/19] libxl: suspend: New domain_suspend_pvcontrol_acked
2014-03-04 14:56 ` [PATCH 13/19] libxl: suspend: New domain_suspend_pvcontrol_acked Ian Jackson
@ 2014-03-13 17:05 ` Ian Campbell
2014-03-13 18:22 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:05 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Factor out domain_suspend_pvcontrol_acked.
>
> This replaces a bunch of open-coded strcmp()s and makes the code
> clearer. It also eliminates the need to check for state==NULL each
> time it's read, because we can check for NULL once before the strcmp.
>
> No functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_dom.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 59f6ce3..a0b3a57 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1034,6 +1034,12 @@ static void domain_suspend_common_done(libxl__egc *egc,
> libxl__domain_suspend_state *dss,
> bool ok);
>
> +static bool domain_suspend_pvcontrol_acked(const char *state) {
> + /* any value other than "suspend", including ENOENT, is OK */
Where ENOENT is indicated by !state I suppose?
Assuming so you can mention that here if you like, either way:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 13/19] libxl: suspend: New domain_suspend_pvcontrol_acked
2014-03-13 17:05 ` Ian Campbell
@ 2014-03-13 18:22 ` Ian Jackson
0 siblings, 0 replies; 70+ messages in thread
From: Ian Jackson @ 2014-03-13 18:22 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 13/19] libxl: suspend: New domain_suspend_pvcontrol_acked"):
> On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > +static bool domain_suspend_pvcontrol_acked(const char *state) {
> > + /* any value other than "suspend", including ENOENT, is OK */
>
> Where ENOENT is indicated by !state I suppose?
>
> Assuming so you can mention that here if you like, either way:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Yes, thanks. I have changed it to say:
/* any value other than "suspend", including ENOENT (i.e. !state), is OK */
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 14/19] libxl: suspend: domain_suspend_callback_common xs errs
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (12 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 13/19] libxl: suspend: New domain_suspend_pvcontrol_acked Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:06 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait Ian Jackson
` (5 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
In domain_suspend_callback_common, use libxl__xs_transaction_start in
a loop, rather than xs_transaction_start and a goto label.
This will improve the error handling, but have no other semantic
effect.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index a0b3a57..8aceba9 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1050,6 +1050,7 @@ static void domain_suspend_callback_common(libxl__egc *egc,
char *state = "suspend";
int watchdog;
xs_transaction_t t;
+ int rc;
/* Convenience aliases */
const uint32_t domid = dss->domid;
@@ -1116,17 +1117,19 @@ static void domain_suspend_callback_common(libxl__egc *egc,
*/
if (!domain_suspend_pvcontrol_acked(state)) {
LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
- retry_transaction:
- t = xs_transaction_start(CTX->xsh);
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto err;
- state = libxl__domain_pvcontrol_read(gc, t, domid);
+ state = libxl__domain_pvcontrol_read(gc, t, domid);
- if (!domain_suspend_pvcontrol_acked(state))
- libxl__domain_pvcontrol_write(gc, t, domid, "");
+ if (!domain_suspend_pvcontrol_acked(state))
+ libxl__domain_pvcontrol_write(gc, t, domid, "");
- if (!xs_transaction_end(CTX->xsh, t, 0))
- if (errno == EAGAIN)
- goto retry_transaction;
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc<0) goto err;
+ }
}
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 14/19] libxl: suspend: domain_suspend_callback_common xs errs
2014-03-04 14:56 ` [PATCH 14/19] libxl: suspend: domain_suspend_callback_common xs errs Ian Jackson
@ 2014-03-13 17:06 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:06 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> In domain_suspend_callback_common, use libxl__xs_transaction_start in
> a loop, rather than xs_transaction_start and a goto label.
>
> This will improve the error handling, but have no other semantic
> effect.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (13 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 14/19] libxl: suspend: domain_suspend_callback_common xs errs Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:13 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait Ian Jackson
` (4 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
When negotiating guest suspend via the xenstore pvcontrol protocol
(ie when the guest does NOT support the evtchn fast suspend protocol):
Replace the use of loops and usleep with a call to libxl__xswait.
Also, replace the xenstore transaction loop with one using
libxl__xs_transaction_start et al.
There is not intended to be any semantic change, other than to make
the algorithm properly asynchronous.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 93 ++++++++++++++++++++++++++----------------
tools/libxl/libxl_internal.h | 1 +
2 files changed, 59 insertions(+), 35 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 8aceba9..dde7e33 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1028,6 +1028,8 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
libxl__domain_suspend_state *dss);
static void domain_suspend_common_guest_suspended(libxl__egc *egc,
libxl__domain_suspend_state *dss);
+static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
+ libxl__xswait_state *xswa, int rc, const char *state);
static void domain_suspend_common_failed(libxl__egc *egc,
libxl__domain_suspend_state *dss);
static void domain_suspend_common_done(libxl__egc *egc,
@@ -1047,10 +1049,6 @@ static void domain_suspend_callback_common(libxl__egc *egc,
STATE_AO_GC(dss->ao);
unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
int ret;
- char *state = "suspend";
- int watchdog;
- xs_transaction_t t;
- int rc;
/* Convenience aliases */
const uint32_t domid = dss->domid;
@@ -1096,59 +1094,81 @@ static void domain_suspend_callback_common(libxl__egc *egc,
libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
- LOG(DEBUG, "wait for the guest to acknowledge suspend request");
- watchdog = 60;
- while (!domain_suspend_pvcontrol_acked(state) && watchdog > 0) {
- usleep(100000);
+ dss->pvcontrol.path = libxl__domain_pvcontrol_xspath(gc, domid);
+ if (!dss->pvcontrol.path) goto err;
- state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
+ dss->pvcontrol.ao = ao;
+ dss->pvcontrol.what = "guest acknowledgement of suspend request";
+ dss->pvcontrol.timeout_ms = 60 * 1000;
+ dss->pvcontrol.callback = domain_suspend_common_pvcontrol_suspending;
+ libxl__xswait_start(gc, &dss->pvcontrol);
+ return;
- watchdog--;
- }
+ err:
+ domain_suspend_common_failed(egc, dss);
+}
- /*
- * 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 (!domain_suspend_pvcontrol_acked(state)) {
- LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
+static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
+ libxl__xswait_state *xswa, int rc, const char *state)
+{
+ libxl__domain_suspend_state *dss = CONTAINER_OF(xswa, *dss, pvcontrol);
+ STATE_AO_GC(dss->ao);
+ xs_transaction_t t = 0;
+
+ if (!rc && !domain_suspend_pvcontrol_acked(state))
+ /* keep waiting */
+ return;
+
+ libxl__xswait_stop(gc, &dss->pvcontrol);
+
+ if (rc == ERROR_TIMEDOUT) {
+ /*
+ * 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.
+ */
for (;;) {
rc = libxl__xs_transaction_start(gc, &t);
if (rc) goto err;
- state = libxl__domain_pvcontrol_read(gc, t, domid);
+ rc = libxl__xs_read_checked(gc, t, xswa->path, &state);
+ if (rc) goto err;
+
+ if (domain_suspend_pvcontrol_acked(state))
+ break;
- if (!domain_suspend_pvcontrol_acked(state))
- libxl__domain_pvcontrol_write(gc, t, domid, "");
+ rc = libxl__xs_write_checked(gc, t, xswa->path, "");
+ if (rc) goto err;
rc = libxl__xs_transaction_commit(gc, &t);
- if (!rc) break;
+ if (!rc) {
+ LOG(ERROR,
+ "guest didn't acknowledge suspend, cancelling request");
+ goto err;
+ }
if (rc<0) goto err;
}
- }
-
- /*
- * 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 (!domain_suspend_pvcontrol_acked(state)) {
- LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
+ } else if (rc) {
+ /* some error in xswait's read of xenstore, already logged */
goto err;
}
+ assert(domain_suspend_pvcontrol_acked(state));
LOG(DEBUG, "guest acknowledged suspend request");
+
+ libxl__xs_transaction_abort(gc, &t);
dss->guest_responded = 1;
domain_suspend_common_wait_guest(egc,dss);
return;
err:
+ libxl__xs_transaction_abort(gc, &t);
domain_suspend_common_failed(egc, dss);
+ return;
}
static void domain_suspend_common_wait_guest(libxl__egc *egc,
@@ -1215,6 +1235,8 @@ static void domain_suspend_common_done(libxl__egc *egc,
libxl__domain_suspend_state *dss,
bool ok)
{
+ EGC_GC;
+ assert(!libxl__xswait_inuse(&dss->pvcontrol));
dss->callback_common_done(egc, dss, ok);
}
@@ -1400,6 +1422,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
&dss->shs.callbacks.save.a;
logdirty_init(&dss->logdirty);
+ libxl__xswait_init(&dss->pvcontrol);
switch (type) {
case LIBXL_DOMAIN_TYPE_HVM: {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 97dbd8e..5ac02cb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2422,6 +2422,7 @@ struct libxl__domain_suspend_state {
int hvm;
int xcflags;
int guest_responded;
+ libxl__xswait_state pvcontrol;
const char *dm_savefile;
int interval; /* checkpoint interval (for Remus) */
libxl__save_helper_state shs;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait
2014-03-04 14:56 ` [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait Ian Jackson
@ 2014-03-13 17:13 ` Ian Campbell
2014-03-13 18:26 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:13 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> When negotiating guest suspend via the xenstore pvcontrol protocol
> (ie when the guest does NOT support the evtchn fast suspend protocol):
>
> Replace the use of loops and usleep with a call to libxl__xswait.
>
> Also, replace the xenstore transaction loop with one using
> libxl__xs_transaction_start et al.
>
> There is not intended to be any semantic change, other than to make
> the algorithm properly asynchronous.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_dom.c | 93 ++++++++++++++++++++++++++----------------
> tools/libxl/libxl_internal.h | 1 +
> 2 files changed, 59 insertions(+), 35 deletions(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 8aceba9..dde7e33 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1028,6 +1028,8 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
> libxl__domain_suspend_state *dss);
> static void domain_suspend_common_guest_suspended(libxl__egc *egc,
> libxl__domain_suspend_state *dss);
> +static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
> + libxl__xswait_state *xswa, int rc, const char *state);
> static void domain_suspend_common_failed(libxl__egc *egc,
> libxl__domain_suspend_state *dss);
> static void domain_suspend_common_done(libxl__egc *egc,
> @@ -1047,10 +1049,6 @@ static void domain_suspend_callback_common(libxl__egc *egc,
> STATE_AO_GC(dss->ao);
> unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
> int ret;
> - char *state = "suspend";
> - int watchdog;
> - xs_transaction_t t;
> - int rc;
>
> /* Convenience aliases */
> const uint32_t domid = dss->domid;
> @@ -1096,59 +1094,81 @@ static void domain_suspend_callback_common(libxl__egc *egc,
>
> libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "suspend");
>
> - LOG(DEBUG, "wait for the guest to acknowledge suspend request");
> - watchdog = 60;
> - while (!domain_suspend_pvcontrol_acked(state) && watchdog > 0) {
> - usleep(100000);
> + dss->pvcontrol.path = libxl__domain_pvcontrol_xspath(gc, domid);
> + if (!dss->pvcontrol.path) goto err;
>
> - state = libxl__domain_pvcontrol_read(gc, XBT_NULL, domid);
> + dss->pvcontrol.ao = ao;
> + dss->pvcontrol.what = "guest acknowledgement of suspend request";
> + dss->pvcontrol.timeout_ms = 60 * 1000;
> + dss->pvcontrol.callback = domain_suspend_common_pvcontrol_suspending;
> + libxl__xswait_start(gc, &dss->pvcontrol);
> + return;
>
> - watchdog--;
> - }
> + err:
> + domain_suspend_common_failed(egc, dss);
> +}
>
> - /*
> - * 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 (!domain_suspend_pvcontrol_acked(state)) {
> - LOG(ERROR, "guest didn't acknowledge suspend, cancelling request");
> +static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
> + libxl__xswait_state *xswa, int rc, const char *state)
> +{
> + libxl__domain_suspend_state *dss = CONTAINER_OF(xswa, *dss, pvcontrol);
> + STATE_AO_GC(dss->ao);
> + xs_transaction_t t = 0;
> +
> + if (!rc && !domain_suspend_pvcontrol_acked(state))
> + /* keep waiting */
> + return;
> +
> + libxl__xswait_stop(gc, &dss->pvcontrol);
> +
> + if (rc == ERROR_TIMEDOUT) {
> + /*
> + * 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.
> + */
> for (;;) {
> rc = libxl__xs_transaction_start(gc, &t);
> if (rc) goto err;
>
> - state = libxl__domain_pvcontrol_read(gc, t, domid);
> + rc = libxl__xs_read_checked(gc, t, xswa->path, &state);
> + if (rc) goto err;
> +
> + if (domain_suspend_pvcontrol_acked(state))
> + break;
>
> - if (!domain_suspend_pvcontrol_acked(state))
> - libxl__domain_pvcontrol_write(gc, t, domid, "");
> + rc = libxl__xs_write_checked(gc, t, xswa->path, "");
> + if (rc) goto err;
>
> rc = libxl__xs_transaction_commit(gc, &t);
> - if (!rc) break;
> + if (!rc) {
> + LOG(ERROR,
> + "guest didn't acknowledge suspend, cancelling request");
> + goto err;
> + }
> if (rc<0) goto err;
> }
> - }
> -
> - /*
> - * 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.
This behaviour has gone completely now?
> - */
> - if (!domain_suspend_pvcontrol_acked(state)) {
> - LOG(ERROR, "guest didn't acknowledge suspend, request cancelled");
> + } else if (rc) {
> + /* some error in xswait's read of xenstore, already logged */
> goto err;
> }
>
> + assert(domain_suspend_pvcontrol_acked(state));
> LOG(DEBUG, "guest acknowledged suspend request");
> +
> + libxl__xs_transaction_abort(gc, &t);
Is this right/proper in the success case?
If rc != TIMEOUT t may not have been started, although it is initialised
so I suppose this is intended to be ok.
> dss->guest_responded = 1;
> domain_suspend_common_wait_guest(egc,dss);
> return;
>
> err:
> + libxl__xs_transaction_abort(gc, &t);
> domain_suspend_common_failed(egc, dss);
> + return;
> }
>
> static void domain_suspend_common_wait_guest(libxl__egc *egc,
> @@ -1215,6 +1235,8 @@ static void domain_suspend_common_done(libxl__egc *egc,
> libxl__domain_suspend_state *dss,
> bool ok)
> {
> + EGC_GC;
> + assert(!libxl__xswait_inuse(&dss->pvcontrol));
> dss->callback_common_done(egc, dss, ok);
> }
>
> @@ -1400,6 +1422,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
> &dss->shs.callbacks.save.a;
>
> logdirty_init(&dss->logdirty);
> + libxl__xswait_init(&dss->pvcontrol);
>
> switch (type) {
> case LIBXL_DOMAIN_TYPE_HVM: {
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 97dbd8e..5ac02cb 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2422,6 +2422,7 @@ struct libxl__domain_suspend_state {
> int hvm;
> int xcflags;
> int guest_responded;
> + libxl__xswait_state pvcontrol;
> const char *dm_savefile;
> int interval; /* checkpoint interval (for Remus) */
> libxl__save_helper_state shs;
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait
2014-03-13 17:13 ` Ian Campbell
@ 2014-03-13 18:26 ` Ian Jackson
2014-03-14 10:06 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-13 18:26 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait"):
> On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > When negotiating guest suspend via the xenstore pvcontrol protocol
> > (ie when the guest does NOT support the evtchn fast suspend protocol):
...
> > - /*
> > - * 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.
>
> This behaviour has gone completely now?
No, this possibility still exists. This is handled by the rc ==
ERROR_TIMEDOUT branch in domain_suspend_common_pvcontrol_suspending.
The comment there (now) reads:
/*
* 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.
*/
> > + assert(domain_suspend_pvcontrol_acked(state));
> > LOG(DEBUG, "guest acknowledged suspend request");
> > +
> > + libxl__xs_transaction_abort(gc, &t);
>
> Is this right/proper in the success case?
Yes. Because it has then already been committed. This is exactly the
pattern from the usage comment for libxl__xs_transaction_* in
libxl_internal.h.
> If rc != TIMEOUT t may not have been started, although it is initialised
> so I suppose this is intended to be ok.
Yes. Precisely.
Thanks for the careful review :-).
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait
2014-03-13 18:26 ` Ian Jackson
@ 2014-03-14 10:06 ` Ian Campbell
2014-03-14 17:24 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 10:06 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Thu, 2014-03-13 at 18:26 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait"):
> > On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > > When negotiating guest suspend via the xenstore pvcontrol protocol
> > > (ie when the guest does NOT support the evtchn fast suspend protocol):
> ...
> > > - /*
> > > - * 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.
> >
> > This behaviour has gone completely now?
>
> No, this possibility still exists. This is handled by the rc ==
> ERROR_TIMEDOUT branch in domain_suspend_common_pvcontrol_suspending.
> The comment there (now) reads:
>
> /*
> * 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.
> */
I was querying it because the original code appears to have both of the
above ("Guest appears to not..." and "Final check for..."). IOW even
after the final check transaction to clear it checks state again.
I think what you've done is make that "final" check within the
transaction loop with:
if (domain_suspend_pvcontrol_acked(state))
break;
Would it make sense to include something akin to the "Final check"
comment just before this to clarify?
>
> > > + assert(domain_suspend_pvcontrol_acked(state));
> > > LOG(DEBUG, "guest acknowledged suspend request");
> > > +
> > > + libxl__xs_transaction_abort(gc, &t);
> >
> > Is this right/proper in the success case?
>
> Yes. Because it has then already been committed. This is exactly the
> pattern from the usage comment for libxl__xs_transaction_* in
> libxl_internal.h.
That usage comment doesn't have an abort after a successful commit
though i.e. in the "if (!rc) break;" case it drops straight through to
the success and return something case.
I think the subtlety is that the transaction loop here differs slightly
from the canonical example because of the "final check" break quoted
above -- since that exits the transaction successfully without actually
needing to complete the transaction.
Assuming aborting a committed transaction is ok then a comment might be
worthwhile. Or do the abort just before the exceptional success break,
which is a bit unconventional but then so is the check itself.
Ian.
> > If rc != TIMEOUT t may not have been started, although it is initialised
> > so I suppose this is intended to be ok.
>
> Yes. Precisely.
>
> Thanks for the careful review :-).
>
> Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait
2014-03-14 10:06 ` Ian Campbell
@ 2014-03-14 17:24 ` Ian Jackson
2014-03-14 17:39 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-14 17:24 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait"):
> On Thu, 2014-03-13 at 18:26 +0000, Ian Jackson wrote:
> > No, this possibility still exists. This is handled by the rc ==
> > ERROR_TIMEDOUT branch in domain_suspend_common_pvcontrol_suspending.
> > The comment there (now) reads:
> >
> > /*
> > * 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.
> > */
>
> I was querying it because the original code appears to have both of the
> above ("Guest appears to not..." and "Final check for..."). IOW even
> after the final check transaction to clear it checks state again.
That's just an artefact of the tangled control structure (and
confusing comments) in the old code. At the "Final check" it's not
actually reading xs again - that happened earlier. It's just coping
with one of the two possible exits from the transaction loop. In the
old code the last-minute-success path and the timeout-failure path are
mixed during and after the loop and need to be distinguished again.
In the new code, the success case is the main path;
last-minute-success comes out in the wash, falling through into the
main success path. The timeout-failure path goes to err, so there is
nothing corresponding to the if following "Final check".
> I think what you've done is make that "final" check within the
> transaction loop with:
> if (domain_suspend_pvcontrol_acked(state))
> break;
>
> Would it make sense to include something akin to the "Final check"
> comment just before this to clarify?
In the old code the "Final check" comment is the second part of the
explanation "We re-read", earlier.
I think this is only confusing if you look at the confusing old code.
If you just look at the new code it's clearly correct. The comment
above clearly says that the purpose is to handle last-minute-success.
But if you prefer I could add a comment
> if (domain_suspend_pvcontrol_acked(state))
+ /* last minute ack */
> break;
> > > Is this right/proper in the success case?
> >
> > Yes. Because it has then already been committed. This is exactly the
> > pattern from the usage comment for libxl__xs_transaction_* in
> > libxl_internal.h.
>
> That usage comment doesn't have an abort after a successful commit
> though i.e. in the "if (!rc) break;" case it drops straight through to
> the success and return something case.
Oh I see.
> I think the subtlety is that the transaction loop here differs slightly
> from the canonical example because of the "final check" break quoted
> above -- since that exits the transaction successfully without actually
> needing to complete the transaction.
>
> Assuming aborting a committed transaction is ok then a comment might be
> worthwhile. Or do the abort just before the exceptional success break,
> which is a bit unconventional but then so is the check itself.
How about if I add a new patch introducing a comment documenting
formally the possible states of *t with the libxl__xs_transaction
functions ?
And/or perhaps rename libxl__xs_transaction_abort to
libxl__xs_transaction_cleanup ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait
2014-03-14 17:24 ` Ian Jackson
@ 2014-03-14 17:39 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 17:39 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Fri, 2014-03-14 at 17:24 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait"):
> > On Thu, 2014-03-13 at 18:26 +0000, Ian Jackson wrote:
> > > No, this possibility still exists. This is handled by the rc ==
> > > ERROR_TIMEDOUT branch in domain_suspend_common_pvcontrol_suspending.
> > > The comment there (now) reads:
> > >
> > > /*
> > > * 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.
> > > */
> >
> > I was querying it because the original code appears to have both of the
> > above ("Guest appears to not..." and "Final check for..."). IOW even
> > after the final check transaction to clear it checks state again.
>
> That's just an artefact of the tangled control structure (and
> confusing comments) in the old code. At the "Final check" it's not
> actually reading xs again - that happened earlier. It's just coping
> with one of the two possible exits from the transaction loop. In the
> old code the last-minute-success path and the timeout-failure path are
> mixed during and after the loop and need to be distinguished again.
>
> In the new code, the success case is the main path;
> last-minute-success comes out in the wash, falling through into the
> main success path. The timeout-failure path goes to err, so there is
> nothing corresponding to the if following "Final check".
>
> > I think what you've done is make that "final" check within the
> > transaction loop with:
> > if (domain_suspend_pvcontrol_acked(state))
> > break;
> >
> > Would it make sense to include something akin to the "Final check"
> > comment just before this to clarify?
>
> In the old code the "Final check" comment is the second part of the
> explanation "We re-read", earlier.
>
> I think this is only confusing if you look at the confusing old code.
That's quite plausible.
> If you just look at the new code it's clearly correct. The comment
> above clearly says that the purpose is to handle last-minute-success.
> But if you prefer I could add a comment
Based on the above I suppose it isn't necessary (nor harmful). As you
like.
>
> > if (domain_suspend_pvcontrol_acked(state))
> + /* last minute ack */
> > break;
>
> > > > Is this right/proper in the success case?
> > >
> > > Yes. Because it has then already been committed. This is exactly the
> > > pattern from the usage comment for libxl__xs_transaction_* in
> > > libxl_internal.h.
> >
> > That usage comment doesn't have an abort after a successful commit
> > though i.e. in the "if (!rc) break;" case it drops straight through to
> > the success and return something case.
>
> Oh I see.
>
> > I think the subtlety is that the transaction loop here differs slightly
> > from the canonical example because of the "final check" break quoted
> > above -- since that exits the transaction successfully without actually
> > needing to complete the transaction.
> >
> > Assuming aborting a committed transaction is ok then a comment might be
> > worthwhile. Or do the abort just before the exceptional success break,
> > which is a bit unconventional but then so is the check itself.
>
> How about if I add a new patch introducing a comment documenting
> formally the possible states of *t with the libxl__xs_transaction
> functions ?
That would be good.
> And/or perhaps rename libxl__xs_transaction_abort to
> libxl__xs_transaction_cleanup ?
This might encourage people to think they have to call it unnecessarily
when they are following the regular pattern in the comment without the
exception which exists in this case.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (14 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:16 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases Ian Jackson
` (3 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
Replace the use of a loop with usleep().
Instead, use a xenstore watch and an event system timeout. xenstore
fires watches on @releaseDomain when a domain shuts down.
The logic which checks for the state of the domain is unchanged, and
not ideal, but we will leave that for the next patch.
There is not intended to be any semantic change, other than to make
the algorithm properly asynchronous and the consequential waiting on
xenstore, rather than polling.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 80 ++++++++++++++++++++++++++++++------------
tools/libxl/libxl_internal.h | 2 ++
2 files changed, 60 insertions(+), 22 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index dde7e33..6291115 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1028,8 +1028,14 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
libxl__domain_suspend_state *dss);
static void domain_suspend_common_guest_suspended(libxl__egc *egc,
libxl__domain_suspend_state *dss);
+
static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
libxl__xswait_state *xswa, int rc, const char *state);
+static void suspend_common_wait_guest_watch(libxl__egc *egc,
+ libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path);
+static void suspend_common_wait_guest_timeout(libxl__egc *egc,
+ libxl__ev_time *ev, const struct timeval *requested_abs);
+
static void domain_suspend_common_failed(libxl__egc *egc,
libxl__domain_suspend_state *dss);
static void domain_suspend_common_done(libxl__egc *egc,
@@ -1175,36 +1181,59 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
libxl__domain_suspend_state *dss)
{
STATE_AO_GC(dss->ao);
+ int rc;
+
+ LOG(DEBUG, "wait for the guest to suspend");
+
+ rc = libxl__ev_xswatch_register(gc, &dss->guest_watch,
+ suspend_common_wait_guest_watch,
+ "@releaseDomain");
+ if (rc) goto err;
+
+ rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
+ suspend_common_wait_guest_timeout,
+ 60*1000);
+ if (rc) goto err;
+ return;
+
+ err:
+ domain_suspend_common_failed(egc, dss);
+}
+
+static void suspend_common_wait_guest_watch(libxl__egc *egc,
+ libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path)
+{
+ libxl__domain_suspend_state *dss =
+ CONTAINER_OF(xsw, *dss, guest_watch);
+ STATE_AO_GC(dss->ao);
+ xc_domaininfo_t info;
int ret;
- int watchdog;
/* Convenience aliases */
const uint32_t domid = dss->domid;
- 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");
- domain_suspend_common_guest_suspended(egc, dss);
- return;
- }
+ 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");
+ domain_suspend_common_guest_suspended(egc, dss);
+ return;
}
-
- watchdog--;
}
+ /* otherwise, keep waiting */
+}
- LOG(ERROR, "guest did not suspend");
+static void suspend_common_wait_guest_timeout(libxl__egc *egc,
+ libxl__ev_time *ev, const struct timeval *requested_abs)
+{
+ libxl__domain_suspend_state *dss = CONTAINER_OF(ev, *dss, guest_timeout);
+ STATE_AO_GC(dss->ao);
+ LOG(ERROR, "guest did not suspend, timed out");
domain_suspend_common_failed(egc, dss);
}
@@ -1214,6 +1243,9 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
STATE_AO_GC(dss->ao);
int ret;
+ libxl__ev_xswatch_deregister(gc, &dss->guest_watch);
+ libxl__ev_time_deregister(gc, &dss->guest_timeout);
+
if (dss->hvm) {
ret = libxl__domain_suspend_device_model(gc, dss);
if (ret) {
@@ -1237,6 +1269,8 @@ static void domain_suspend_common_done(libxl__egc *egc,
{
EGC_GC;
assert(!libxl__xswait_inuse(&dss->pvcontrol));
+ libxl__ev_xswatch_deregister(gc, &dss->guest_watch);
+ libxl__ev_time_deregister(gc, &dss->guest_timeout);
dss->callback_common_done(egc, dss, ok);
}
@@ -1423,6 +1457,8 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
logdirty_init(&dss->logdirty);
libxl__xswait_init(&dss->pvcontrol);
+ libxl__ev_xswatch_init(&dss->guest_watch);
+ libxl__ev_time_init(&dss->guest_timeout);
switch (type) {
case LIBXL_DOMAIN_TYPE_HVM: {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5ac02cb..7ad15b5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2423,6 +2423,8 @@ struct libxl__domain_suspend_state {
int xcflags;
int guest_responded;
libxl__xswait_state pvcontrol;
+ libxl__ev_xswatch guest_watch;
+ libxl__ev_time guest_timeout;
const char *dm_savefile;
int interval; /* checkpoint interval (for Remus) */
libxl__save_helper_state shs;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-04 14:56 ` [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait Ian Jackson
@ 2014-03-13 17:16 ` Ian Campbell
2014-03-13 18:29 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:16 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> Replace the use of a loop with usleep().
>
> Instead, use a xenstore watch and an event system timeout. xenstore
> fires watches on @releaseDomain when a domain shuts down.
>
> The logic which checks for the state of the domain is unchanged, and
> not ideal, but we will leave that for the next patch.
>
> There is not intended to be any semantic change, other than to make
> the algorithm properly asynchronous and the consequential waiting on
> xenstore, rather than polling.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_dom.c | 80 ++++++++++++++++++++++++++++++------------
> tools/libxl/libxl_internal.h | 2 ++
> 2 files changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index dde7e33..6291115 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1028,8 +1028,14 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
> libxl__domain_suspend_state *dss);
> static void domain_suspend_common_guest_suspended(libxl__egc *egc,
> libxl__domain_suspend_state *dss);
> +
> static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
> libxl__xswait_state *xswa, int rc, const char *state);
> +static void suspend_common_wait_guest_watch(libxl__egc *egc,
> + libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path);
> +static void suspend_common_wait_guest_timeout(libxl__egc *egc,
> + libxl__ev_time *ev, const struct timeval *requested_abs);
> +
> static void domain_suspend_common_failed(libxl__egc *egc,
> libxl__domain_suspend_state *dss);
> static void domain_suspend_common_done(libxl__egc *egc,
> @@ -1175,36 +1181,59 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
> libxl__domain_suspend_state *dss)
> {
> STATE_AO_GC(dss->ao);
> + int rc;
> +
> + LOG(DEBUG, "wait for the guest to suspend");
> +
> + rc = libxl__ev_xswatch_register(gc, &dss->guest_watch,
> + suspend_common_wait_guest_watch,
> + "@releaseDomain");
> + if (rc) goto err;
> +
> + rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
> + suspend_common_wait_guest_timeout,
> + 60*1000);
This smells a lot like the xswait stuff you introduced towards the
beginning of the series.
Is the difference just that you cannot read the "path" @releaseDomain?
If so can we paper over that somehow in the helper?
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-13 17:16 ` Ian Campbell
@ 2014-03-13 18:29 ` Ian Jackson
2014-03-14 10:10 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-13 18:29 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > Replace the use of a loop with usleep().
...
> > + rc = libxl__ev_xswatch_register(gc, &dss->guest_watch,
> > + suspend_common_wait_guest_watch,
> > + "@releaseDomain");
> > + if (rc) goto err;
> > +
> > + rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
> > + suspend_common_wait_guest_timeout,
> > + 60*1000);
>
> This smells a lot like the xswait stuff you introduced towards the
> beginning of the series.
>
> Is the difference just that you cannot read the "path" @releaseDomain?
Precisely.
> If so can we paper over that somehow in the helper?
Not without complicating its interface, I think. We would really want
to prevent the @releaseDomain "path" being read at all.
I guess we could special-case paths starting with "@", the way
xenstore does, and always return ENOENT. I'm not sure whether this
wrinkle (which would end up in the docs for the general facility) is
worth the saving in the one call site where it would be relevant.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-13 18:29 ` Ian Jackson
@ 2014-03-14 10:10 ` Ian Campbell
2014-03-14 17:28 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 10:10 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Thu, 2014-03-13 at 18:29 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> > On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > > Replace the use of a loop with usleep().
> ...
> > > + rc = libxl__ev_xswatch_register(gc, &dss->guest_watch,
> > > + suspend_common_wait_guest_watch,
> > > + "@releaseDomain");
> > > + if (rc) goto err;
> > > +
> > > + rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
> > > + suspend_common_wait_guest_timeout,
> > > + 60*1000);
> >
> > This smells a lot like the xswait stuff you introduced towards the
> > beginning of the series.
> >
> > Is the difference just that you cannot read the "path" @releaseDomain?
>
> Precisely.
>
> > If so can we paper over that somehow in the helper?
>
> Not without complicating its interface, I think. We would really want
> to prevent the @releaseDomain "path" being read at all.
>
> I guess we could special-case paths starting with "@", the way
> xenstore does, and always return ENOENT.
I wonder what xs_read on @releaseDomain actually does -- I'd not be
surprised if it was ENOENT. I wonder even more what xs_write on
@releaseDomain would do, hopefully "bugger off" but you never know.
> I'm not sure whether this
> wrinkle (which would end up in the docs for the general facility) is
> worth the saving in the one call site where it would be relevant.
I'm not sure either, I'll leave it to your best judgement. If you want
to stick with this variant then:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-14 10:10 ` Ian Campbell
@ 2014-03-14 17:28 ` Ian Jackson
2014-03-14 17:39 ` Ian Campbell
2014-03-14 17:41 ` Ian Jackson
0 siblings, 2 replies; 70+ messages in thread
From: Ian Jackson @ 2014-03-14 17:28 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> On Thu, 2014-03-13 at 18:29 +0000, Ian Jackson wrote:
> > I guess we could special-case paths starting with "@", the way
> > xenstore does, and always return ENOENT.
>
> I wonder what xs_read on @releaseDomain actually does -- I'd not be
> surprised if it was ENOENT. I wonder even more what xs_write on
> @releaseDomain would do, hopefully "bugger off" but you never know.
Reading xenstored_core.c suggests both read and write give EINVAL. So
making xswait work for it would indeed involve special-casing @ in
libxl.
> > I'm not sure whether this
> > wrinkle (which would end up in the docs for the general facility) is
> > worth the saving in the one call site where it would be relevant.
>
> I'm not sure either, I'll leave it to your best judgement. If you want
> to stick with this variant then:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
I think it's probably best but I don't know if that's just inertia...
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-14 17:28 ` Ian Jackson
@ 2014-03-14 17:39 ` Ian Campbell
2014-03-14 17:41 ` Ian Jackson
1 sibling, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 17:39 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Fri, 2014-03-14 at 17:28 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> > On Thu, 2014-03-13 at 18:29 +0000, Ian Jackson wrote:
> > > I guess we could special-case paths starting with "@", the way
> > > xenstore does, and always return ENOENT.
> >
> > I wonder what xs_read on @releaseDomain actually does -- I'd not be
> > surprised if it was ENOENT. I wonder even more what xs_write on
> > @releaseDomain would do, hopefully "bugger off" but you never know.
>
> Reading xenstored_core.c suggests both read and write give EINVAL. So
> making xswait work for it would indeed involve special-casing @ in
> libxl.
Right.
> > > I'm not sure whether this
> > > wrinkle (which would end up in the docs for the general facility) is
> > > worth the saving in the one call site where it would be relevant.
> >
> > I'm not sure either, I'll leave it to your best judgement. If you want
> > to stick with this variant then:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> I think it's probably best but I don't know if that's just inertia...
Well, it's not like we'd be stuck with it if we change our minds later.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-14 17:28 ` Ian Jackson
2014-03-14 17:39 ` Ian Campbell
@ 2014-03-14 17:41 ` Ian Jackson
2014-03-14 17:46 ` Ian Campbell
1 sibling, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-14 17:41 UTC (permalink / raw)
To: Ian Campbell, xen-devel, Lai Jiangshan, Shriram Rajagopalan,
Stefano Stabellini
Ian Jackson writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> Reading xenstored_core.c suggests both read and write give EINVAL. So
> making xswait work for it would indeed involve special-casing @ in
> libxl.
I looked at how this would seem in the API and it's very simple. See
below.
I think I should probably add this to my series and use it in the
later patch.
Ian.
>From 41abced84e669449a926ca38a78194a2188c7cff Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Fri, 14 Mar 2014 17:38:38 +0000
Subject: [PATCH] libxl: events: libxl__xswait* support @paths
Special-case paths starting with '@' in libxl__xswait. Attempting to
read these from xenstore gives EINVAL. Callers waiting for (say)
@releaseDomain will be checking for some condition which can be
observed other than by looking at xenstore.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/libxl/libxl_aoutils.c | 8 ++++++--
tools/libxl/libxl_internal.h | 2 ++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 477717b..1c9eb9e 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -69,8 +69,12 @@ void xswait_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *xsw,
int rc;
const char *data;
- rc = libxl__xs_read_checked(gc, XBT_NULL, xswa->path, &data);
- if (rc) { xswait_report_error(egc, xswa, rc); return; }
+ if (xswa->path[0] == '@') {
+ data = 0;
+ } else {
+ rc = libxl__xs_read_checked(gc, XBT_NULL, xswa->path, &data);
+ if (rc) { xswait_report_error(egc, xswa, rc); return; }
+ }
xswa->callback(egc, xswa, 0, data);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 644fcee..dc3d34c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1110,6 +1110,8 @@ typedef struct libxl__xswait_state libxl__xswait_state;
* This HAS been logged.
* xswait will not continue (but calling libxl__xswait_stop is OK).
*
+ * xswait.path may start with with '@', in which case no read is done
+ * and the callback will always get data==0.
*/
typedef void libxl__xswait_callback(libxl__egc *egc,
libxl__xswait_state *xswa, int rc, const char *data);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-14 17:41 ` Ian Jackson
@ 2014-03-14 17:46 ` Ian Campbell
2014-03-14 18:16 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 17:46 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Fri, 2014-03-14 at 17:41 +0000, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> > Reading xenstored_core.c suggests both read and write give EINVAL. So
> > making xswait work for it would indeed involve special-casing @ in
> > libxl.
>
> I looked at how this would seem in the API and it's very simple. See
> below.
>
> I think I should probably add this to my series and use it in the
> later patch.
>
> Ian.
>
> From 41abced84e669449a926ca38a78194a2188c7cff Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Fri, 14 Mar 2014 17:38:38 +0000
> Subject: [PATCH] libxl: events: libxl__xswait* support @paths
>
> Special-case paths starting with '@' in libxl__xswait. Attempting to
> read these from xenstore gives EINVAL. Callers waiting for (say)
> @releaseDomain will be checking for some condition which can be
> observed other than by looking at xenstore.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(including if you fold it into the original xswait patch)
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-14 17:46 ` Ian Campbell
@ 2014-03-14 18:16 ` Ian Jackson
2014-03-17 9:55 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-14 18:16 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> On Fri, 2014-03-14 at 17:41 +0000, Ian Jackson wrote:
> > Special-case paths starting with '@' in libxl__xswait. Attempting to
> > read these from xenstore gives EINVAL. Callers waiting for (say)
> > @releaseDomain will be checking for some condition which can be
> > observed other than by looking at xenstore.
> >
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks.
> (including if you fold it into the original xswait patch)
I think it's marginally clearer separately and there's no point
folding it in.
I've done the update to the patch which will use this ("libxl:
suspend: Abolish usleeps in domain suspend wait") and it builds but I
want to test it a bit before I formally resend.
But for your edification, it's below.
Ian.
>From d337d7cf831360b0e7ed8f672db6b2559d063271 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Fri, 6 Dec 2013 16:12:44 +0000
Subject: [PATCH 18/21] libxl: suspend: Abolish usleeps in domain suspend wait
Replace the use of a loop with usleep().
Instead, use an xswait xenstore watch with timeout. (xenstore fires
watches on @releaseDomain when a domain shuts down.)
The logic which checks for the state of the domain is unchanged, and
not ideal, but we will leave that for the next patch.
There is not intended to be any semantic change, other than to make
the algorithm properly asynchronous and the consequential waiting be
on xenstore, rather than polling.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
v3: Use an xswait instead of separate watch and timeout.
Improve commit message.
Remove some trailing whitespace
---
tools/libxl/libxl_dom.c | 74 +++++++++++++++++++++++++++++++----------------
1 file changed, 49 insertions(+), 25 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 66c84d2..7b78c88 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1028,8 +1028,12 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
libxl__domain_suspend_state *dss);
static void domain_suspend_common_guest_suspended(libxl__egc *egc,
libxl__domain_suspend_state *dss);
+
static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
libxl__xswait_state *xswa, int rc, const char *state);
+static void suspend_common_wait_guest_watch(libxl__egc *egc,
+ libxl__xswait_state *xswa, int rc, const char *data_dummy);
+
static void domain_suspend_common_failed(libxl__egc *egc,
libxl__domain_suspend_state *dss);
static void domain_suspend_common_done(libxl__egc *egc,
@@ -1175,37 +1179,55 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
libxl__domain_suspend_state *dss)
{
STATE_AO_GC(dss->ao);
+ int rc;
+
+ LOG(DEBUG, "wait for the guest to suspend");
+
+ dss->guest_wait.ao = ao;
+ dss->guest_wait.what = "guest to suspend";
+ dss->guest_wait.path = "@releaseDomain";
+ dss->guest_wait.timeout_ms = 60*1000;
+ dss->guest_wait.callback = suspend_common_wait_guest_watch;
+ rc = libxl__xswait_start(gc, &dss->guest_wait);
+ if (rc) goto err;
+ return;
+
+ err:
+ domain_suspend_common_failed(egc, dss);
+}
+
+static void suspend_common_wait_guest_watch(libxl__egc *egc,
+ libxl__xswait_state *xswa, int rc, const char *data_dummy)
+{
+ libxl__domain_suspend_state *dss = CONTAINER_OF(xswa, *dss, guest_wait);
+ STATE_AO_GC(dss->ao);
+ xc_domaininfo_t info;
int ret;
- int watchdog;
+
+ if (rc) {
+ if (rc == ERROR_TIMEDOUT)
+ LOG(ERROR, "guest did not suspend, timed out");
+ domain_suspend_common_failed(egc, dss);
+ return;
+ }
/* Convenience aliases */
const uint32_t domid = dss->domid;
- 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");
- domain_suspend_common_guest_suspended(egc, dss);
- return;
- }
+ 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");
+ domain_suspend_common_guest_suspended(egc, dss);
+ return;
}
-
- watchdog--;
}
-
- LOG(ERROR, "guest did not suspend");
- domain_suspend_common_failed(egc, dss);
+ /* otherwise, keep waiting */
}
static void domain_suspend_common_guest_suspended(libxl__egc *egc,
@@ -1214,6 +1236,8 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
STATE_AO_GC(dss->ao);
int ret;
+ libxl__xswait_stop(gc, &dss->guest_wait);
+
if (dss->hvm) {
ret = libxl__domain_suspend_device_model(gc, dss);
if (ret) {
@@ -1236,7 +1260,7 @@ static void domain_suspend_common_done(libxl__egc *egc,
bool ok)
{
EGC_GC;
- assert(!libxl__xswait_inuse(&dss->guest_wait));
+ libxl__xswait_stop(gc,&dss->guest_wait);
dss->callback_common_done(egc, dss, ok);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-14 18:16 ` Ian Jackson
@ 2014-03-17 9:55 ` Ian Campbell
2014-03-17 11:55 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-17 9:55 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Fri, 2014-03-14 at 18:16 +0000, Ian Jackson wrote:
> I think it's marginally clearer separately and there's no point
> folding it in.
OK.
[...]
> From d337d7cf831360b0e7ed8f672db6b2559d063271 Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Fri, 6 Dec 2013 16:12:44 +0000
> Subject: [PATCH 18/21] libxl: suspend: Abolish usleeps in domain suspend wait
>
> Replace the use of a loop with usleep().
>
> Instead, use an xswait xenstore watch with timeout. (xenstore fires
> watches on @releaseDomain when a domain shuts down.)
>
> The logic which checks for the state of the domain is unchanged, and
> not ideal, but we will leave that for the next patch.
>
> There is not intended to be any semantic change, other than to make
> the algorithm properly asynchronous and the consequential waiting be
> on xenstore, rather than polling.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-17 9:55 ` Ian Campbell
@ 2014-03-17 11:55 ` Ian Jackson
2014-03-17 11:58 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-17 11:55 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> On Fri, 2014-03-14 at 18:16 +0000, Ian Jackson wrote:
>
> > I think it's marginally clearer separately and there's no point
> > folding it in.
>
> OK.
So it turns out that doing this makes the final patch quite fiddly and
annoying: it needs to have a separate timeout anyway because it's also
waiting for event channels. Making the two paths separate in this way
produces an extra callback function etc.
So I'm going to go back to the previous approach. But I guess I might
as well leave the patch "libxl: events: libxl__xswait* support @paths"
in the series even though nothing uses that right now.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait
2014-03-17 11:55 ` Ian Jackson
@ 2014-03-17 11:58 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-17 11:58 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Mon, 2014-03-17 at 11:55 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait"):
> > On Fri, 2014-03-14 at 18:16 +0000, Ian Jackson wrote:
> >
> > > I think it's marginally clearer separately and there's no point
> > > folding it in.
> >
> > OK.
>
> So it turns out that doing this makes the final patch quite fiddly and
> annoying: it needs to have a separate timeout anyway because it's also
> waiting for event channels. Making the two paths separate in this way
> produces an extra callback function etc.
OK, not to worry.
> So I'm going to go back to the previous approach. But I guess I might
> as well leave the patch "libxl: events: libxl__xswait* support @paths"
> in the series even though nothing uses that right now.
Agreed.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (15 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 16/19] libxl: suspend: Abolish usleeps in domain suspend wait Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:18 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 18/19] libxl: suspend: Async evtchn wait Ian Jackson
` (2 subsequent siblings)
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
When we are waiting for a guest to suspend, this suspend operation
would continue to wait (until the timeout) if the guest was destroyed
or shut down for another reason, or if xc_domain_getinfolist failed.
Handle these cases correctly, as errors.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 42 ++++++++++++++++++++++++++++++------------
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 6291115..88700ee 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1208,24 +1208,42 @@ static void suspend_common_wait_guest_watch(libxl__egc *egc,
STATE_AO_GC(dss->ao);
xc_domaininfo_t info;
int ret;
+ int shutdown_reason;
/* Convenience aliases */
const uint32_t domid = dss->domid;
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");
- domain_suspend_common_guest_suspended(egc, dss);
- return;
- }
+ if (ret < 0) {
+ LOGE(ERROR, "unable to check for status of guest %"PRId32"", domid);
+ goto err;
+ domain_suspend_common_failed(egc, dss);
+ }
+
+ if (!(ret == 1 && info.domain == domid)) {
+ LOGE(ERROR, "guest %"PRId32" we were suspending has been destroyed",
+ domid);
+ goto err;
+ }
+
+ if (!(info.flags & XEN_DOMINF_shutdown))
+ /* keep waiting */
+ return;
+
+ shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
+ & XEN_DOMINF_shutdownmask;
+ if (shutdown_reason != SHUTDOWN_suspend) {
+ LOG(DEBUG, "guest %"PRId32" we were suspending has shut down"
+ " with unexpected reason code %d", domid, shutdown_reason);
+ goto err;
}
- /* otherwise, keep waiting */
+
+ LOG(DEBUG, "guest has suspended");
+ domain_suspend_common_guest_suspended(egc, dss);
+ return;
+
+ err:
+ domain_suspend_common_failed(egc, dss);
}
static void suspend_common_wait_guest_timeout(libxl__egc *egc,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases
2014-03-04 14:56 ` [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases Ian Jackson
@ 2014-03-13 17:18 ` Ian Campbell
2014-03-13 18:33 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:18 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> When we are waiting for a guest to suspend, this suspend operation
> would continue to wait (until the timeout) if the guest was destroyed
> or shut down for another reason, or if xc_domain_getinfolist failed.
>
> Handle these cases correctly, as errors.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_dom.c | 42 ++++++++++++++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 6291115..88700ee 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1208,24 +1208,42 @@ static void suspend_common_wait_guest_watch(libxl__egc *egc,
> STATE_AO_GC(dss->ao);
> xc_domaininfo_t info;
> int ret;
> + int shutdown_reason;
>
> /* Convenience aliases */
> const uint32_t domid = dss->domid;
>
> 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");
> - domain_suspend_common_guest_suspended(egc, dss);
> - return;
> - }
> + if (ret < 0) {
> + LOGE(ERROR, "unable to check for status of guest %"PRId32"", domid);
> + goto err;
> + domain_suspend_common_failed(egc, dss);
You don't want this here.
> + }
> +
> + if (!(ret == 1 && info.domain == domid)) {
> + LOGE(ERROR, "guest %"PRId32" we were suspending has been destroyed",
> + domid);
Is there an (unlikely) race here where a new domain gets created with
the same domid? Not that I have any suggestion what to do about that...
> + goto err;
> + }
> +
> + if (!(info.flags & XEN_DOMINF_shutdown))
> + /* keep waiting */
> + return;
> +
> + shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
> + & XEN_DOMINF_shutdownmask;
> + if (shutdown_reason != SHUTDOWN_suspend) {
> + LOG(DEBUG, "guest %"PRId32" we were suspending has shut down"
> + " with unexpected reason code %d", domid, shutdown_reason);
> + goto err;
> }
> - /* otherwise, keep waiting */
> +
> + LOG(DEBUG, "guest has suspended");
> + domain_suspend_common_guest_suspended(egc, dss);
> + return;
> +
> + err:
> + domain_suspend_common_failed(egc, dss);
> }
>
> static void suspend_common_wait_guest_timeout(libxl__egc *egc,
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases
2014-03-13 17:18 ` Ian Campbell
@ 2014-03-13 18:33 ` Ian Jackson
2014-03-14 10:20 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-13 18:33 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases"):
> On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > + if (ret < 0) {
> > + LOGE(ERROR, "unable to check for status of guest %"PRId32"", domid);
> > + goto err;
> > + domain_suspend_common_failed(egc, dss);
>
> You don't want this here.
Indeed I don't.
> > + if (!(ret == 1 && info.domain == domid)) {
> > + LOGE(ERROR, "guest %"PRId32" we were suspending has been destroyed",
> > + domid);
>
> Is there an (unlikely) race here where a new domain gets created with
> the same domid? Not that I have any suggestion what to do about that...
If domids are reused within the lifetime of the libxc/libxl code
managing the domain, the whole edifice is unsafe. AFAICT this is a
fundamental problem which cannot be avoided in any toolstack
which is capable of concurrently issuing hypercalls (specifically,
destroying domains).
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases
2014-03-13 18:33 ` Ian Jackson
@ 2014-03-14 10:20 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 10:20 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Thu, 2014-03-13 at 18:33 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases"):
> > On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > > + if (ret < 0) {
> > > + LOGE(ERROR, "unable to check for status of guest %"PRId32"", domid);
> > > + goto err;
> > > + domain_suspend_common_failed(egc, dss);
> >
> > You don't want this here.
>
> Indeed I don't.
>
> > > + if (!(ret == 1 && info.domain == domid)) {
> > > + LOGE(ERROR, "guest %"PRId32" we were suspending has been destroyed",
> > > + domid);
> >
> > Is there an (unlikely) race here where a new domain gets created with
> > the same domid? Not that I have any suggestion what to do about that...
>
> If domids are reused within the lifetime of the libxc/libxl code
> managing the domain, the whole edifice is unsafe. AFAICT this is a
> fundamental problem which cannot be avoided in any toolstack
> which is capable of concurrently issuing hypercalls (specifically,
> destroying domains).
Yes. Oh well.
With the unwanted dmain_suspend call removed:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 18/19] libxl: suspend: Async evtchn wait
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (16 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:23 ` Ian Campbell
2014-03-04 14:56 ` [PATCH 19/19] libxl: suspend: Apply guest timeout in evtchn case Ian Jackson
2014-03-11 8:55 ` [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Lai Jiangshan
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
When negotiating guest suspend via the evtchn ("fast") protocol,
abolish synchronous wait for domain suspend.
If the guest supports the event channel suspend protocol, we used to
sit in a loop waiting for it to suspend.
Instead, use the new libxl event channel event facility. When we see
that the event is signaled, we look at the domain to see if it has
suspended.
So the suspend operation no longer blocks with the libxl ctx lock
held, and instead returns to the event loop. Additionally, domains
which signal the event channel themselves, or undergo other state
changes, will be handled more correctly.
We end up making a few more hypercalls.
Also, if we encounter errors setting up the suspend event channel
(which should not happen), abort the operation rather than falling
back to the xenstore protocol.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 78 ++++++++++++++++++++++++++----------------
tools/libxl/libxl_internal.h | 3 +-
2 files changed, 50 insertions(+), 31 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 88700ee..c431a2d 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1031,8 +1031,12 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
libxl__xswait_state *xswa, int rc, const char *state);
+static void domain_suspend_common_wait_guest_evtchn(libxl__egc *egc,
+ libxl__ev_evtchn *evev);
static void suspend_common_wait_guest_watch(libxl__egc *egc,
libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path);
+static void suspend_common_wait_guest_check(libxl__egc *egc,
+ libxl__domain_suspend_state *dss);
static void suspend_common_wait_guest_timeout(libxl__egc *egc,
libxl__ev_time *ev, const struct timeval *requested_abs);
@@ -1054,7 +1058,7 @@ static void domain_suspend_callback_common(libxl__egc *egc,
{
STATE_AO_GC(dss->ao);
unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
- int ret;
+ int ret, rc;
/* Convenience aliases */
const uint32_t domid = dss->domid;
@@ -1064,21 +1068,19 @@ static void domain_suspend_callback_common(libxl__egc *egc,
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 ((hvm_s_state == 0) && (dss->guest_evtchn.port >= 0)) {
LOG(DEBUG, "issuing %s suspend request via event channel",
dss->hvm ? "PVHVM" : "PV");
- ret = xc_evtchn_notify(dss->xce, dss->suspend_eventchn);
+ ret = xc_evtchn_notify(CTX->xce, dss->guest_evtchn.port);
if (ret < 0) {
LOG(ERROR, "xc_evtchn_notify failed ret=%d", ret);
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);
- goto err;
- }
- dss->guest_responded = 1;
- domain_suspend_common_guest_suspended(egc, dss);
+
+ dss->guest_evtchn.callback = domain_suspend_common_wait_guest_evtchn;
+ rc = libxl__ev_evtchn_wait(gc, &dss->guest_evtchn);
+ if (rc) goto err;
+
return;
}
@@ -1114,6 +1116,19 @@ static void domain_suspend_callback_common(libxl__egc *egc,
domain_suspend_common_failed(egc, dss);
}
+static void domain_suspend_common_wait_guest_evtchn(libxl__egc *egc,
+ libxl__ev_evtchn *evev)
+{
+ libxl__domain_suspend_state *dss = CONTAINER_OF(evev, *dss, guest_evtchn);
+ STATE_AO_GC(dss->ao);
+ /* If we should be done waiting, suspend_common_wait_guest_check
+ * will end up calling domain_suspend_common_guest_suspended or
+ * domain_suspend_common_failed, both of which cancel the evtchn
+ * wait. So re-enable it now. */
+ libxl__ev_evtchn_wait(gc, &dss->guest_evtchn);
+ suspend_common_wait_guest_check(egc, dss);
+}
+
static void domain_suspend_common_pvcontrol_suspending(libxl__egc *egc,
libxl__xswait_state *xswa, int rc, const char *state)
{
@@ -1203,8 +1218,13 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
static void suspend_common_wait_guest_watch(libxl__egc *egc,
libxl__ev_xswatch *xsw, const char *watch_path, const char *event_path)
{
- libxl__domain_suspend_state *dss =
- CONTAINER_OF(xsw, *dss, guest_watch);
+ libxl__domain_suspend_state *dss = CONTAINER_OF(xsw, *dss, guest_watch);
+ suspend_common_wait_guest_check(egc, dss);
+}
+
+static void suspend_common_wait_guest_check(libxl__egc *egc,
+ libxl__domain_suspend_state *dss)
+{
STATE_AO_GC(dss->ao);
xc_domaininfo_t info;
int ret;
@@ -1261,6 +1281,7 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
STATE_AO_GC(dss->ao);
int ret;
+ libxl__ev_evtchn_cancel(gc, &dss->guest_evtchn);
libxl__ev_xswatch_deregister(gc, &dss->guest_watch);
libxl__ev_time_deregister(gc, &dss->guest_timeout);
@@ -1287,6 +1308,7 @@ static void domain_suspend_common_done(libxl__egc *egc,
{
EGC_GC;
assert(!libxl__xswait_inuse(&dss->pvcontrol));
+ libxl__ev_evtchn_cancel(gc, &dss->guest_evtchn);
libxl__ev_xswatch_deregister(gc, &dss->guest_watch);
libxl__ev_time_deregister(gc, &dss->guest_timeout);
dss->callback_common_done(egc, dss, ok);
@@ -1475,6 +1497,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
logdirty_init(&dss->logdirty);
libxl__xswait_init(&dss->pvcontrol);
+ libxl__ev_evtchn_init(&dss->guest_evtchn);
libxl__ev_xswatch_init(&dss->guest_watch);
libxl__ev_time_init(&dss->guest_timeout);
@@ -1503,7 +1526,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
| (debug ? XCFLAGS_DEBUG : 0)
| (dss->hvm ? XCFLAGS_HVM : 0);
- dss->suspend_eventchn = -1;
+ dss->guest_evtchn.port = -1;
dss->guest_evtchn_lockfd = -1;
dss->guest_responded = 0;
dss->dm_savefile = libxl__device_model_savefile(gc, domid);
@@ -1514,20 +1537,17 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
dss->xcflags |= XCFLAGS_CHECKPOINT_COMPRESS;
}
- dss->xce = xc_evtchn_open(NULL, 0);
- if (dss->xce == NULL)
- goto out;
- else
- {
- port = xs_suspend_evtchn_port(dss->domid);
+ port = xs_suspend_evtchn_port(dss->domid);
- if (port >= 0) {
- dss->suspend_eventchn =
- xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce,
+ if (port >= 0) {
+ dss->guest_evtchn.port =
+ xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce,
dss->domid, port, &dss->guest_evtchn_lockfd);
- if (dss->suspend_eventchn < 0)
- LOG(WARN, "Suspend event channel initialization failed");
+ if (dss->guest_evtchn.port < 0) {
+ LOG(WARN, "Suspend event channel initialization failed");
+ rc = ERROR_FAIL;
+ goto out;
}
}
@@ -1686,11 +1706,11 @@ static void domain_suspend_done(libxl__egc *egc,
/* Convenience aliases */
const uint32_t domid = dss->domid;
- if (dss->suspend_eventchn > 0)
- xc_suspend_evtchn_release(CTX->xch, dss->xce, domid,
- dss->suspend_eventchn, &dss->guest_evtchn_lockfd);
- if (dss->xce != NULL)
- xc_evtchn_close(dss->xce);
+ libxl__ev_evtchn_cancel(gc, &dss->guest_evtchn);
+
+ if (dss->guest_evtchn.port > 0)
+ xc_suspend_evtchn_release(CTX->xch, CTX->xce, domid,
+ dss->guest_evtchn.port, &dss->guest_evtchn_lockfd);
dss->callback(egc, dss, rc);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7ad15b5..ceb5af7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2416,8 +2416,7 @@ struct libxl__domain_suspend_state {
int debug;
const libxl_domain_remus_info *remus;
/* private */
- xc_evtchn *xce; /* event channel handle */
- int suspend_eventchn;
+ libxl__ev_evtchn guest_evtchn;
int guest_evtchn_lockfd;
int hvm;
int xcflags;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 18/19] libxl: suspend: Async evtchn wait
2014-03-04 14:56 ` [PATCH 18/19] libxl: suspend: Async evtchn wait Ian Jackson
@ 2014-03-13 17:23 ` Ian Campbell
2014-03-13 18:36 ` Ian Jackson
0 siblings, 1 reply; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:23 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> When negotiating guest suspend via the evtchn ("fast") protocol,
> abolish synchronous wait for domain suspend.
>
> If the guest supports the event channel suspend protocol, we used to
> sit in a loop waiting for it to suspend.
^ in xc_await_suspend
(I was looking for a loop being removed here)
> Instead, use the new libxl event channel event facility. When we see
> that the event is signaled, we look at the domain to see if it has
> suspended.
I was looking for a timeout case, but I see now that this is in the next
patch.
> So the suspend operation no longer blocks with the libxl ctx lock
> held, and instead returns to the event loop. Additionally, domains
> which signal the event channel themselves, or undergo other state
> changes, will be handled more correctly.
>
> We end up making a few more hypercalls.
>
> Also, if we encounter errors setting up the suspend event channel
> (which should not happen), abort the operation rather than falling
> back to the xenstore protocol.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Neither of the above comments necessitate a change (although you could
clarify the commit message if you were so inclined):
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 18/19] libxl: suspend: Async evtchn wait
2014-03-13 17:23 ` Ian Campbell
@ 2014-03-13 18:36 ` Ian Jackson
2014-03-14 10:21 ` Ian Campbell
0 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-13 18:36 UTC (permalink / raw)
To: Ian Campbell
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH 18/19] libxl: suspend: Async evtchn wait"):
> On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > When negotiating guest suspend via the evtchn ("fast") protocol,
> > abolish synchronous wait for domain suspend.
>
> [stuff]
>
> Neither of the above comments necessitate a change (although you could
> clarify the commit message if you were so inclined):
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks. Good idea. Here's the new one.
Ian.
libxl: suspend: Async evtchn wait
When negotiating guest suspend via the evtchn ("fast") protocol,
abolish synchronous wait for domain suspend.
If the guest supports the event channel suspend protocol, we used to
sit in a loop in xc_await_suspend waiting (perhaps indefinitely) for
it to suspend.
Instead, use the new libxl event channel event facility. When we see
that the event is signaled, we look at the domain to see if it has
suspended. (In this patch we do not yet set a timeout; that will come
next.)
So the suspend operation no longer blocks with the libxl ctx lock
held, and instead returns to the event loop. Additionally, domains
which signal the event channel themselves, or undergo other state
changes, will be handled more correctly.
We end up making a few more hypercalls.
Also, if we encounter errors setting up the suspend event channel
(which should not happen), abort the operation rather than falling
back to the xenstore protocol.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
v4: Improve commit message.
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 18/19] libxl: suspend: Async evtchn wait
2014-03-13 18:36 ` Ian Jackson
@ 2014-03-14 10:21 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-14 10:21 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Thu, 2014-03-13 at 18:36 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 18/19] libxl: suspend: Async evtchn wait"):
> > On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > > When negotiating guest suspend via the evtchn ("fast") protocol,
> > > abolish synchronous wait for domain suspend.
> >
> > [stuff]
> >
> > Neither of the above comments necessitate a change (although you could
> > clarify the commit message if you were so inclined):
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Thanks. Good idea. Here's the new one.
Looks good, thanks.
Ian.
>
> Ian.
>
> libxl: suspend: Async evtchn wait
>
> When negotiating guest suspend via the evtchn ("fast") protocol,
> abolish synchronous wait for domain suspend.
>
> If the guest supports the event channel suspend protocol, we used to
> sit in a loop in xc_await_suspend waiting (perhaps indefinitely) for
> it to suspend.
>
> Instead, use the new libxl event channel event facility. When we see
> that the event is signaled, we look at the domain to see if it has
> suspended. (In this patch we do not yet set a timeout; that will come
> next.)
>
> So the suspend operation no longer blocks with the libxl ctx lock
> held, and instead returns to the event loop. Additionally, domains
> which signal the event channel themselves, or undergo other state
> changes, will be handled more correctly.
>
> We end up making a few more hypercalls.
>
> Also, if we encounter errors setting up the suspend event channel
> (which should not happen), abort the operation rather than falling
> back to the xenstore protocol.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v4: Improve commit message.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 19/19] libxl: suspend: Apply guest timeout in evtchn case
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (17 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 18/19] libxl: suspend: Async evtchn wait Ian Jackson
@ 2014-03-04 14:56 ` Ian Jackson
2014-03-13 17:23 ` Ian Campbell
2014-03-11 8:55 ` [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Lai Jiangshan
19 siblings, 1 reply; 70+ messages in thread
From: Ian Jackson @ 2014-03-04 14:56 UTC (permalink / raw)
To: xen-devel
Cc: Shriram Rajagopalan, Stefano Stabellini, Ian Jackson,
Ian Campbell, Lai Jiangshan
When negotiating guest suspend via the evtchn ("fast") protocol,
the guest may still fail to respond.
So set the timeout. The existing error path will already properly
tear down our (event channel) wait.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dom.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c431a2d..820ec3a 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1081,6 +1081,11 @@ static void domain_suspend_callback_common(libxl__egc *egc,
rc = libxl__ev_evtchn_wait(gc, &dss->guest_evtchn);
if (rc) goto err;
+ rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
+ suspend_common_wait_guest_timeout,
+ 60*1000);
+ if (rc) goto err;
+
return;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 19/19] libxl: suspend: Apply guest timeout in evtchn case
2014-03-04 14:56 ` [PATCH 19/19] libxl: suspend: Apply guest timeout in evtchn case Ian Jackson
@ 2014-03-13 17:23 ` Ian Campbell
0 siblings, 0 replies; 70+ messages in thread
From: Ian Campbell @ 2014-03-13 17:23 UTC (permalink / raw)
To: Ian Jackson
Cc: Shriram Rajagopalan, xen-devel, Lai Jiangshan, Stefano Stabellini
On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> When negotiating guest suspend via the evtchn ("fast") protocol,
> the guest may still fail to respond.
>
> So set the timeout. The existing error path will already properly
> tear down our (event channel) wait.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend
2014-03-04 14:56 [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Ian Jackson
` (18 preceding siblings ...)
2014-03-04 14:56 ` [PATCH 19/19] libxl: suspend: Apply guest timeout in evtchn case Ian Jackson
@ 2014-03-11 8:55 ` Lai Jiangshan
2014-03-11 11:35 ` Ian Jackson
19 siblings, 1 reply; 70+ messages in thread
From: Lai Jiangshan @ 2014-03-11 8:55 UTC (permalink / raw)
To: Ian Jackson; +Cc: Shriram Rajagopalan, xen-devel, Ian Campbell
Ian Jackson
Should the remus netbuf&replicated-disks patchset be rebased on top of
this patchset?
Or I can send them as if this asynchronous suspend patchset is not applied.
Thanks,
Lai.
PS: git-am found some trailing whitespaces:
Applying: libxl: init: Provide a gc later in libxl_ctx_alloc
Applying: libxl: init: libxl__poller_init and _get take gc
Applying: libxl: events: const-correct *_inuse, *_isregistered
Applying: libxl: events: Provide libxl__xswait_*
Applying: libxl: events: Use libxl__xswait_* in spawn code
Applying: libxl: events: Provide libxl__ev_evtchn*
Applying: libxc: suspend: Rename, improve xc_suspend_evtchn_init
Applying: libxc: suspend: Fix suspend event channel locking
Applying: libxl: suspend: Async libxl__domain_suspend_callback
/home/laijs/work/xen/.git/rebase-apply/patch:87: trailing whitespace.
[ 3, 'scxA', "suspend", [] ],
warning: 1 line adds whitespace errors.
Applying: libxl: suspend: Async domain_suspend_callback_common
/home/laijs/work/xen/.git/rebase-apply/patch:102: trailing whitespace.
}
/home/laijs/work/xen/.git/rebase-apply/patch:123: trailing whitespace.
}
warning: 2 lines add whitespace errors.
Applying: libxl: suspend: Reorg domain_suspend_callback_common
Applying: libxl: suspend: New libxl__domain_pvcontrol_xspath
Applying: libxl: suspend: New domain_suspend_pvcontrol_acked
Applying: libxl: suspend: domain_suspend_callback_common xs errs
Applying: libxl: suspend: Async xenstore pvcontrol wait
Applying: libxl: suspend: Abolish usleeps in domain suspend wait
/home/laijs/work/xen/.git/rebase-apply/patch:38: trailing whitespace.
rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
warning: 1 line adds whitespace errors.
Applying: libxl: suspend: Fix suspend wait corner cases
Applying: libxl: suspend: Async evtchn wait
Applying: libxl: suspend: Apply guest timeout in evtchn case
On 03/04/2014 10:56 PM, Ian Jackson wrote:
> This series removes the usleeps and waiting loops in libxl_dom.c and
> replaces them with event-callback code.
>
> We want some additional libxl event facilities:
> 01/19 libxl: init: Provide a gc later in libxl_ctx_alloc
> 02/19 libxl: init: libxl__poller_init and _get take gc
> a 03/19 libxl: events: const-correct *_inuse, *_isregistered
> - 04/19 libxl: events: Provide libxl__xswait_*
> a 05/19 libxl: events: Use libxl__xswait_* in spawn code
> + 06/19 libxl: events: Provide libxl__ev_evtchn*
>
> We need to clean up some unfortunate code in libxc:
> 07/19 libxc: suspend: Rename, improve xc_suspend_evtchn_init
> 08/19 libxc: suspend: Fix suspend event channel locking
>
> We do some shuffling around of the libxl suspend control flow:
> - 09/19 libxl: suspend: Async libxl__domain_suspend_callback
> 10/19 libxl: suspend: Async domain_suspend_callback_common
> 11/19 libxl: suspend: Reorg domain_suspend_callback_common
> 12/19 libxl: suspend: New libxl__domain_pvcontrol_xspath
> 13/19 libxl: suspend: New domain_suspend_pvcontrol_acked
> No functional change in those five. These changes are broken down
> just to make the changes reviewable.
>
> Finally, we can start to work on the event code, removing the bugs,
> usleeps and loops one at a time:
> 14/19 libxl: suspend: domain_suspend_callback_common xs errs
> 15/19 libxl: suspend: Async xenstore pvcontrol wait
> 16/19 libxl: suspend: Abolish usleeps in domain suspend wait
> 17/19 libxl: suspend: Fix suspend wait corner cases
> 18/19 libxl: suspend: Async evtchn wait
> 19/19 libxl: suspend: Apply guest timeout in evtchn case
>
> Notes:
> + modified in v2
> - modified in v2 but only in commit message or comments
> a acked
>
> I have tested v2 with a Debian pvops kernel (xenstore pvcontrol
> suspend signalling) and OpenSUSE 11 (event channel suspend
> signalling). v2.1 is a rebase onto current staging.
>
> I haven't really touched the Remus-specific code here but this series
> ought to be suitable for the Remus developers to base things on.
>
>
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend
2014-03-11 8:55 ` [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend Lai Jiangshan
@ 2014-03-11 11:35 ` Ian Jackson
0 siblings, 0 replies; 70+ messages in thread
From: Ian Jackson @ 2014-03-11 11:35 UTC (permalink / raw)
To: Lai Jiangshan; +Cc: Shriram Rajagopalan, xen-devel, Ian Campbell
Lai Jiangshan writes ("Re: [PATCH v2.1 RESEND 00/19] libxl: asynchronous suspend"):
> Should the remus netbuf&replicated-disks patchset be rebased on top of
> this patchset?
I think that would be best, but I'd wait for an opinion from Ian
Campbell before doing the rebase. He's just got back and will
hopefully comment shortly...
> PS: git-am found some trailing whitespaces:
I'll fix this, thanks.
Ian.
^ permalink raw reply [flat|nested] 70+ messages in thread