From: Ben Pfaff <blp@nicira.com>
Date: Mon, 28 Nov 2011 18:35:15 +0000 (-0800)
Subject: ofproto: Add "fast path".
X-Git-Tag: v1.4.0~71
X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=5fcc0d004ae0aa080deed51c842d074a83ec1f67;p=sliver-openvswitch.git

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 5f5417b73..9cbf87717 100644
--- 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 448f3d231..1b7bc4480 100644
--- 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 acc284026..6576069f8 100644
--- 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 bb040712f..b81bd6b84 100644
--- 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 fb001061a..ccd82025a 100644
--- 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 aadb0c7a5..d2dcd0273 100644
--- 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 32b581e2d..2d3883384 100644
--- 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 bdc353309..301d07390 100644
--- 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();