Re: [RFC] use event channel to improve suspend speed

[Brendan Cully <brendan@cs.ubc.ca>]

[-- Attachment #1: Type: text/plain, Size: 2692 bytes --]

On Friday, 11 May 2007 at 07:55, Keir Fraser wrote:
> On 11/5/07 00:00, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > It would be interesting to know what aspect of the xenstore interaction
> > is responsible for the slowdown. In particular, whether it is a fundamental
> > architectural constraint, or whether it is merely due to the poor performance
> > of the current impl. We already know from previous tests that XenD impl of
> > transactions absolutely kills performance of various XenD operations due to
> > the vast amount of unneccessary I/O it does.
> > 
> > If fixing the XenstoreD transaction code were to help suspend performance
> > too, it might be a better option than re-writing all code which touches
> > xenstore. A quick test of putting /var/lib/xenstored on a ramdisk would
> > be a way of testing whether its the I/O which is hurting suspend time.
> 
> Yes. We could go either way -- it wouldn't be too bad to add support via
> dynamic VIRQ_DOM_EXC for example, or add other things to get xenstore off
> the critical path for save/restore. But if the problem is that xenstored
> sucks it probably is worth investing a bit of time to tackle the problem
> directly and see where the time is going. We could end up with optimisations
> which have benefits beyond just save/restore.

I'm sure xenstore could be made significantly faster, but barring a
redesign maybe it's better just to use it for low-frequency
transactions with pretty loose latency expectations? Running the
suspend notification through xenstore, to xend and finally back to
xc_save (as the current code does) seems convoluted, and bound to
create opportunities for bad scheduling compared to directly notifying
xc_save.

In case there's interest, I'll attach the two patches I'm using to
speed up checkpointing (and live migration downtime). As I mentioned
earlier, the first patch should be semantically equivalent to existing
code, and cuts downtime to about 30-35ms. The second notifies xend
that the domain has been suspended asynchronously, so that final round
memory copying may begin before device migration stage 2. This is a
semantic change, but I can't think of a concrete drawback. It's a
little rough-and-ready -- suggestions for improvement are welcome.

Here are some stats on final round time (100 runs):

xen 3.1:
  avg: 93.40 ms, min: 72.59, max: 432.46, median: 85.10
patch 1 (trigger suspend via event channel):
  avg: 43.69 ms, min: 35.21, max: 409.50, median: 37.21
patch 1, /var/lib/xenstored on tmpfs:
  avg: 33.88 ms, min: 27.01, max: 369.21, median: 28.34
patch 2 (receive suspended notification via event channel):
  avg: 4.95 ms, min: 3.46, max: 14.73, median: 4.63

[-- Attachment #2: suspend-evtchn.patch --]
[-- Type: text/x-patch, Size: 6441 bytes --]

# HG changeset patch
# User Brendan Cully <brendan@cs.ubc.ca>
# Date 1180051484 25200
# Node ID 5c5b8a69c9d631399b3a139dea2026f7a11f6dd7
# Parent  6649c29d3720fa9087eade3a9c77f5785ff54279
Set up an event channel to signal guest suspend.
Have xc_save use it directly instead of waiting for xend to signal the
guest via xenstore.  This cuts the overhead for the last round in half
in my tests.

Signed-off-by: Brendan Cully <brendan@cs.ubc.ca>

diff -r 6649c29d3720 -r 5c5b8a69c9d6 linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c
--- a/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c	Thu May 24 17:04:44 2007 -0700
+++ b/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c	Thu May 24 17:04:44 2007 -0700
@@ -27,6 +27,8 @@ void (*pm_power_off)(void);
 void (*pm_power_off)(void);
 EXPORT_SYMBOL(pm_power_off);
 
+int setup_suspend_evtchn(void);
+
 void machine_emergency_restart(void)
 {
 	/* We really want to get pending console data out before we die. */
@@ -230,6 +232,7 @@ int __xen_suspend(int fast_suspend)
 	if (!suspend_cancelled) {
 		xencons_resume();
 		xenbus_resume();
+		setup_suspend_evtchn();
 	} else {
 		xenbus_suspend_cancel();
 	}
diff -r 6649c29d3720 -r 5c5b8a69c9d6 linux-2.6-xen-sparse/drivers/xen/core/reboot.c
--- a/linux-2.6-xen-sparse/drivers/xen/core/reboot.c	Thu May 24 17:04:44 2007 -0700
+++ b/linux-2.6-xen-sparse/drivers/xen/core/reboot.c	Thu May 24 17:04:44 2007 -0700
@@ -7,6 +7,7 @@
 #include <linux/sysrq.h>
 #include <asm/hypervisor.h>
 #include <xen/xenbus.h>
+#include <xen/evtchn.h>
 #include <linux/kthread.h>
 
 #ifdef HAVE_XEN_PLATFORM_COMPAT_H
@@ -194,6 +195,36 @@ static struct xenbus_watch sysrq_watch =
 	.callback = sysrq_handler
 };
 
+static irqreturn_t suspend_int(int irq, void* dev_id, struct pt_regs *ptregs)
+{
+  shutting_down = SHUTDOWN_SUSPEND;
+  schedule_work(&shutdown_work);
+
+  return IRQ_HANDLED;
+}
+
+int setup_suspend_evtchn(void)
+{
+  static int irq = -1;
+  int port;
+  char portstr[5]; /* 1024 max? */
+
+  if (irq > 0)
+    unbind_from_irqhandler(irq, NULL);
+
+  /* TODO: get other end dynamically */
+  irq = bind_listening_port_to_irqhandler(0, suspend_int, 0, "suspend", NULL);
+  if (irq <= 0) {
+    return -1;
+  }
+  port = irq_to_evtchn_port(irq);
+  printk(KERN_ERR "suspend: event channel %d\n", port);
+  sprintf(portstr, "%d", port);
+  xenbus_write(XBT_NIL, "device/suspend", "event-channel", portstr);
+
+  return 0;
+}
+
 static int setup_shutdown_watcher(void)
 {
 	int err;
@@ -212,6 +243,13 @@ static int setup_shutdown_watcher(void)
 	if (err) {
 		printk(KERN_ERR "Failed to set sysrq watcher\n");
 		return err;
+	}
+
+	/* suspend event channel */
+	err = setup_suspend_evtchn();
+	if (err) {
+	  printk(KERN_ERR "Failed to register suspend event channel\n");
+	  return err;
 	}
 
 	return 0;
diff -r 6649c29d3720 -r 5c5b8a69c9d6 tools/python/xen/xend/XendCheckpoint.py
--- a/tools/python/xen/xend/XendCheckpoint.py	Thu May 24 17:04:44 2007 -0700
+++ b/tools/python/xen/xend/XendCheckpoint.py	Thu May 24 17:04:44 2007 -0700
@@ -92,6 +92,7 @@ def save(fd, dominfo, network, live, dst
             if line == "suspend":
                 log.debug("Suspending %d ...", dominfo.getDomid())
                 dominfo.shutdown('suspend')
+            if line in ('suspend', 'suspended'):
                 dominfo.waitForShutdown()
                 dominfo.migrateDevices(network, dst, DEV_MIGRATE_STEP2,
                                        domain_name)
diff -r 6649c29d3720 -r 5c5b8a69c9d6 tools/xcutils/xc_save.c
--- a/tools/xcutils/xc_save.c	Thu May 24 17:04:44 2007 -0700
+++ b/tools/xcutils/xc_save.c	Thu May 24 17:04:44 2007 -0700
@@ -23,8 +23,13 @@
 #include <xenctrl.h>
 #include <xenguest.h>
 
+#include <errno.h>
+
 /* defined in xc_linux_save. Yes, this is cheezy. */
 extern int do_last_round;
+
+static int xce = -1;
+static int suspend_evtchn = -1;
 
 /**
  * Issue a suspend request through stdout, and receive the acknowledgement
@@ -32,9 +37,24 @@ extern int do_last_round;
  */
 static int suspend(int domid)
 {
+    static char* suspend = "suspend\n";
+    static char* suspended = "suspended\n";
+
     char ans[30];
-
-    printf("suspend\n");
+    char* msg;
+    int rc;
+
+    msg = suspend;
+
+    if (suspend_evtchn > 0) {
+      rc = xc_evtchn_notify(xce, suspend_evtchn);
+      if (rc < 0)
+	fprintf(stderr, "failed to notify suspend event channel: %d\n", rc);
+      else
+	msg = suspended;
+    }
+
+    printf(msg);
     fflush(stdout);
 
     return (fgets(ans, sizeof(ans), stdin) != NULL &&
@@ -164,6 +184,68 @@ static void sigusr1(int sig)
   do_last_round = 1;
 }
 
+static int setup_suspend_evtchn(unsigned int domid)
+{
+  struct xs_handle* xs;
+  char path[128];
+  char *portstr;
+  unsigned int plen;
+  int port;
+  int rc = -1;
+
+  xce = xc_evtchn_open();
+  if (xce < 0) {
+    fprintf(stderr, "failed to open event channel handle\n");
+    return -1;
+  }
+
+  xs = xs_daemon_open_readonly();
+  if (!xs) {
+    fprintf(stderr, "failed to open xenstore handle\n");
+    goto xce_err;
+  }
+
+  sprintf(path, "/local/domain/%d/device/suspend/event-channel", domid);
+
+  portstr = xs_read(xs, XBT_NULL, path, &plen);
+  if (!portstr || !plen) {
+    fprintf(stderr, "failed to read suspend event channel\n");
+    goto xs_err;
+  }
+  port = atoi(portstr);
+  free(portstr);
+
+  fprintf(stderr, "binding to suspend evtchn %u:%d\n", domid, port);
+
+  suspend_evtchn = xc_evtchn_bind_interdomain(xce, domid, port);
+  if (suspend_evtchn < 0) {
+    fprintf(stderr, "failed to bind suspend event channel: %d (%s)\n",
+	    suspend_evtchn, strerror(errno));
+    goto xs_err;
+  }
+
+  rc = 0;
+
+  xs_err:
+  xs_daemon_close(xs);
+  xce_err:
+  if (xce >= 0 && rc < 0) {
+    xc_evtchn_close(xce);
+    xce = -1;
+  }
+
+  return rc;
+}
+
+static void release_suspend_evtchn(void)
+{
+  if (xce >= 0) {
+    if (suspend_evtchn > 0)
+      xc_evtchn_unbind(xce, suspend_evtchn);
+    xc_evtchn_close(xce);
+  }
+}
+
 int
 main(int argc, char **argv)
 {
@@ -188,10 +270,14 @@ main(int argc, char **argv)
     sa.sa_flags = SA_RESTART;
     sigaction(SIGUSR1, &sa, NULL);
 
+    setup_suspend_evtchn(domid);
+
     ret = xc_domain_save(xc_fd, io_fd, domid, maxit, max_f, flags, 
                          &suspend, !!(flags & XCFLAGS_HVM),
                          &init_qemu_maps, &qemu_flip_buffer);
 
+    release_suspend_evtchn();
+
     xc_interface_close(xc_fd);
 
     return ret;

[-- Attachment #3: subscribe-suspend.patch --]
[-- Type: text/x-patch, Size: 8354 bytes --]

# HG changeset patch
# User Brendan Cully <brendan@cs.ubc.ca>
# Date 1180051484 25200
# Node ID fcc62feb1d2c92cfece16c0b11b8bf5ac3c411cb
# Parent  5c5b8a69c9d631399b3a139dea2026f7a11f6dd7
Add facility to subscribe to a domain via event channel.
This event channel will be notified when the domain transitions to the
suspended state, which can be much faster than raising VIRQ_DOM_EXC
and waiting for the notification to be propagated via xenstore.

Signed-off-by: Brendan Cully <brendan@cs.ubc.ca>

diff -r 5c5b8a69c9d6 -r fcc62feb1d2c tools/libxc/xc_domain.c
--- a/tools/libxc/xc_domain.c	Thu May 24 17:04:44 2007 -0700
+++ b/tools/libxc/xc_domain.c	Thu May 24 17:04:44 2007 -0700
@@ -696,6 +696,17 @@ int xc_get_hvm_param(int handle, domid_t
     return rc;
 }
 
+int xc_dom_subscribe(int xc_handle, domid_t dom, evtchn_port_t port)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_subscribe;
+    domctl.domain = dom;
+    domctl.u.subscribe.port = port;
+
+    return do_domctl(xc_handle, &domctl);
+}
+
 /*
  * Local variables:
  * mode: C
diff -r 5c5b8a69c9d6 -r fcc62feb1d2c tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h	Thu May 24 17:04:44 2007 -0700
+++ b/tools/libxc/xenctrl.h	Thu May 24 17:04:44 2007 -0700
@@ -728,6 +728,12 @@ evtchn_port_t xc_evtchn_pending(int xce_
  */
 int xc_evtchn_unmask(int xce_handle, evtchn_port_t port);
 
+/*
+ * Subscribe to state changes in a domain via evtchn.
+ * Returns -1 on failure, in which case errno will be set appropriately.
+ */
+int xc_dom_subscribe(int xc_handle, domid_t domid, evtchn_port_t port);
+
 /**************************
  * GRANT TABLE OPERATIONS *
  **************************/
diff -r 5c5b8a69c9d6 -r fcc62feb1d2c tools/python/xen/xend/XendCheckpoint.py
--- a/tools/python/xen/xend/XendCheckpoint.py	Thu May 24 17:04:44 2007 -0700
+++ b/tools/python/xen/xend/XendCheckpoint.py	Thu May 24 17:04:44 2007 -0700
@@ -92,7 +92,7 @@ def save(fd, dominfo, network, live, dst
             if line == "suspend":
                 log.debug("Suspending %d ...", dominfo.getDomid())
                 dominfo.shutdown('suspend')
-            if line in ('suspend', 'suspended'):
+            if line in ('suspend'):
                 dominfo.waitForShutdown()
                 dominfo.migrateDevices(network, dst, DEV_MIGRATE_STEP2,
                                        domain_name)
diff -r 5c5b8a69c9d6 -r fcc62feb1d2c tools/xcutils/xc_save.c
--- a/tools/xcutils/xc_save.c	Thu May 24 17:04:44 2007 -0700
+++ b/tools/xcutils/xc_save.c	Thu May 24 17:04:44 2007 -0700
@@ -28,8 +28,12 @@
 /* defined in xc_linux_save. Yes, this is cheezy. */
 extern int do_last_round;
 
+unsigned int xc_fd;
 static int xce = -1;
+/* notify this to cause guest to start suspend */
 static int suspend_evtchn = -1;
+/* get call back on this when guest has finished suspend */
+static int suspended_evtchn = -1;
 
 /**
  * Issue a suspend request through stdout, and receive the acknowledgement
@@ -50,8 +54,28 @@ static int suspend(int domid)
       rc = xc_evtchn_notify(xce, suspend_evtchn);
       if (rc < 0)
 	fprintf(stderr, "failed to notify suspend event channel: %d\n", rc);
-      else
+      else {
 	msg = suspended;
+	if (suspended_evtchn > 0) {
+	  do {
+	    rc = xc_evtchn_pending(xce);
+	  } while (rc > 0 && rc != suspended_evtchn);
+	  if (rc <= 0) {
+	    fprintf(stderr, "failed to received suspend notification: %d\n", rc);
+	    return 0;
+	  }
+	  if (xc_evtchn_unmask(xce, suspended_evtchn) < 0) {
+	    fprintf(stderr, "failed to unmask suspend notification channel: %d\n",
+		    rc);
+	    return 0;
+	  }
+
+	  printf(msg);
+	  fflush(stdout);
+
+	  return 1;
+	}
+      }
     }
 
     printf(msg);
@@ -224,8 +248,35 @@ static int setup_suspend_evtchn(unsigned
     goto xs_err;
   }
 
+  suspended_evtchn = xc_evtchn_bind_unbound_port(xce, domid);
+  if (suspended_evtchn < 0) {
+    fprintf(stderr,
+	    "failed to bind suspend notification event channel: %d (%s)\n",
+	    suspended_evtchn, strerror(errno));
+    goto se_err;
+  }
+
+  rc = xc_dom_subscribe(xc_fd, domid, suspended_evtchn);
+  if (rc < 0) {
+    fprintf(stderr,
+            "failed to subscribe to domain: %d (%s)\n", rc, strerror(errno));
+    goto se2_err;
+  }
+  fprintf(stderr, "subscribed to domain %d on port %d\n",
+	  domid, suspended_evtchn);
+
   rc = 0;
 
+  se2_err:
+  if (rc < 0) {
+    xc_evtchn_unbind(xce, suspended_evtchn);
+    suspended_evtchn = -1;
+  }
+  se_err:
+  if (rc < 0) {
+    xc_evtchn_unbind(xce, suspend_evtchn);
+    suspend_evtchn = -1;
+  }
   xs_err:
   xs_daemon_close(xs);
   xce_err:
@@ -237,8 +288,13 @@ static int setup_suspend_evtchn(unsigned
   return rc;
 }
 
-static void release_suspend_evtchn(void)
-{
+static void release_suspend_evtchn(unsigned int domid)
+{
+  /* TODO: teach xen to clean up if port is unbound */
+  if (suspended_evtchn > 0) {
+    xc_dom_subscribe(xc_fd, domid, 0);
+    suspended_evtchn = 0;
+  }
   if (xce >= 0) {
     if (suspend_evtchn > 0)
       xc_evtchn_unbind(xce, suspend_evtchn);
@@ -249,7 +305,7 @@ int
 int
 main(int argc, char **argv)
 {
-    unsigned int xc_fd, io_fd, domid, maxit, max_f, flags; 
+    unsigned int io_fd, domid, maxit, max_f, flags; 
     int ret;
     struct sigaction sa;
 
@@ -276,7 +332,7 @@ main(int argc, char **argv)
                          &suspend, !!(flags & XCFLAGS_HVM),
                          &init_qemu_maps, &qemu_flip_buffer);
 
-    release_suspend_evtchn();
+    release_suspend_evtchn(domid);
 
     xc_interface_close(xc_fd);
 
diff -r 5c5b8a69c9d6 -r fcc62feb1d2c xen/common/domain.c
--- a/xen/common/domain.c	Thu May 24 17:04:44 2007 -0700
+++ b/xen/common/domain.c	Thu May 24 17:04:44 2007 -0700
@@ -103,7 +103,13 @@ static void __domain_finalise_shutdown(s
     for_each_vcpu ( d, v )
         vcpu_sleep_nosync(v);
 
-    send_guest_global_virq(dom0, VIRQ_DOM_EXC);
+    if ( d->shutdown_code == SHUTDOWN_suspend
+         && d->suspend_evtchn > 0 )
+    {
+        evtchn_set_pending(dom0->vcpu[0], d->suspend_evtchn);
+    }
+    else
+        send_guest_global_virq(dom0, VIRQ_DOM_EXC);
 }
 
 static void vcpu_check_shutdown(struct vcpu *v)
diff -r 5c5b8a69c9d6 -r fcc62feb1d2c xen/common/domctl.c
--- a/xen/common/domctl.c	Thu May 24 17:04:44 2007 -0700
+++ b/xen/common/domctl.c	Thu May 24 17:04:44 2007 -0700
@@ -707,6 +707,21 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domc
     }
     break;
 
+    case XEN_DOMCTL_subscribe:
+    {
+        struct domain *d;
+
+        ret = -ESRCH;
+        d = rcu_lock_domain_by_id(op->domain);
+        if ( d != NULL )
+        {
+            d->suspend_evtchn = op->u.subscribe.port;
+            rcu_unlock_domain(d);
+            ret = 0;
+        }
+    }
+    break;
+
     default:
         ret = arch_do_domctl(op, u_domctl);
         break;
diff -r 5c5b8a69c9d6 -r fcc62feb1d2c xen/include/public/domctl.h
--- a/xen/include/public/domctl.h	Thu May 24 17:04:44 2007 -0700
+++ b/xen/include/public/domctl.h	Thu May 24 17:04:44 2007 -0700
@@ -429,7 +429,13 @@ typedef struct xen_domctl_sendtrigger xe
 typedef struct xen_domctl_sendtrigger xen_domctl_sendtrigger_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t);
 
- 
+#define XEN_DOMCTL_subscribe         29
+struct xen_domctl_subscribe {
+    uint32_t port;      /* IN */
+};
+typedef struct xen_domctl_subscribe xen_domctl_subscribe_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_subscribe_t);
+
 struct xen_domctl {
     uint32_t cmd;
     uint32_t interface_version; /* XEN_DOMCTL_INTERFACE_VERSION */
@@ -459,6 +465,7 @@ struct xen_domctl {
         struct xen_domctl_hvmcontext        hvmcontext;
         struct xen_domctl_address_size      address_size;
         struct xen_domctl_sendtrigger       sendtrigger;
+        struct xen_domctl_subscribe         subscribe;
         uint8_t                             pad[128];
     } u;
 };
diff -r 5c5b8a69c9d6 -r fcc62feb1d2c xen/include/xen/sched.h
--- a/xen/include/xen/sched.h	Thu May 24 17:04:44 2007 -0700
+++ b/xen/include/xen/sched.h	Thu May 24 17:04:44 2007 -0700
@@ -201,6 +201,10 @@ struct domain
     bool_t           is_shut_down;     /* fully shut down? */
     int              shutdown_code;
 
+    /* If this is not 0, send suspend notification here instead of
+     * raising DOM_EXC */
+    int              suspend_evtchn;
+
     atomic_t         pause_count;
 
     unsigned long    vm_assist;

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel