[libvirt] [PATCH 0/4] libxl: fixes related to concurrency improvements
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
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
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
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
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
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
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
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
participants (2)
-
Jim Fehlig -
Michal Privoznik