[libvirt] [PATCH 1/2] libxl: fix 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(a)suse.com&gt; --- src/libxl/libxl_domain.c | 234 +---------------------------------------------- src/libxl/libxl_driver.c | 201 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 201 insertions(+), 234 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 856cfb4..c44799b 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-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) { @@ -796,7 +565,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 c95b387..00dab98 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-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, -- 1.8.4.5