[PATCH 0/4] libxl: fixes related to concurrency improvements

[PATCH 0/4] libxl: fixes related to concurrency improvements

[Jim Fehlig]

While reviving old patches to add job support to the libxl driver,
testing revealed some problems that were difficult to encounter
in the current, more serialized processing approach used in the
driver.

The first patch is a bug fix, plugging leaks of libxlDomainObjPrivate
objects.  The second patch removes the list of libxl timer registrations
maintained in the driver - a hack I was never fond of.  The third patch
moves domain shutdown handling to a thread, instead of doing all the
shutdown work in the event handler.  The fourth patch fixes an issue wrt
child process handling discussed in this thread

http://lists.xen.org/archives/html/xen-devel/2014-01/msg01553.html

Ian Jackson's latest patches on the libxl side are here

http://lists.xen.org/archives/html/xen-devel/2014-02/msg00124.html


Jim Fehlig (4):
  libxl: fix leaking libxlDomainObjPrivate
  libxl: remove list of timer registrations from libxlDomainObjPrivate
  libxl: handle domain shutdown events in a thread
  libxl: improve subprocess handling

 src/libxl/libxl_conf.h   |   5 +-
 src/libxl/libxl_domain.c | 102 ++++++++---------------------------
 src/libxl/libxl_domain.h |   8 +--
 src/libxl/libxl_driver.c | 135 +++++++++++++++++++++++++++++++----------------
 4 files changed, 115 insertions(+), 135 deletions(-)

-- 
1.8.1.4

[PATCH 1/4] libxl: fix leaking libxlDomainObjPrivate

[Jim Fehlig]

When libxl registers an FD with the libxl driver, the refcnt of the
associated libxlDomainObjPrivate object is incremented. The refcnt
is decremented when libxl deregisters the FD.  But some FDs are only
deregistered when their libxl ctx is freed, which unfortunately is
done in the libxlDomainObjPrivate dispose function.  With references
held by the FDs, libxlDomainObjPrivate is never disposed.

I added the ref/unref in FD registration/deregistration when adding
the same in timer registration/deregistration.  For timers, this
is a simple approach to ensuring the libxlDomainObjPrivate is not
disposed prior to their expirtation, which libxl guarantees will
occur.  It is not needed for FDs, and only causes
libxlDomainObjPrivate to leak.

This patch removes the reference on libxlDomainObjPrivate for FD
registrations, but retains them for timer registrations.  Tested on
the latest releases of Xen supported by the libxl driver:  4.2.3,
4.3.1, and 4.4.0 RC3.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_domain.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index e72c483..7efc13b 100644
--- 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-2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
+ * Copyright (C) 2011-2014 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
@@ -92,7 +92,13 @@ libxlDomainObjPrivateOnceInit(void)
 VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate)
 
 static void
-libxlDomainObjEventHookInfoFree(void *obj)
+libxlDomainObjFDEventHookInfoFree(void *obj)
+{
+    VIR_FREE(obj);
+}
+
+static void
+libxlDomainObjTimerEventHookInfoFree(void *obj)
 {
     libxlEventHookInfoPtr info = obj;
 
@@ -138,12 +144,6 @@ libxlDomainObjFDRegisterEventHook(void *priv,
         return -1;
 
     info->priv = priv;
-    /*
-     * Take a reference on the domain object.  Reference is dropped in
-     * libxlDomainObjEventHookInfoFree, ensuring the domain object outlives
-     * the fd event objects.
-     */
-    virObjectRef(info->priv);
     info->xl_priv = xl_priv;
 
     if (events & POLLIN)
@@ -152,9 +152,8 @@ libxlDomainObjFDRegisterEventHook(void *priv,
         vir_events |= VIR_EVENT_HANDLE_WRITABLE;
 
     info->id = virEventAddHandle(fd, vir_events, libxlDomainObjFDEventCallback,
-                                 info, libxlDomainObjEventHookInfoFree);
+                                 info, libxlDomainObjFDEventHookInfoFree);
     if (info->id < 0) {
-        virObjectUnref(info->priv);
         VIR_FREE(info);
         return -1;
     }
@@ -259,7 +258,7 @@ libxlDomainObjTimeoutRegisterEventHook(void *priv,
         timeout = res.tv_sec * 1000 + (res.tv_usec + 999) / 1000;
     }
     info->id = virEventAddTimeout(timeout, libxlDomainObjTimerCallback,
-                                  info, libxlDomainObjEventHookInfoFree);
+                                  info, libxlDomainObjTimerEventHookInfoFree);
     if (info->id < 0) {
         virObjectUnref(info->priv);
         VIR_FREE(info);
-- 
1.8.1.4

[PATCH 2/4] libxl: remove list of timer registrations from libxlDomainObjPrivate

[Jim Fehlig]

Due to some misunderstanding of requirements libxl places on timer
handling, I introduced the half-brained idea of maintaining a list
of timeouts that the driver could force to expire before freeing a
libxlDomainObjPrivate (and hence libxl_ctx).  But testing all
the latest versions of Xen supported by the libxl driver (4.2.3,
4.3.1, 4.4.0 RC3), I see that libxl will handle this just fine and
there is no need to force expiration behind libxl's back.  Indeed it
may be harmful to do so.

This patch removes the timer list, allowing libxl to handle cleanup
of its timer registrations.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_conf.h   |  5 +---
 src/libxl/libxl_domain.c | 72 +++---------------------------------------------
 src/libxl/libxl_domain.h |  8 +-----
 src/libxl/libxl_driver.c |  3 +-
 4 files changed, 7 insertions(+), 81 deletions(-)

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index f743541..ca7bc7d 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -1,7 +1,7 @@
 /*
  * libxl_conf.h: libxl configuration management
  *
- * Copyright (C) 2011-2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
+ * Copyright (C) 2011-2014 SUSE LINUX Products GmbH, Nuernberg, Germany.
  * Copyright (C) 2011 Univention GmbH.
  *
  * This library is free software; you can redistribute it and/or
@@ -115,9 +115,6 @@ struct _libxlDriverPrivate {
     virSysinfoDefPtr hostsysinfo;
 };
 
-typedef struct _libxlEventHookInfo libxlEventHookInfo;
-typedef libxlEventHookInfo *libxlEventHookInfoPtr;
-
 # define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r"
 # define LIBXL_SAVE_VERSION 1
 
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 7efc13b..fbd6cab 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -34,37 +34,9 @@
 #define VIR_FROM_THIS VIR_FROM_LIBXL
 
 
-/* Append an event registration to the list of registrations */
-#define LIBXL_EV_REG_APPEND(head, add)                 \
-    do {                                               \
-        libxlEventHookInfoPtr temp;                    \
-        if (head) {                                    \
-            temp = head;                               \
-            while (temp->next)                         \
-                temp = temp->next;                     \
-            temp->next = add;                          \
-        } else {                                       \
-            head = add;                                \
-        }                                              \
-    } while (0)
-
-/* Remove an event registration from the list of registrations */
-#define LIBXL_EV_REG_REMOVE(head, del)                 \
-    do {                                               \
-        libxlEventHookInfoPtr temp;                    \
-        if (head == del) {                             \
-            head = head->next;                         \
-        } else {                                       \
-            temp = head;                               \
-            while (temp->next && temp->next != del)    \
-                temp = temp->next;                     \
-            if (temp->next) {                          \
-                temp->next = del->next;                \
-            }                                          \
-        }                                              \
-    } while (0)
-
 /* Object used to store info related to libxl event registrations */
+typedef struct _libxlEventHookInfo libxlEventHookInfo;
+typedef libxlEventHookInfo *libxlEventHookInfoPtr;
 struct _libxlEventHookInfo {
     libxlEventHookInfoPtr next;
     libxlDomainObjPrivatePtr priv;
@@ -214,12 +186,7 @@ libxlDomainObjTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info)
     virObjectUnlock(p);
     libxl_osevent_occurred_timeout(p->ctx, info->xl_priv);
     virObjectLock(p);
-    /*
-     * Timeout could have been freed while the lock was dropped.
-     * Only remove it from the list if it still exists.
-     */
-    if (virEventRemoveTimeout(info->id) == 0)
-        LIBXL_EV_REG_REMOVE(p->timerRegistrations, info);
+    virEventRemoveTimeout(info->id);
     virObjectUnlock(p);
 }
 
@@ -265,9 +232,6 @@ libxlDomainObjTimeoutRegisterEventHook(void *priv,
         return -1;
     }
 
-    virObjectLock(info->priv);
-    LIBXL_EV_REG_APPEND(info->priv->timerRegistrations, info);
-    virObjectUnlock(info->priv);
     *hndp = info;
 
     return 0;
@@ -306,12 +270,7 @@ libxlDomainObjTimeoutDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
     libxlDomainObjPrivatePtr p = info->priv;
 
     virObjectLock(p);
-    /*
-     * Only remove the timeout from the list if removal from the
-     * event loop is successful.
-     */
-    if (virEventRemoveTimeout(info->id) == 0)
-        LIBXL_EV_REG_REMOVE(p->timerRegistrations, info);
+    virEventRemoveTimeout(info->id);
     virObjectUnlock(p);
 }
 
@@ -443,26 +402,3 @@ cleanup:
     VIR_FREE(log_file);
     return ret;
 }
-
-void
-libxlDomainObjRegisteredTimeoutsCleanup(libxlDomainObjPrivatePtr priv)
-{
-    libxlEventHookInfoPtr info;
-
-    virObjectLock(priv);
-    info = priv->timerRegistrations;
-    while (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(priv->ctx, info->xl_priv);
-        virEventRemoveTimeout(info->id);
-        info = info->next;
-    }
-    priv->timerRegistrations = NULL;
-    virObjectUnlock(priv);
-}
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index e4695ef..8565820 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -1,7 +1,7 @@
 /*
  * libxl_domain.h: libxl domain object private state
  *
- * Copyright (C) 2011-2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
+ * Copyright (C) 2011-2014 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
@@ -43,9 +43,6 @@ struct _libxlDomainObjPrivate {
     /* console */
     virChrdevsPtr devs;
     libxl_evgen_domain_death *deathW;
-
-    /* list of libxl timeout registrations */
-    libxlEventHookInfoPtr timerRegistrations;
 };
 
 
@@ -56,7 +53,4 @@ extern virDomainDefParserConfig libxlDomainDefParserConfig;
 int
 libxlDomainObjPrivateInitCtx(virDomainObjPtr vm);
 
-void
-libxlDomainObjRegisteredTimeoutsCleanup(libxlDomainObjPrivatePtr priv);
-
 #endif /* LIBXL_DOMAIN_H */
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index cb3deec..d639011 100644
--- 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-2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
+ * Copyright (C) 2011-2014 SUSE LINUX Products GmbH, Nuernberg, Germany.
  * Copyright (C) 2011 Univention GmbH.
  *
  * This library is free software; you can redistribute it and/or
@@ -313,7 +313,6 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
         vm->newDef = NULL;
     }
 
-    libxlDomainObjRegisteredTimeoutsCleanup(priv);
     virObjectUnref(cfg);
 }
 
-- 
1.8.1.4

[PATCH 3/4] libxl: handle domain shutdown events in a thread

[Jim Fehlig]

Handling the domain shutdown event within the event handler seems
a bit unfair to libxl's event machinery.  Domain "shutdown" could
take considerable time.  E.g. if the shutdown reason is reboot,
the domain must be reaped and then started again.

Spawn a shutdown handler thread to do this work, allowing libxl's
event machinery to go about its business.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_driver.c | 132 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 89 insertions(+), 43 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d639011..a1c6c0f 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -352,61 +352,107 @@ libxlVmReap(libxlDriverPrivatePtr driver,
 # define VIR_LIBXL_EVENT_CONST const
 #endif
 
+struct libxlShutdownThreadInfo
+{
+    virDomainObjPtr vm;
+    libxl_event *event;
+};
+
+
 static void
-libxlEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
+libxlDomainShutdownThread(void *opaque)
 {
     libxlDriverPrivatePtr driver = libxl_driver;
-    libxlDomainObjPrivatePtr priv = ((virDomainObjPtr)data)->privateData;
-    virDomainObjPtr vm = NULL;
+    struct libxlShutdownThreadInfo *shutdown_info = opaque;
+    virDomainObjPtr vm = shutdown_info->vm;
+    libxlDomainObjPrivatePtr priv = vm->privateData;
+    libxl_event *ev = shutdown_info->event;
+    libxl_ctx *ctx = priv->ctx;
     virObjectEventPtr dom_event = NULL;
-    libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
-
-    if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
-        virDomainShutoffReason reason;
-
-        /*
-         * Similar to the xl implementation, ignore SUSPEND.  Any actions needed
-         * after calling libxl_domain_suspend() are handled by it's callers.
-         */
-        if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
-            goto cleanup;
+    libxl_shutdown_reason xl_reason = ev->u.domain_shutdown.shutdown_reason;
+    virDomainShutoffReason reason;
 
-        vm = virDomainObjListFindByID(driver->domains, event->domid);
-        if (!vm)
-            goto cleanup;
+    virObjectLock(vm);
 
-        switch (xl_reason) {
-            case LIBXL_SHUTDOWN_REASON_POWEROFF:
-            case LIBXL_SHUTDOWN_REASON_CRASH:
-                if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
-                    dom_event = virDomainEventLifecycleNewFromObj(vm,
-                                              VIR_DOMAIN_EVENT_STOPPED,
-                                              VIR_DOMAIN_EVENT_STOPPED_CRASHED);
-                    reason = VIR_DOMAIN_SHUTOFF_CRASHED;
-                } else {
-                    reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
-                }
-                libxlVmReap(driver, vm, reason);
-                if (!vm->persistent) {
-                    virDomainObjListRemove(driver->domains, vm);
-                    vm = NULL;
-                }
-                break;
-            case LIBXL_SHUTDOWN_REASON_REBOOT:
-                libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
-                libxlVmStart(driver, vm, 0, -1);
-                break;
-            default:
-                VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
-                break;
-        }
+    switch (xl_reason) {
+        case LIBXL_SHUTDOWN_REASON_POWEROFF:
+        case LIBXL_SHUTDOWN_REASON_CRASH:
+            if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
+                dom_event = virDomainEventLifecycleNewFromObj(vm,
+                                           VIR_DOMAIN_EVENT_STOPPED,
+                                           VIR_DOMAIN_EVENT_STOPPED_CRASHED);
+                reason = VIR_DOMAIN_SHUTOFF_CRASHED;
+            } else {
+                reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
+            }
+            libxlVmReap(driver, vm, reason);
+            if (!vm->persistent) {
+                virDomainObjListRemove(driver->domains, vm);
+                vm = NULL;
+            }
+            break;
+        case LIBXL_SHUTDOWN_REASON_REBOOT:
+            libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
+            libxlVmStart(driver, vm, 0, -1);
+            break;
+        default:
+            VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
+            break;
     }
 
-cleanup:
     if (vm)
         virObjectUnlock(vm);
     if (dom_event)
         libxlDomainEventQueue(driver, dom_event);
+    libxl_event_free(ctx, ev);
+    VIR_FREE(shutdown_info);
+}
+
+static void
+libxlEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
+{
+    virDomainObjPtr vm = data;
+    libxlDomainObjPrivatePtr priv = vm->privateData;
+    libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
+    struct libxlShutdownThreadInfo *shutdown_info;
+    virThread thread;
+
+    if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
+        VIR_INFO("Unhandled event type %d", event->type);
+        goto cleanup;
+    }
+
+    /*
+     * Similar to the xl implementation, ignore SUSPEND.  Any actions needed
+     * after calling libxl_domain_suspend() are handled by it's callers.
+     */
+    if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
+        goto cleanup;
+
+    /*
+     * Start a thread to handle shutdown.  We don't want to be tying up
+     * libxl's event machinery by doing a potentially lengthy shutdown.
+     */
+    if (VIR_ALLOC(shutdown_info) < 0)
+        goto cleanup;
+
+    shutdown_info->vm = data;
+    shutdown_info->event = (libxl_event *)event;
+    if (virThreadCreate(&thread, true, libxlDomainShutdownThread,
+                        shutdown_info) < 0) {
+        /*
+         * Not much we can do on error here except log it.
+         */
+        VIR_ERROR(_("Failed to create thread to handle domain shutdown"));
+        goto cleanup;
+    }
+
+    /*
+     * libxl_event freed in shutdown thread
+     */
+    return;
+
+cleanup:
     /* Cast away any const */
     libxl_event_free(priv->ctx, (libxl_event *)event);
 }
-- 
1.8.1.4

[PATCH 4/4] libxl: improve subprocess handling

[Jim Fehlig]

If available, let libxl handle reaping any children it creates by
specifying libxl_sigchld_owner_libxl_always_selective_reap.  This
feature was added to improve subprocess handling in libxl when used
in an application that does not install a SIGCHLD handler like
libvirt

http://lists.xen.org/archives/html/xen-devel/2014-01/msg01555.html

Prior to this patch, it is possible to hit asserts in libxl when
reaping subprocesses, particularly during simultaneous operations
on multiple domains.  With this patch, and the corresponding changes
to libxl, I no longer see the asserts.

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

The libxl patch has not yet hit xen.git, but without it this patch
has no semantic change, only explicitly setting chldowner to the
default of libxl_sigchld_owner_libxl.

 src/libxl/libxl_domain.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index fbd6cab..eb2e50e 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -358,6 +358,14 @@ virDomainDefParserConfig libxlDomainDefParserConfig = {
     .devicesPostParseCallback = libxlDomainDeviceDefPostParse,
 };
 
+static const libxl_childproc_hooks libxl_child_hooks = {
+#ifdef LIBXL_HAVE_SIGCHLD_OWNER_SELECTIVE_REAP
+    .chldowner = libxl_sigchld_owner_libxl_always_selective_reap,
+#else
+    .chldowner = libxl_sigchld_owner_libxl,
+#endif
+};
+
 int
 libxlDomainObjPrivateInitCtx(virDomainObjPtr vm)
 {
@@ -395,6 +403,7 @@ libxlDomainObjPrivateInitCtx(virDomainObjPtr vm)
     }
 
     libxl_osevent_register_hooks(priv->ctx, &libxl_event_callbacks, priv);
+    libxl_childproc_setmode(priv->ctx, &libxl_child_hooks, priv);
 
     ret = 0;
 
-- 
1.8.1.4

Re: [libvirt] [PATCH 3/4] libxl: handle domain shutdown events in a thread

[Michal Privoznik]

On 05.02.2014 18:39, Jim Fehlig wrote:
> Handling the domain shutdown event within the event handler seems
> a bit unfair to libxl's event machinery.  Domain "shutdown" could
> take considerable time.  E.g. if the shutdown reason is reboot,
> the domain must be reaped and then started again.
>
> Spawn a shutdown handler thread to do this work, allowing libxl's
> event machinery to go about its business.
>
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>   src/libxl/libxl_driver.c | 132 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 89 insertions(+), 43 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d639011..a1c6c0f 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -352,61 +352,107 @@ libxlVmReap(libxlDriverPrivatePtr driver,
>   # define VIR_LIBXL_EVENT_CONST const
>   #endif
>
> +struct libxlShutdownThreadInfo
> +{
> +    virDomainObjPtr vm;
> +    libxl_event *event;
> +};
> +
> +
>   static void
> -libxlEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
> +libxlDomainShutdownThread(void *opaque)
>   {
>       libxlDriverPrivatePtr driver = libxl_driver;
> -    libxlDomainObjPrivatePtr priv = ((virDomainObjPtr)data)->privateData;
> -    virDomainObjPtr vm = NULL;
> +    struct libxlShutdownThreadInfo *shutdown_info = opaque;
> +    virDomainObjPtr vm = shutdown_info->vm;
> +    libxlDomainObjPrivatePtr priv = vm->privateData;
> +    libxl_event *ev = shutdown_info->event;
> +    libxl_ctx *ctx = priv->ctx;
>       virObjectEventPtr dom_event = NULL;
> -    libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
> -
> -    if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
> -        virDomainShutoffReason reason;
> -
> -        /*
> -         * Similar to the xl implementation, ignore SUSPEND.  Any actions needed
> -         * after calling libxl_domain_suspend() are handled by it's callers.
> -         */
> -        if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
> -            goto cleanup;
> +    libxl_shutdown_reason xl_reason = ev->u.domain_shutdown.shutdown_reason;
> +    virDomainShutoffReason reason;
>
> -        vm = virDomainObjListFindByID(driver->domains, event->domid);
> -        if (!vm)
> -            goto cleanup;
> +    virObjectLock(vm);
>
> -        switch (xl_reason) {
> -            case LIBXL_SHUTDOWN_REASON_POWEROFF:
> -            case LIBXL_SHUTDOWN_REASON_CRASH:
> -                if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
> -                    dom_event = virDomainEventLifecycleNewFromObj(vm,
> -                                              VIR_DOMAIN_EVENT_STOPPED,
> -                                              VIR_DOMAIN_EVENT_STOPPED_CRASHED);
> -                    reason = VIR_DOMAIN_SHUTOFF_CRASHED;
> -                } else {
> -                    reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
> -                }
> -                libxlVmReap(driver, vm, reason);
> -                if (!vm->persistent) {
> -                    virDomainObjListRemove(driver->domains, vm);
> -                    vm = NULL;
> -                }
> -                break;
> -            case LIBXL_SHUTDOWN_REASON_REBOOT:
> -                libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> -                libxlVmStart(driver, vm, 0, -1);
> -                break;
> -            default:
> -                VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
> -                break;
> -        }
> +    switch (xl_reason) {
> +        case LIBXL_SHUTDOWN_REASON_POWEROFF:
> +        case LIBXL_SHUTDOWN_REASON_CRASH:
> +            if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
> +                dom_event = virDomainEventLifecycleNewFromObj(vm,
> +                                           VIR_DOMAIN_EVENT_STOPPED,
> +                                           VIR_DOMAIN_EVENT_STOPPED_CRASHED);
> +                reason = VIR_DOMAIN_SHUTOFF_CRASHED;
> +            } else {
> +                reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
> +            }
> +            libxlVmReap(driver, vm, reason);
> +            if (!vm->persistent) {
> +                virDomainObjListRemove(driver->domains, vm);
> +                vm = NULL;
> +            }
> +            break;
> +        case LIBXL_SHUTDOWN_REASON_REBOOT:
> +            libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> +            libxlVmStart(driver, vm, 0, -1);
> +            break;
> +        default:
> +            VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
> +            break;
>       }
>
> -cleanup:
>       if (vm)
>           virObjectUnlock(vm);
>       if (dom_event)
>           libxlDomainEventQueue(driver, dom_event);
> +    libxl_event_free(ctx, ev);
> +    VIR_FREE(shutdown_info);
> +}
> +
> +static void
> +libxlEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
> +{
> +    virDomainObjPtr vm = data;
> +    libxlDomainObjPrivatePtr priv = vm->privateData;
> +    libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
> +    struct libxlShutdownThreadInfo *shutdown_info;
> +    virThread thread;
> +
> +    if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
> +        VIR_INFO("Unhandled event type %d", event->type);
> +        goto cleanup;
> +    }
> +
> +    /*
> +     * Similar to the xl implementation, ignore SUSPEND.  Any actions needed
> +     * after calling libxl_domain_suspend() are handled by it's callers.
> +     */
> +    if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
> +        goto cleanup;
> +
> +    /*
> +     * Start a thread to handle shutdown.  We don't want to be tying up
> +     * libxl's event machinery by doing a potentially lengthy shutdown.
> +     */
> +    if (VIR_ALLOC(shutdown_info) < 0)
> +        goto cleanup;
> +
> +    shutdown_info->vm = data;
> +    shutdown_info->event = (libxl_event *)event;
> +    if (virThreadCreate(&thread, true, libxlDomainShutdownThread,
> +                        shutdown_info) < 0) {

The 2nd argument 'true' tells, if @thread is joinable. Since you are not 
joining it anywhere, I guess it should be rather 'false'. Moreover, it 
will avoid some memory leaks, as pthread free()-s memory needed for 
thread itself (otherwise the memory would be free()-d in phtread_join - 
which again is not called anywhere).

> +        /*
> +         * Not much we can do on error here except log it.
> +         */
> +        VIR_ERROR(_("Failed to create thread to handle domain shutdown"));
> +        goto cleanup;
> +    }
> +
> +    /*
> +     * libxl_event freed in shutdown thread
> +     */
> +    return;
> +
> +cleanup:

Since this is in fact an error path, I suggest renaming the label to 
'error'.

>       /* Cast away any const */
>       libxl_event_free(priv->ctx, (libxl_event *)event);
>   }
>

ACK if you fix these two issues.

Michal

Re: [libvirt] [PATCH 0/4] libxl: fixes related to concurrency improvements

[Michal Privoznik]

On 05.02.2014 18:39, Jim Fehlig wrote:
> While reviving old patches to add job support to the libxl driver,
> testing revealed some problems that were difficult to encounter
> in the current, more serialized processing approach used in the
> driver.
>
> The first patch is a bug fix, plugging leaks of libxlDomainObjPrivate
> objects.  The second patch removes the list of libxl timer registrations
> maintained in the driver - a hack I was never fond of.  The third patch
> moves domain shutdown handling to a thread, instead of doing all the
> shutdown work in the event handler.  The fourth patch fixes an issue wrt
> child process handling discussed in this thread
>
> http://lists.xen.org/archives/html/xen-devel/2014-01/msg01553.html
>
> Ian Jackson's latest patches on the libxl side are here
>
> http://lists.xen.org/archives/html/xen-devel/2014-02/msg00124.html
>
>
> Jim Fehlig (4):
>    libxl: fix leaking libxlDomainObjPrivate
>    libxl: remove list of timer registrations from libxlDomainObjPrivate
>    libxl: handle domain shutdown events in a thread
>    libxl: improve subprocess handling
>
>   src/libxl/libxl_conf.h   |   5 +-
>   src/libxl/libxl_domain.c | 102 ++++++++---------------------------
>   src/libxl/libxl_domain.h |   8 +--
>   src/libxl/libxl_driver.c | 135 +++++++++++++++++++++++++++++++----------------
>   4 files changed, 115 insertions(+), 135 deletions(-)
>

ACK series but see my comment on 3/4 where I'm asking for a pair of 
fixes prior pushing.

Michal

Re: [libvirt] [PATCH 0/4] libxl: fixes related to concurrency improvements

[Jim Fehlig]

Michal Privoznik wrote:
> On 05.02.2014 18:39, Jim Fehlig wrote:
>> While reviving old patches to add job support to the libxl driver,
>> testing revealed some problems that were difficult to encounter
>> in the current, more serialized processing approach used in the
>> driver.
>>
>> The first patch is a bug fix, plugging leaks of libxlDomainObjPrivate
>> objects.  The second patch removes the list of libxl timer registrations
>> maintained in the driver - a hack I was never fond of.  The third patch
>> moves domain shutdown handling to a thread, instead of doing all the
>> shutdown work in the event handler.  The fourth patch fixes an issue wrt
>> child process handling discussed in this thread
>>
>> http://lists.xen.org/archives/html/xen-devel/2014-01/msg01553.html
>>
>> Ian Jackson's latest patches on the libxl side are here
>>
>> http://lists.xen.org/archives/html/xen-devel/2014-02/msg00124.html
>>
>>
>> Jim Fehlig (4):
>>    libxl: fix leaking libxlDomainObjPrivate
>>    libxl: remove list of timer registrations from libxlDomainObjPrivate
>>    libxl: handle domain shutdown events in a thread
>>    libxl: improve subprocess handling
>>
>>   src/libxl/libxl_conf.h   |   5 +-
>>   src/libxl/libxl_domain.c | 102 ++++++++---------------------------
>>   src/libxl/libxl_domain.h |   8 +--
>>   src/libxl/libxl_driver.c | 135
>> +++++++++++++++++++++++++++++++----------------
>>   4 files changed, 115 insertions(+), 135 deletions(-)
>>
>
> ACK series but see my comment on 3/4 where I'm asking for a pair of
> fixes prior pushing.

Thanks for pointing those out, especially creating the joinable thread
that was never joined :).  Fixed.  I also added a note to the commit
message of 4/4 stating that the fixes on the libxl side will be included
in Xen 4.4.0

http://lists.xen.org/archives/html/xen-devel/2014-02/msg00463.html

Pushed series.  Thanks!

Regards,
Jim