Re: [Xen-devel] [PATCH RFC] libxl: use libxl_event_wait to process libxl events

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-11-13 at 14:36 -0700, Jim Fehlig wrote:
> Prior to this patch, libxl events were delivered to libvirt via
> the libxlDomainEventHandler callback registered with libxl.
> Documenation in $xensrc/tools/libxl/libxl_event.h states that the
> callback "may occur on any thread in which the application calls
> libxl". This can result in deadlock since many of the libvirt
> callees of libxl hold a lock on the virDomainObj they are working
> on. When the callback is invoked, it attempts to find a virDomainObj
> corresponding to the domain ID provided by libxl. Searching the
> domain obj list results in locking each obj before checking if it is
> active, and its ID equals the requested ID. Deadlock is possible
> when attempting to lock an obj that is already locked further up
> the call stack. Indeed, Max Ustermann recently reported an instance
> of this deadlock
> 
> https://www.redhat.com/archives/libvir-list/2015-November/msg00130.html
> 
> This patch moves processing of libxl events to a thread, where
> libxl_event_wait() is used to collect events. This allows processing
> libxl events asynchronously in libvirt, avoiding the deadlock.
> 
> Reported-by: max ustermann <ustermann78@xxxxxx>
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
> 
> The only reservations I have about this patch come from comments
> about libxl_event_wait in libxl_event.h
> 
> Â Like libxl_event_check but blocks if no suitable events are
> Â available, until some are.ÂÂUses libxl_osevent_beforepoll/
> Â _afterpoll so may be inefficient if very many domains are being
> Â handled by a single program.
> 
> libvirt does handle "very many domains" :-). But thus far, I haven't
> noticed any problems in my testing. The reporter expressed interest
> in testing the patch. Positive results from that testing would improve
> my confidence, as would an ACK from Ian Jackson.

Would it be possible/allowable to have your cake and eat it by using the
callbacks but having them simply enqueue the events on the libvirt side and
kick the dedicated thread for further processing?

Or we could consider whether libxl should gain such functionality.


> 
> Âsrc/libxl/libxl_domain.c | 130 ++++++++++++++++++++++-------------------
> ------
> Âsrc/libxl/libxl_domain.h |ÂÂ16 +-----
> Âsrc/libxl/libxl_driver.c |ÂÂ13 ++---
> Â3 files changed, 66 insertions(+), 93 deletions(-)
> 
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 40dcea1..0b8c292 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -380,27 +380,14 @@ virDomainDefParserConfig libxlDomainDefParserConfig
> = {
> ÂÂÂÂÂ.domainPostParseCallback = libxlDomainDefPostParse,
> Â};
> Â
> -
> -struct libxlShutdownThreadInfo
> -{
> -ÂÂÂÂlibxlDriverPrivatePtr driver;
> -ÂÂÂÂvirDomainObjPtr vm;
> -ÂÂÂÂlibxl_event *event;
> -};
> -
> -
> Âstatic void
> -libxlDomainShutdownThread(void *opaque)
> +libxlDomainShutdownEventHandler(libxlDriverPrivatePtr driver,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂvirDomainObjPtr vm,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl_event *ev)
> Â{
> -ÂÂÂÂstruct libxlShutdownThreadInfo *shutdown_info = opaque;
> -ÂÂÂÂvirDomainObjPtr vm = shutdown_info->vm;
> -ÂÂÂÂlibxl_event *ev = shutdown_info->event;
> -ÂÂÂÂlibxlDriverPrivatePtr driver = shutdown_info->driver;
> ÂÂÂÂÂvirObjectEventPtr dom_event = NULL;
> ÂÂÂÂÂlibxl_shutdown_reason xl_reason = ev-
> >u.domain_shutdown.shutdown_reason;
> -ÂÂÂÂlibxlDriverConfigPtr cfg;
> -
> -ÂÂÂÂcfg = libxlDriverConfigGet(driver);
> +ÂÂÂÂlibxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> Â
> ÂÂÂÂÂif (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> ÂÂÂÂÂÂÂÂÂgoto cleanup;
> @@ -501,74 +488,77 @@ libxlDomainShutdownThread(void *opaque)
> ÂÂÂÂÂÂÂÂÂvirObjectUnlock(vm);
> ÂÂÂÂÂif (dom_event)
> ÂÂÂÂÂÂÂÂÂlibxlDomainEventQueue(driver, dom_event);
> -ÂÂÂÂlibxl_event_free(cfg->ctx, ev);
> -ÂÂÂÂVIR_FREE(shutdown_info);
> ÂÂÂÂÂvirObjectUnref(cfg);
> Â}
> Â
> +static int
> +libxlDomainXEventsPredicate(const libxl_event *event,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂATTRIBUTE_UNUSED void *data)
> +{
> +ÂÂÂÂ/*
> +ÂÂÂÂÂ* Currently we only handle shutdown event
> +ÂÂÂÂÂ*/
> +ÂÂÂÂif (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN)
> +ÂÂÂÂÂÂÂÂreturn 1;
> +
> +ÂÂÂÂreturn 0;
> +}
> +
> Â/*
> - * Handle previously registered domain event notification from
> libxenlight.
> + * Process events from libxl using libxl_event_wait.
> Â */
> -void
> -libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event
> *event)
> +static void
> +libxlDomainXEventsProcess(void *opaque)
> Â{
> -ÂÂÂÂlibxlDriverPrivatePtr driver = data;
> +ÂÂÂÂlibxlDriverPrivatePtr driver = opaque;
> +ÂÂÂÂlibxl_event *event;
> ÂÂÂÂÂvirDomainObjPtr vm = NULL;
> -ÂÂÂÂlibxl_shutdown_reason xl_reason = event-
> >u.domain_shutdown.shutdown_reason;
> -ÂÂÂÂstruct libxlShutdownThreadInfo *shutdown_info = NULL;
> -ÂÂÂÂvirThread thread;
> -ÂÂÂÂlibxlDriverConfigPtr cfg;
> +ÂÂÂÂlibxl_shutdown_reason xl_reason;
> +ÂÂÂÂlibxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> Â
> -ÂÂÂÂif (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
> -ÂÂÂÂÂÂÂÂVIR_INFO("Unhandled event type %d", event->type);
> -ÂÂÂÂÂÂÂÂgoto error;
> -ÂÂÂÂ}
> +ÂÂÂÂwhile (1) {
> +ÂÂÂÂÂÂÂÂevent = NULL;
> +ÂÂÂÂÂÂÂÂlibxl_event_wait(cfg->ctx, &event, LIBXL_EVENTMASK_ALL,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxlDomainXEventsPredicate, NULL);
> Â
> -ÂÂÂÂ/*
> -ÂÂÂÂÂ* Similar to the xl implementation, ignore SUSPEND.ÂÂAny actions
> needed
> -ÂÂÂÂÂ* after calling libxl_domain_suspend() are handled by its callers.
> -ÂÂÂÂÂ*/
> -ÂÂÂÂif (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
> -ÂÂÂÂÂÂÂÂgoto error;
> +ÂÂÂÂÂÂÂÂif (event == NULL) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂVIR_INFO("libxl_event_wait returned a NULL event");
> +ÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> +ÂÂÂÂÂÂÂÂ}
> Â
> -ÂÂÂÂvm = virDomainObjListFindByID(driver->domains, event->domid);
> -ÂÂÂÂif (!vm) {
> -ÂÂÂÂÂÂÂÂVIR_INFO("Received event for unknown domain ID %d", event-
> >domid);
> -ÂÂÂÂÂÂÂÂgoto error;
> -ÂÂÂÂ}
> -
> -ÂÂÂÂ/*
> -ÂÂÂÂÂ* 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 error;
> -
> -ÂÂÂÂshutdown_info->driver = driver;
> -ÂÂÂÂshutdown_info->vm = vm;
> -ÂÂÂÂshutdown_info->event = (libxl_event *)event;
> -ÂÂÂÂif (virThreadCreate(&thread, false, libxlDomainShutdownThread,
> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂshutdown_info) < 0) {
> +ÂÂÂÂÂÂÂÂxl_reason = event->u.domain_shutdown.shutdown_reason;
> ÂÂÂÂÂÂÂÂÂ/*
> -ÂÂÂÂÂÂÂÂÂ* Not much we can do on error here except log it.
> +ÂÂÂÂÂÂÂÂÂ* Similar to the xl implementation, ignore SUSPEND.ÂÂAny
> actions needed
> +ÂÂÂÂÂÂÂÂÂ* after calling libxl_domain_suspend() are handled by its
> callers.
> ÂÂÂÂÂÂÂÂÂÂ*/
> -ÂÂÂÂÂÂÂÂVIR_ERROR(_("Failed to create thread to handle domain
> shutdown"));
> -ÂÂÂÂÂÂÂÂgoto error;
> +ÂÂÂÂÂÂÂÂif (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂlibxl_event_free(cfg->ctx, event);
> +ÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> +ÂÂÂÂÂÂÂÂ}
> +
> +ÂÂÂÂÂÂÂÂvm = virDomainObjListFindByID(driver->domains, event->domid);
> +ÂÂÂÂÂÂÂÂif (!vm) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂVIR_INFO("Received event for unknown domain ID %d", event-
> >domid);
> +ÂÂÂÂÂÂÂÂÂÂÂÂlibxl_event_free(cfg->ctx, event);
> +ÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> +ÂÂÂÂÂÂÂÂ}
> +
> +ÂÂÂÂÂÂÂÂlibxlDomainShutdownEventHandler(driver, vm, event);
> +ÂÂÂÂÂÂÂÂlibxl_event_free(cfg->ctx, event);
> ÂÂÂÂÂ}
> Â
> -ÂÂÂÂ/*
> -ÂÂÂÂÂ* VM is unlocked and libxl_event freed in shutdown thread
> -ÂÂÂÂÂ*/
> -ÂÂÂÂreturn;
> -
> - error:
> -ÂÂÂÂcfg = libxlDriverConfigGet(driver);
> -ÂÂÂÂ/* Cast away any const */
> -ÂÂÂÂlibxl_event_free(cfg->ctx, (libxl_event *)event);
> ÂÂÂÂÂvirObjectUnref(cfg);
> -ÂÂÂÂif (vm)
> -ÂÂÂÂÂÂÂÂvirObjectUnlock(vm);
> -ÂÂÂÂVIR_FREE(shutdown_info);
> +}
> +
> +int
> +libxlDomainXEventsInit(libxlDriverPrivatePtr driver)
> +{
> +ÂÂÂÂvirThread thread;
> +
> +ÂÂÂÂif (virThreadCreate(&thread, false, libxlDomainXEventsProcess,
> driver) < 0)
> +ÂÂÂÂÂÂÂÂreturn -1;
> +
> +ÂÂÂÂreturn 0;
> Â}
> Â
> Âvoid
> diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> index 44b3e0b..18a9ddc 100644
> --- a/src/libxl/libxl_domain.h
> +++ b/src/libxl/libxl_domain.h
> @@ -112,20 +112,8 @@ void
> ÂlibxlDomainCleanup(libxlDriverPrivatePtr driver,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂvirDomainObjPtr vm);
> Â
> -/*
> - * Note: Xen 4.3 removed the const from the event handler signature.
> - * Detect which signature to use based on
> - * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG.
> - */
> -# ifdef LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG
> -#ÂÂdefine VIR_LIBXL_EVENT_CONST /* empty */
> -# else
> -#ÂÂdefine VIR_LIBXL_EVENT_CONST const
> -# endif
> -
> -void
> -libxlDomainEventHandler(void *data,
> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂVIR_LIBXL_EVENT_CONST libxl_event *event);
> +int
> +libxlDomainXEventsInit(libxlDriverPrivatePtr driver);
> Â
> Âint
> ÂlibxlDomainAutoCoreDump(libxlDriverPrivatePtr driver,
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index fcdcbdb..050ed0f 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -503,12 +503,6 @@ static const libxl_childproc_hooks libxl_child_hooks
> = {
> Â#endif
> Â};
> Â
> -const struct libxl_event_hooks ev_hooks = {
> -ÂÂÂÂ.event_occurs_mask = LIBXL_EVENTMASK_ALL,
> -ÂÂÂÂ.event_occurs = libxlDomainEventHandler,
> -ÂÂÂÂ.disaster = NULL,
> -};
> -
> Âstatic int
> ÂlibxlAddDom0(libxlDriverPrivatePtr driver)
> Â{
> @@ -626,9 +620,6 @@ libxlStateInitialize(bool privileged,
> ÂÂÂÂÂ/* Setup child process handling.ÂÂSee $xen-
> src/tools/libxl/libxl_event.h */
> ÂÂÂÂÂlibxl_childproc_setmode(cfg->ctx, &libxl_child_hooks, cfg->ctx);
> Â
> -ÂÂÂÂ/* Register callback to handle domain events */
> -ÂÂÂÂlibxl_event_register_callbacks(cfg->ctx, &ev_hooks, libxl_driver);
> -
> ÂÂÂÂÂlibxl_driver->config = cfg;
> ÂÂÂÂÂif (virFileMakePath(cfg->stateDir) < 0) {
> ÂÂÂÂÂÂÂÂÂvirReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -683,6 +674,10 @@ libxlStateInitialize(bool privileged,
> ÂÂÂÂÂif (!(libxl_driver->xmlopt = libxlCreateXMLConf()))
> ÂÂÂÂÂÂÂÂÂgoto error;
> Â
> +ÂÂÂÂ/* Start processing events from libxl */
> +ÂÂÂÂif (libxlDomainXEventsInit(libxl_driver) < 0)
> +ÂÂÂÂÂÂÂÂgoto error;
> +
> ÂÂÂÂÂ/* Add Domain-0 */
> ÂÂÂÂÂif (libxlAddDom0(libxl_driver) < 0)
> ÂÂÂÂÂÂÂÂÂgoto error;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel