|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH 4/8] libxl: event: Fix hang when mixing blocking and eventy calls
If the application calls libxl with ao_how==0 and also makes calls
like _occurred, libxl will sometimes get stuck.
The bug happens as follows (for example):
Thread A
libxl_do_thing(,ao_how==0)
libxl_do_thing starts, sets up some callbacks
libxl_do_thing exit path calls AO_INPROGRESS
libxl__ao_inprogress goes into event loop
eventloop_iteration sleeps on:
- do_thing's current fd set
- sigchld pipe if applicable
- its poller
Thread B
libxl_something_occurred
the something is to do with do_thing, above
do_thing_next_callback does some more work
do_thing_next_callback becomes interested in fd N
thread B returns to application
Note that nothing wakes up thread A. A is not listening on fd N. So
do_thing_* will not spot when fd N signals. do_thing will not make
further timely progress. If there is no timeout thread A will never
wake up.
The problem here occurs because thread A is waiting on an out of date
osevent set. We need the following property: whenever any thread is
blocking in the libxl event loop, at least one thread must be using an
up to date osevent set. It is OK for all but one threads to have
stale event sets, because so long as one waiting thread has the right
event set, any actually interesting event will, if nothing else, wake
that "right" thread up. It will then make some progress and/or, if it
exits, ensure that some other thread becomes the "right" thread.
There is also the possibility that a thread might block waiting for
libxl osevents but outside libxl, eg if the application used
libxl_osevent_beforepoll. We will deal with that in a moment.
Reported-by: George Dunlap <george.dunlap@xxxxxxxxxx>
Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
---
tools/libxl/libxl_event.c | 72 +++++++++++++++++++++++++++++++++++++-------
tools/libxl/libxl_internal.h | 33 ++++++++++++++++----
2 files changed, 88 insertions(+), 17 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index be37e12bb0..18db091a6e 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -36,6 +36,42 @@ static libxl__ao *ao_nested_root(libxl__ao *ao);
static void ao__check_destroy(libxl_ctx *ctx, libxl__ao *ao);
+static void pollers_note_osevent_added(libxl_ctx *ctx) {
+ libxl__poller *poller;
+ LIBXL_LIST_FOREACH(poller, &ctx->pollers_active, active_entry)
+ poller->osevents_added = 1;
+}
+
+void libxl__egc_cleanup_1_baton(libxl__egc *egc)
+{
+ EGC_GC;
+ libxl__poller *search, *wake=0;
+
+ LIBXL_LIST_FOREACH(search, &CTX->pollers_active, active_entry) {
+ if (search == CTX->poller_app)
+ /* This one is special. We can't give it the baton. */
+ continue;
+ if (!search->osevents_added)
+ /* This poller is up to date and will wake up as needed. */
+ return;
+ if (!wake)
+ wake = search;
+ }
+
+ if (!wake)
+ /* no-one in libxl waiting for any events */
+ return;
+
+ libxl__poller_wakeup(egc, wake);
+
+ wake->osevents_added = 0;
+ /* This serves to make _1_baton idempotent. It is OK even though
+ * that poller may currently be sleeping on only old osevents,
+ * because it is going to wake up because we've just prodded it,
+ * and it pick up new osevents on its next iteration (or pass
+ * on the baton). */
+}
+
/*
* The counter osevent_in_hook is used to ensure that the application
* honours the reentrancy restriction documented in libxl_event.h.
@@ -194,6 +230,7 @@ int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev,
ev->func = func;
LIBXL_LIST_INSERT_HEAD(&CTX->efds, ev, entry);
+ pollers_note_osevent_added(CTX);
rc = 0;
@@ -214,6 +251,8 @@ int libxl__ev_fd_modify(libxl__gc *gc, libxl__ev_fd *ev,
short events)
rc = OSEVENT_HOOK(fd,modify, noop, ev->fd, &ev->nexus->for_app_reg,
events);
if (rc) goto out;
+ if ((events & ~ev->events))
+ pollers_note_osevent_added(CTX);
ev->events = events;
rc = 0;
@@ -315,6 +354,7 @@ static int time_register_finite(libxl__gc *gc,
libxl__ev_time *ev,
LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch, /*empty*/,
timercmp(&ev->abs, &evsearch->abs, >));
+ pollers_note_osevent_added(CTX);
return 0;
}
@@ -1121,6 +1161,7 @@ static int beforepoll_internal(libxl__gc *gc,
libxl__poller *poller,
*nfds_io = used;
poller->fds_deregistered = 0;
+ poller->osevents_added = 0;
libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
if (etime) {
@@ -1444,7 +1485,7 @@ static void egc_run_callbacks(libxl__egc *egc)
}
}
-void libxl__egc_cleanup(libxl__egc *egc)
+void libxl__egc_cleanup_2_ul_cb_gc(libxl__egc *egc)
{
EGC_GC;
egc_run_callbacks(egc);
@@ -1754,13 +1795,15 @@ int libxl_event_wait(libxl_ctx *ctx, libxl_event
**event_r,
rc = eventloop_iteration(egc, poller);
if (rc) goto out;
- /* we unlock and cleanup the egc each time we go through this loop,
- * so that (a) we don't accumulate garbage and (b) any events
- * which are to be dispatched by callback are actually delivered
- * in a timely fashion.
+ /* we unlock and cleanup the egc each time we go through this
+ * loop, so that (a) we don't accumulate garbage and (b) any
+ * events which are to be dispatched by callback are actually
+ * delivered in a timely fashion. _1_baton will be
+ * called to pass the baton iff we actually leave; otherwise
+ * we are still carrying it.
*/
CTX_UNLOCK;
- libxl__egc_cleanup(egc);
+ libxl__egc_cleanup_2_ul_cb_gc(egc);
CTX_LOCK;
}
@@ -2033,10 +2076,12 @@ int libxl__ao_inprogress(libxl__ao *ao,
* synchronous cancellation ability. */
}
+ /* The call to egc..1_baton is below, only if we are leaving. */
CTX_UNLOCK;
- libxl__egc_cleanup(&egc);
+ libxl__egc_cleanup_2_ul_cb_gc(&egc);
CTX_LOCK;
}
+ libxl__egc_cleanup_1_baton(&egc);
} else {
rc = 0;
}
@@ -2053,6 +2098,9 @@ int libxl__ao_inprogress(libxl__ao *ao,
static int ao__abort(libxl_ctx *ctx, libxl__ao *parent)
/* Temporarily unlocks ctx, which must be locked exactly once on entry. */
{
+ libxl__egc egc;
+ LIBXL_INIT_EGC(egc,ctx);
+
int rc;
ao__manip_enter(parent);
@@ -2073,9 +2121,6 @@ static int ao__abort(libxl_ctx *ctx, libxl__ao *parent)
/* We keep calling abort hooks until there are none left */
while (!LIBXL_LIST_EMPTY(&parent->abortables)) {
- libxl__egc egc;
- LIBXL_INIT_EGC(egc,ctx);
-
assert(!parent->complete);
libxl__ao_abortable *abrt = LIBXL_LIST_FIRST(&parent->abortables);
@@ -2088,8 +2133,9 @@ static int ao__abort(libxl_ctx *ctx, libxl__ao *parent)
"ao %p: abrt=%p: aborting", parent, abrt->ao);
abrt->callback(&egc, abrt, ERROR_ABORTED);
+ /* The call to egc..1_baton is in the out block below. */
libxl__ctx_unlock(ctx);
- libxl__egc_cleanup(&egc);
+ libxl__egc_cleanup_2_ul_cb_gc(&egc);
libxl__ctx_lock(ctx);
}
@@ -2097,6 +2143,10 @@ static int ao__abort(libxl_ctx *ctx, libxl__ao *parent)
out:
ao__manip_leave(ctx, parent);
+ /* The call to egc..2_ul_cb_gc is above. This is sufficient
+ * because only code inside the loop adds anything to the egc, and
+ * we ensures that the egc is clean when we leave the loop. */
+ libxl__egc_cleanup_1_baton(&egc);
return rc;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 983fffac7a..b9c4031863 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -634,9 +634,23 @@ struct libxl__poller {
* event is deregistered, we set the fds_deregistered of all non-idle
* pollers. So afterpoll can tell whether any POLLNVAL is
* plausibly due to an fd being closed and reopened.
+ *
+ * Additionally, we record whether any fd or time event sources
+ * have been registered. This is necessary because sometimes we
+ * need to wake up the only libxl thread stuck in
+ * eventloop_iteration so that it will pick up new fds or earlier
+ * timeouts. osevents_added is cleared by beforepoll, and set by
+ * fd or timeout event registration. When we are about to leave
+ * libxl (strictly, when we are about to give up an egc), we check
+ * whether there are any pollers. If there are, then at least one
+ * of them must have osevents_added clear. If not, we wake up the
+ * first one on the list. Any entry on pollers_active constitutes
+ * a promise to also make this check, so the baton will never be
+ * dropped.
*/
LIBXL_LIST_ENTRY(libxl__poller) active_entry;
bool fds_deregistered;
+ bool osevents_added;
};
struct libxl__gc {
@@ -2350,7 +2364,10 @@ _hidden libxl_device_model_version
libxl__default_device_model(libxl__gc *gc);
LIBXL_STAILQ_INIT(&(egc).ev_immediates); \
} while(0)
-_hidden void libxl__egc_cleanup(libxl__egc *egc);
+_hidden void libxl__egc_cleanup_1_baton(libxl__egc *egc);
+ /* Passes the baton for added osevents. See comment for
+ * osevents_added in struct libxl__poller. */
+_hidden void libxl__egc_cleanup_2_ul_cb_gc(libxl__egc *egc);
/* Frees memory allocated within this egc's gc, and and report all
* occurred events via callback, if applicable. May reenter the
* application; see restrictions above. The ctx must be UNLOCKED. */
@@ -2361,9 +2378,11 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
libxl__egc egc[1]; LIBXL_INIT_EGC(egc[0],ctx); \
EGC_GC
-#define EGC_FREE libxl__egc_cleanup(egc)
-
-#define CTX_UNLOCK_EGC_FREE do{ CTX_UNLOCK; EGC_FREE; }while(0)
+#define CTX_UNLOCK_EGC_FREE do{ \
+ libxl__egc_cleanup_1_baton(egc); \
+ CTX_UNLOCK; \
+ libxl__egc_cleanup_2_ul_cb_gc(egc); \
+ }while(0)
/*
@@ -2468,8 +2487,9 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
#define AO_INPROGRESS ({ \
libxl_ctx *ao__ctx = libxl__gc_owner(&ao->gc); \
+ /* __ao_inprogress will do egc..1_baton if needed */ \
CTX_UNLOCK; \
- EGC_FREE; \
+ libxl__egc_cleanup_2_ul_cb_gc(egc); \
CTX_LOCK; \
int ao__rc = libxl__ao_inprogress(ao, \
__FILE__, __LINE__, __func__); \
@@ -2481,8 +2501,9 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
libxl_ctx *ao__ctx = libxl__gc_owner(&ao->gc); \
assert(rc); \
libxl__ao_create_fail(ao); \
+ libxl__egc_cleanup_1_baton(egc); \
libxl__ctx_unlock(ao__ctx); /* gc is now invalid */ \
- EGC_FREE; \
+ libxl__egc_cleanup_2_ul_cb_gc(egc); \
(rc); \
})
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |