libxl: use driver-wide ctx in fd and timer event handling

Long ago I incorrectly associated libxl fd and timer registrations
with per-domain libxl_ctx objects.  When creating a libxlDomainObjPrivate,
a libxl_ctx is allocated, and libxl_osevent_register_hooks is called
passing a pointer to the libxlDomainObjPrivate.  When an fd or timer
registration occurred, the registration callback received the
libxlDomainObjPrivate, containing the per-domain libxl_ctx.  This
libxl_ctx was then used when informing libxl about fd events or
timer expirations.

The problem with this approach is that fd and timer registrations do not
share the same lifespan as libxlDomainObjPrivate, and hence the per-domain
libxl_ctx ojects.  The result is races between per-domain libxl_ctx's being
destoryed and events firing on associated fds/timers, typically manifesting
as an assert in libxl

libxl_internal.h:2788: libxl__ctx_unlock: Assertion `!r' failed

There is no need to associate libxlDomainObjPrivate objects with libxl's
desire to use libvirt's event loop.  Instead, the driver-wide libxl_ctx can
be used for the fd and timer registrations.

This patch moves the fd and timer handling code away from the
domain-specific code in libxl_domain.c into libxl_driver.c.  While at it,
function names were changed a bit to better describe their purpose.

The unnecessary locking was also removed since the code simply provides a
wrapper over the event loop interface.  Indeed the locks may have been
causing some deadlocks when repeatedly creating/destroying muliple domains.
There have also been rumors about such deadlocks during parallel OpenStack
Tempest runs.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ff1e4dee0b7f25211585f2733f95dd7855edc013..c1d507f60483a5f9fe1f81f447826a8ef3b5b84d 100644 (file)
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1,7 +1,7 @@
 /*
  * libxl_domain.c: libxl domain object private state
  *
- * Copyright (C) 2011-2014 SUSE LINUX Products GmbH, Nuernberg, Germany.
+ * Copyright (C) 2011-2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -46,16 +46,6 @@ VIR_ENUM_IMPL(libxlDomainJob, LIBXL_JOB_LAST,
               "modify",
 );
 
-/* Object used to store info related to libxl event registrations */
-typedef struct _libxlEventHookInfo libxlEventHookInfo;
-typedef libxlEventHookInfo *libxlEventHookInfoPtr;
-struct _libxlEventHookInfo {
-    libxlEventHookInfoPtr next;
-    libxlDomainObjPrivatePtr priv;
-    void *xl_priv;
-    int id;
-};
-
 static virClassPtr libxlDomainObjPrivateClass;
 
 static void
@@ -75,227 +65,6 @@ libxlDomainObjPrivateOnceInit(void)
 
 VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate)
 
-static void
-libxlDomainObjFDEventHookInfoFree(void *obj)
-{
-    VIR_FREE(obj);
-}
-
-static void
-libxlDomainObjTimerEventHookInfoFree(void *obj)
-{
-    libxlEventHookInfoPtr info = obj;
-
-    /* Drop reference on libxlDomainObjPrivate */
-    virObjectUnref(info->priv);
-    VIR_FREE(info);
-}
-
-static void
-libxlDomainObjFDEventCallback(int watch ATTRIBUTE_UNUSED,
-                              int fd,
-                              int vir_events,
-                              void *fd_info)
-{
-    libxlEventHookInfoPtr info = fd_info;
-    int events = 0;
-
-    virObjectLock(info->priv);
-    if (vir_events & VIR_EVENT_HANDLE_READABLE)
-        events |= POLLIN;
-    if (vir_events & VIR_EVENT_HANDLE_WRITABLE)
-        events |= POLLOUT;
-    if (vir_events & VIR_EVENT_HANDLE_ERROR)
-        events |= POLLERR;
-    if (vir_events & VIR_EVENT_HANDLE_HANGUP)
-        events |= POLLHUP;
-
-    virObjectUnlock(info->priv);
-    libxl_osevent_occurred_fd(info->priv->ctx, info->xl_priv, fd, 0, events);
-}
-
-static int
-libxlDomainObjFDRegisterEventHook(void *priv,
-                                  int fd,
-                                  void **hndp,
-                                  short events,
-                                  void *xl_priv)
-{
-    int vir_events = VIR_EVENT_HANDLE_ERROR;
-    libxlEventHookInfoPtr info;
-
-    if (VIR_ALLOC(info) < 0)
-        return -1;
-
-    info->priv = priv;
-    info->xl_priv = xl_priv;
-
-    if (events & POLLIN)
-        vir_events |= VIR_EVENT_HANDLE_READABLE;
-    if (events & POLLOUT)
-        vir_events |= VIR_EVENT_HANDLE_WRITABLE;
-
-    info->id = virEventAddHandle(fd, vir_events, libxlDomainObjFDEventCallback,
-                                 info, libxlDomainObjFDEventHookInfoFree);
-    if (info->id < 0) {
-        VIR_FREE(info);
-        return -1;
-    }
-
-    *hndp = info;
-
-    return 0;
-}
-
-static int
-libxlDomainObjFDModifyEventHook(void *priv ATTRIBUTE_UNUSED,
-                                int fd ATTRIBUTE_UNUSED,
-                                void **hndp,
-                                short events)
-{
-    libxlEventHookInfoPtr info = *hndp;
-    int vir_events = VIR_EVENT_HANDLE_ERROR;
-
-    virObjectLock(info->priv);
-    if (events & POLLIN)
-        vir_events |= VIR_EVENT_HANDLE_READABLE;
-    if (events & POLLOUT)
-        vir_events |= VIR_EVENT_HANDLE_WRITABLE;
-
-    virEventUpdateHandle(info->id, vir_events);
-    virObjectUnlock(info->priv);
-
-    return 0;
-}
-
-static void
-libxlDomainObjFDDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
-                                    int fd ATTRIBUTE_UNUSED,
-                                    void *hnd)
-{
-    libxlEventHookInfoPtr info = hnd;
-    libxlDomainObjPrivatePtr p = info->priv;
-
-    virObjectLock(p);
-    virEventRemoveHandle(info->id);
-    virObjectUnlock(p);
-}
-
-static void
-libxlDomainObjTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info)
-{
-    libxlEventHookInfoPtr info = timer_info;
-    libxlDomainObjPrivatePtr p = info->priv;
-
-    virObjectLock(p);
-    /*
-     * libxl expects the event to be deregistered when calling
-     * libxl_osevent_occurred_timeout, but we dont want the event info
-     * destroyed.  Disable the timeout and only remove it after returning
-     * from libxl.
-     */
-    virEventUpdateTimeout(info->id, -1);
-    virObjectUnlock(p);
-    libxl_osevent_occurred_timeout(p->ctx, info->xl_priv);
-    virObjectLock(p);
-    virEventRemoveTimeout(info->id);
-    virObjectUnlock(p);
-}
-
-static int
-libxlDomainObjTimeoutRegisterEventHook(void *priv,
-                                       void **hndp,
-                                       struct timeval abs_t,
-                                       void *xl_priv)
-{
-    libxlEventHookInfoPtr info;
-    struct timeval now;
-    struct timeval res;
-    static struct timeval zero;
-    int timeout;
-
-    if (VIR_ALLOC(info) < 0)
-        return -1;
-
-    info->priv = priv;
-    /*
-     * Also take a reference on the domain object.  Reference is dropped in
-     * libxlDomainObjEventHookInfoFree, ensuring the domain object outlives the
-     * timeout event objects.
-     */
-    virObjectRef(info->priv);
-    info->xl_priv = xl_priv;
-
-    gettimeofday(&now, NULL);
-    timersub(&abs_t, &now, &res);
-    /* Ensure timeout is not overflowed */
-    if (timercmp(&res, &zero, <)) {
-        timeout = 0;
-    } else if (res.tv_sec > INT_MAX / 1000) {
-        timeout = INT_MAX;
-    } else {
-        timeout = res.tv_sec * 1000 + (res.tv_usec + 999) / 1000;
-    }
-    info->id = virEventAddTimeout(timeout, libxlDomainObjTimerCallback,
-                                  info, libxlDomainObjTimerEventHookInfoFree);
-    if (info->id < 0) {
-        virObjectUnref(info->priv);
-        VIR_FREE(info);
-        return -1;
-    }
-
-    *hndp = info;
-
-    return 0;
-}
-
-/*
- * Note:  There are two changes wrt timeouts starting with xen-unstable
- * changeset 26469:
- *
- * 1. Timeout modify callbacks will only be invoked with an abs_t of {0,0},
- * i.e. make the timeout fire immediately.  Prior to this commit, timeout
- * modify callbacks were never invoked.
- *
- * 2. Timeout deregister hooks will no longer be called.
- */
-static int
-libxlDomainObjTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED,
-                                     void **hndp,
-                                     struct timeval abs_t ATTRIBUTE_UNUSED)
-{
-    libxlEventHookInfoPtr info = *hndp;
-
-    virObjectLock(info->priv);
-    /* Make the timeout fire */
-    virEventUpdateTimeout(info->id, 0);
-    virObjectUnlock(info->priv);
-
-    return 0;
-}
-
-static void
-libxlDomainObjTimeoutDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
-                                         void *hnd)
-{
-    libxlEventHookInfoPtr info = hnd;
-    libxlDomainObjPrivatePtr p = info->priv;
-
-    virObjectLock(p);
-    virEventRemoveTimeout(info->id);
-    virObjectUnlock(p);
-}
-
-
-static const libxl_osevent_hooks libxl_event_callbacks = {
-    .fd_register = libxlDomainObjFDRegisterEventHook,
-    .fd_modify = libxlDomainObjFDModifyEventHook,
-    .fd_deregister = libxlDomainObjFDDeregisterEventHook,
-    .timeout_register = libxlDomainObjTimeoutRegisterEventHook,
-    .timeout_modify = libxlDomainObjTimeoutModifyEventHook,
-    .timeout_deregister = libxlDomainObjTimeoutDeregisterEventHook,
-};
-
 static int
 libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv)
 {
@@ -811,7 +580,6 @@ libxlDomainObjPrivateInitCtx(virDomainObjPtr vm)
         goto cleanup;
     }
 
-    libxl_osevent_register_hooks(priv->ctx, &libxl_event_callbacks, priv);
     libxl_childproc_setmode(priv->ctx, &libxl_child_hooks, priv);
 
     ret = 0;

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 1c1af79ee076527671e7e9681dc096bfa373be73..0f40836d44ef9679ce6075db9eacb56fa7c939da 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2,7 +2,7 @@
  * libxl_driver.c: core driver methods for managing libxenlight domains
  *
  * Copyright (C) 2006-2014 Red Hat, Inc.
- * Copyright (C) 2011-2014 SUSE LINUX Products GmbH, Nuernberg, Germany.
+ * Copyright (C) 2011-2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
  * Copyright (C) 2011 Univention GmbH.
  *
  * This library is free software; you can redistribute it and/or
@@ -80,6 +80,15 @@ VIR_LOG_INIT("libxl.libxl_driver");
 
 static libxlDriverPrivatePtr libxl_driver;
 
+/* Object used to store info related to libxl event registrations */
+typedef struct _libxlOSEventHookInfo libxlOSEventHookInfo;
+typedef libxlOSEventHookInfo *libxlOSEventHookInfoPtr;
+struct _libxlOSEventHookInfo {
+    libxl_ctx *ctx;
+    void *xl_priv;
+    int id;
+};
+
 /* Function declarations */
 static int
 libxlDomainManagedSaveLoad(virDomainObjPtr vm,
@@ -87,6 +96,183 @@ libxlDomainManagedSaveLoad(virDomainObjPtr vm,
 
 
 /* Function definitions */
+static void
+libxlOSEventHookInfoFree(void *obj)
+{
+    VIR_FREE(obj);
+}
+
+static void
+libxlFDEventCallback(int watch ATTRIBUTE_UNUSED,
+                     int fd,
+                     int vir_events,
+                     void *fd_info)
+{
+    libxlOSEventHookInfoPtr info = fd_info;
+    int events = 0;
+
+    if (vir_events & VIR_EVENT_HANDLE_READABLE)
+        events |= POLLIN;
+    if (vir_events & VIR_EVENT_HANDLE_WRITABLE)
+        events |= POLLOUT;
+    if (vir_events & VIR_EVENT_HANDLE_ERROR)
+        events |= POLLERR;
+    if (vir_events & VIR_EVENT_HANDLE_HANGUP)
+        events |= POLLHUP;
+
+    libxl_osevent_occurred_fd(info->ctx, info->xl_priv, fd, 0, events);
+}
+
+static int
+libxlFDRegisterEventHook(void *priv,
+                         int fd,
+                         void **hndp,
+                         short events,
+                         void *xl_priv)
+{
+    int vir_events = VIR_EVENT_HANDLE_ERROR;
+    libxlOSEventHookInfoPtr info;
+
+    if (VIR_ALLOC(info) < 0)
+        return -1;
+
+    info->ctx = priv;
+    info->xl_priv = xl_priv;
+
+    if (events & POLLIN)
+        vir_events |= VIR_EVENT_HANDLE_READABLE;
+    if (events & POLLOUT)
+        vir_events |= VIR_EVENT_HANDLE_WRITABLE;
+
+    info->id = virEventAddHandle(fd, vir_events, libxlFDEventCallback,
+                                 info, libxlOSEventHookInfoFree);
+    if (info->id < 0) {
+        VIR_FREE(info);
+        return -1;
+    }
+
+    *hndp = info;
+
+    return 0;
+}
+
+static int
+libxlFDModifyEventHook(void *priv ATTRIBUTE_UNUSED,
+                       int fd ATTRIBUTE_UNUSED,
+                       void **hndp,
+                       short events)
+{
+    libxlOSEventHookInfoPtr info = *hndp;
+    int vir_events = VIR_EVENT_HANDLE_ERROR;
+
+    if (events & POLLIN)
+        vir_events |= VIR_EVENT_HANDLE_READABLE;
+    if (events & POLLOUT)
+        vir_events |= VIR_EVENT_HANDLE_WRITABLE;
+
+    virEventUpdateHandle(info->id, vir_events);
+
+    return 0;
+}
+
+static void
+libxlFDDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
+                           int fd ATTRIBUTE_UNUSED,
+                           void *hnd)
+{
+    libxlOSEventHookInfoPtr info = hnd;
+
+    virEventRemoveHandle(info->id);
+}
+
+static void
+libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info)
+{
+    libxlOSEventHookInfoPtr info = timer_info;
+
+    /*
+     * libxl expects the event to be deregistered when calling
+     * libxl_osevent_occurred_timeout, but we dont want the event info
+     * destroyed.  Disable the timeout and only remove it after returning
+     * from libxl.
+     */
+    virEventUpdateTimeout(info->id, -1);
+    libxl_osevent_occurred_timeout(info->ctx, info->xl_priv);
+    virEventRemoveTimeout(info->id);
+}
+
+static int
+libxlTimeoutRegisterEventHook(void *priv,
+                              void **hndp,
+                              struct timeval abs_t,
+                              void *xl_priv)
+{
+    libxlOSEventHookInfoPtr info;
+    struct timeval now;
+    struct timeval res;
+    static struct timeval zero;
+    int timeout;
+
+    if (VIR_ALLOC(info) < 0)
+        return -1;
+
+    info->ctx = priv;
+    info->xl_priv = xl_priv;
+
+    gettimeofday(&now, NULL);
+    timersub(&abs_t, &now, &res);
+    /* Ensure timeout is not overflowed */
+    if (timercmp(&res, &zero, <)) {
+        timeout = 0;
+    } else if (res.tv_sec > INT_MAX / 1000) {
+        timeout = INT_MAX;
+    } else {
+        timeout = res.tv_sec * 1000 + (res.tv_usec + 999) / 1000;
+    }
+    info->id = virEventAddTimeout(timeout, libxlTimerCallback,
+                                  info, libxlOSEventHookInfoFree);
+    if (info->id < 0) {
+        VIR_FREE(info);
+        return -1;
+    }
+
+    *hndp = info;
+
+    return 0;
+}
+
+/*
+ * Note:  There are two changes wrt timeouts starting with xen-unstable
+ * changeset 26469:
+ *
+ * 1. Timeout modify callbacks will only be invoked with an abs_t of {0,0},
+ * i.e. make the timeout fire immediately.  Prior to this commit, timeout
+ * modify callbacks were never invoked.
+ *
+ * 2. Timeout deregister hooks will no longer be called.
+ */
+static int
+libxlTimeoutModifyEventHook(void *priv ATTRIBUTE_UNUSED,
+                            void **hndp,
+                            struct timeval abs_t ATTRIBUTE_UNUSED)
+{
+    libxlOSEventHookInfoPtr info = *hndp;
+
+    /* Make the timeout fire */
+    virEventUpdateTimeout(info->id, 0);
+
+    return 0;
+}
+
+static void
+libxlTimeoutDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
+                                void *hnd)
+{
+    libxlOSEventHookInfoPtr info = hnd;
+
+    virEventRemoveTimeout(info->id);
+}
+
 static virDomainObjPtr
 libxlDomObjFromDomain(virDomainPtr dom)
 {
@@ -277,6 +463,16 @@ libxlDriverShouldLoad(bool privileged)
     return ret;
 }
 
+/* Callbacks wrapping libvirt's event loop interface */
+static const libxl_osevent_hooks libxl_osevent_callbacks = {
+    .fd_register = libxlFDRegisterEventHook,
+    .fd_modify = libxlFDModifyEventHook,
+    .fd_deregister = libxlFDDeregisterEventHook,
+    .timeout_register = libxlTimeoutRegisterEventHook,
+    .timeout_modify = libxlTimeoutModifyEventHook,
+    .timeout_deregister = libxlTimeoutDeregisterEventHook,
+};
+
 static int
 libxlStateInitialize(bool privileged,
                      virStateInhibitCallback callback ATTRIBUTE_UNUSED,
@@ -322,6 +518,9 @@ libxlStateInitialize(bool privileged,
     if (!(cfg = libxlDriverConfigNew()))
         goto error;
 
+    /* Register the callbacks providing access to libvirt's event loop */
+    libxl_osevent_register_hooks(cfg->ctx, &libxl_osevent_callbacks, cfg->ctx);
+
     libxl_driver->config = cfg;
     if (virFileMakePath(cfg->stateDir) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,