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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
---
  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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]