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 5f5417b7358ce386ad93d38f3fc408002f006766..9cbf877179d2f2d0848f6b342219a50cf1cd93d0 100644 (file)
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1110,6 +1110,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 448f3d2314419b160b2ee80f682cad6fe9e73aca..1b7bc44800c5702c559d03a5b8a03ca97662df4c 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -722,18 +722,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.
      *
@@ -746,13 +739,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)) {
@@ -2673,9 +2683,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;
         }
 
@@ -6032,6 +6039,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 acc284026fbb0b0e4f19e5622844b01b039e6806..6576069f8f9de7dec264d5d28be707d210bdbd15 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -332,12 +332,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 bb040712f9d0331826b8eb67a6833b889932ccef..b81bd6b84d08bb3e808bde9d1ee2ff869d560065 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -908,14 +908,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) {
@@ -951,7 +945,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 fb001061a64a12d16ae7ea2869dd057d62e63703..ccd82025ad0c9f267e793ccd8d619b03ddd19b74 100644 (file)
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -144,6 +144,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 aadb0c7a52cc9d53ec098ec1ba309851dfe58a27..d2dcd02732940fbf8e345c78d9e6d9bd9ede9371 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1782,13 +1782,28 @@ 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 vlan_splinters_changed;
-    bool datapath_destroyed;
     bool database_changed;
     struct bridge *br;
 
@@ -1811,15 +1826,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,
@@ -1847,7 +1855,7 @@ bridge_run(void)
         }
     }
 
-    if (database_changed || datapath_destroyed || vlan_splinters_changed) {
+    if (database_changed || vlan_splinters_changed) {
         if (cfg) {
             struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
 

diff --git a/vswitchd/bridge.h b/vswitchd/bridge.h
index 32b581e2d63c2becd652366e614e1962a069d983..2d3883384bd32d7726b5d323652a8431ed4a0676 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 bdc3533094aacea0157f8cf68a3741c6f8ad519b..301d073905fd1f613ec9c395969dea2f4965580b 100644 (file)
--- a/vswitchd/ovs-vswitchd.c
+++ b/vswitchd/ovs-vswitchd.c
@@ -92,7 +92,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();