ofproto: Add "fast path".

The key to getting good performance on the netperf CRR test seems to be to
handle the first packet of each new flow as quickly as possible.  Until
now, we've only had one opportunity to do that on each trip through the
main poll loop.  One way to improve would be to make that poll loop
circulate more quickly.  My experiments show, however, that even just
commenting out the slower parts of the poll loop yield minimal improvement.

This commit takes another approach.  Instead of making the poll loop
overall faster, it invokes the performance-critical parts of it more than
once during each poll loop.

My measurements show that this commit improves netperf CRR performance by
24% versus the previous commit, for an overall improvement of 87% versus
the baseline just before the commit that removed the poll_fd_woke().  With
this commit, ovs-benchmark performance has also improved by 13% overall
since that baseline.

diff --git a/lib/dpif.c b/lib/dpif.c
index 68a95f6..bd9814b 100644 (file)
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1095,6 +1095,8 @@ dpif_recv(struct dpif *dpif, struct dpif_upcall *upcall)
 
         ds_destroy(&flow);
         free(packet);
+    } else if (error && error != EAGAIN) {
+        log_operation(dpif, "recv", error);
     }
     return error;
 }

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4cf5370..12825d7 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -582,18 +582,11 @@ destruct(struct ofproto *ofproto_)
 }
 
 static int
-run(struct ofproto *ofproto_)
+run_fast(struct ofproto *ofproto_)
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-    struct ofport_dpif *ofport;
-    struct ofbundle *bundle;
     unsigned int work;
 
-    if (!clogged) {
-        complete_operations(ofproto);
-    }
-    dpif_run(ofproto->dpif);
-
     /* Handle one or more batches of upcalls, until there's nothing left to do
      * or until we do a fixed total amount of work.
      *
@@ -606,13 +599,30 @@ run(struct ofproto *ofproto_)
     work = 0;
     while (work < FLOW_MISS_MAX_BATCH) {
         int retval = handle_upcalls(ofproto, FLOW_MISS_MAX_BATCH - work);
-        if (retval < 0) {
+        if (retval <= 0) {
             return -retval;
-        } else if (!retval) {
-            break;
-        } else {
-            work += retval;
         }
+        work += retval;
+    }
+    return 0;
+}
+
+static int
+run(struct ofproto *ofproto_)
+{
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+    struct ofport_dpif *ofport;
+    struct ofbundle *bundle;
+    int error;
+
+    if (!clogged) {
+        complete_operations(ofproto);
+    }
+    dpif_run(ofproto->dpif);
+
+    error = run_fast(ofproto_);
+    if (error) {
+        return error;
     }
 
     if (timer_expired(&ofproto->next_expiration)) {
@@ -2351,9 +2361,6 @@ handle_upcalls(struct ofproto_dpif *ofproto, unsigned int max_batch)
 
         error = dpif_recv(ofproto->dpif, upcall);
         if (error) {
-            if (error == ENODEV && n_misses == 0) {
-                return -ENODEV;
-            }
             break;
         }
 
@@ -5394,6 +5401,7 @@ const struct ofproto_class ofproto_dpif_class = {
     destruct,
     dealloc,
     run,
+    run_fast,
     wait,
     flush,
     get_features,

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 8908dc0..17fd502 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -323,12 +323,17 @@ struct ofproto_class {
      *   - Call ofproto_rule_expire() for each OpenFlow flow that has reached
      *     its hard_timeout or idle_timeout, to expire the flow.
      *
-     * Returns 0 if successful, otherwise a positive errno value.  The ENODEV
-     * return value specifically means that the datapath underlying 'ofproto'
-     * has been destroyed (externally, e.g. by an admin running ovs-dpctl).
-     */
+     * Returns 0 if successful, otherwise a positive errno value. */
     int (*run)(struct ofproto *ofproto);
 
+    /* Performs periodic activity required by 'ofproto' that needs to be done
+     * with the least possible latency.
+     *
+     * This is run multiple times per main loop.  An ofproto provider may
+     * implement it or not, according to whether it provides a performance
+     * boost for that ofproto implementation. */
+    int (*run_fast)(struct ofproto *ofproto);
+
     /* Causes the poll loop to wake up when 'ofproto''s 'run' function needs to
      * be called, e.g. by calling the timer or fd waiting functions in
      * poll-loop.h.  */

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index bfdcf3e..8ff552c 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -862,14 +862,8 @@ ofproto_run(struct ofproto *p)
     int error;
 
     error = p->ofproto_class->run(p);
-    if (error == ENODEV) {
-        /* Someone destroyed the datapath behind our back.  The caller
-         * better destroy us and give up, because we're just going to
-         * spin from here on out. */
-        static struct vlog_rate_limit rl2 = VLOG_RATE_LIMIT_INIT(1, 5);
-        VLOG_ERR_RL(&rl2, "%s: datapath was destroyed externally",
-                    p->name);
-        return ENODEV;
+    if (error && error != EAGAIN) {
+        VLOG_ERR_RL(&rl, "%s: run failed (%s)", p->name, strerror(error));
     }
 
     if (p->ofproto_class->port_poll) {
@@ -905,7 +899,26 @@ ofproto_run(struct ofproto *p)
         NOT_REACHED();
     }
 
-    return 0;
+    return error;
+}
+
+/* Performs periodic activity required by 'ofproto' that needs to be done
+ * with the least possible latency.
+ *
+ * It makes sense to call this function a couple of times per poll loop, to
+ * provide a significant performance boost on some benchmarks with the
+ * ofproto-dpif implementation. */
+int
+ofproto_run_fast(struct ofproto *p)
+{
+    int error;
+
+    error = p->ofproto_class->run_fast ? p->ofproto_class->run_fast(p) : 0;
+    if (error && error != EAGAIN) {
+        VLOG_ERR_RL(&rl, "%s: fastpath run failed (%s)",
+                    p->name, strerror(error));
+    }
+    return error;
 }
 
 void

diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index eed4e50..85e9208 100644 (file)
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -139,6 +139,7 @@ void ofproto_destroy(struct ofproto *);
 int ofproto_delete(const char *name, const char *type);
 
 int ofproto_run(struct ofproto *);
+int ofproto_run_fast(struct ofproto *);
 void ofproto_wait(struct ofproto *);
 bool ofproto_is_alive(const struct ofproto *);
 

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 9b01db5..535191e 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1763,12 +1763,27 @@ refresh_cfm_stats(void)
     }
 }
 
+/* Performs periodic activity required by bridges that needs to be done with
+ * the least possible latency.
+ *
+ * It makes sense to call this function a couple of times per poll loop, to
+ * provide a significant performance boost on some benchmarks with ofprotos
+ * that use the ofproto-dpif implementation. */
+void
+bridge_run_fast(void)
+{
+    struct bridge *br;
+
+    HMAP_FOR_EACH (br, node, &all_bridges) {
+        ofproto_run_fast(br->ofproto);
+    }
+}
+
 void
 bridge_run(void)
 {
     const struct ovsrec_open_vswitch *cfg;
 
-    bool datapath_destroyed;
     bool database_changed;
     struct bridge *br;
 
@@ -1791,15 +1806,8 @@ bridge_run(void)
     cfg = ovsrec_open_vswitch_first(idl);
 
     /* Let each bridge do the work that it needs to do. */
-    datapath_destroyed = false;
     HMAP_FOR_EACH (br, node, &all_bridges) {
-        int error = ofproto_run(br->ofproto);
-        if (error) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-            VLOG_ERR_RL(&rl, "bridge %s: datapath was destroyed externally, "
-                        "forcing reconfiguration", br->name);
-            datapath_destroyed = true;
-        }
+        ofproto_run(br->ofproto);
     }
 
     /* Re-configure SSL.  We do this on every trip through the main loop,
@@ -1815,7 +1823,7 @@ bridge_run(void)
         stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert);
     }
 
-    if (database_changed || datapath_destroyed) {
+    if (database_changed) {
         if (cfg) {
             struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
 

diff --git a/vswitchd/bridge.h b/vswitchd/bridge.h
index 32b581e..2d38833 100644 (file)
--- a/vswitchd/bridge.h
+++ b/vswitchd/bridge.h
@@ -1,4 +1,4 @@
-/* Copyright (c) 2008, 2009, 2010 Nicira Networks
+/* Copyright (c) 2008, 2009, 2010, 2011 Nicira Networks
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@ void bridge_init(const char *remote);
 void bridge_exit(void);
 
 void bridge_run(void);
+void bridge_run_fast(void);
 void bridge_wait(void);
 
 #endif /* bridge.h */

diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
index 359e3ab..177c73c 100644 (file)
--- a/vswitchd/ovs-vswitchd.c
+++ b/vswitchd/ovs-vswitchd.c
@@ -91,7 +91,9 @@ main(int argc, char *argv[])
         if (signal_poll(sighup)) {
             vlog_reopen_log_file();
         }
+        bridge_run_fast();
         bridge_run();
+        bridge_run_fast();
         unixctl_server_run(unixctl);
         netdev_run();