bridge: Rate limit port creations and deletions.

In some datapaths, adding or deleting OpenFlow ports can take quite
a bit of time.  If there are lots of OpenFlow ports which needed to
be added in a run loop, this can cause Open vSwitch to lock up and
stop setting up flows while trying to catch up.  This patch lessons
the severity of the problem by only doing a few OpenFlow port adds
or deletions per run loop allowing other work to be done in
between.

Bug #10672.
Signed-off-by: Ethan Jackson <ethan@nicira.com>

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index e5b546bf4082faa12edd70916e33452cd9736191..22a37069c793062d42041f8bf7b4ea3c7a1b1097 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -143,6 +143,17 @@ static long long int stats_timer = LLONG_MIN;
 #define DB_LIMIT_INTERVAL (1 * 1000) /* In milliseconds. */
 static long long int db_limiter = LLONG_MIN;
 
+/* In some datapaths, creating and destroying OpenFlow ports can be extremely
+ * expensive.  This can cause bridge_reconfigure() to take a long time during
+ * which no other work can be done.  To deal with this problem, we limit port
+ * adds and deletions to a window of OFP_PORT_ACTION_WINDOW milliseconds per
+ * call to bridge_reconfigure().  If there is more work to do after the limit
+ * is reached, 'need_reconfigure', is flagged and it's done on the next loop.
+ * This allows the rest of the code to catch up on important things like
+ * forwarding packets. */
+#define OFP_PORT_ACTION_WINDOW 10
+static bool need_reconfigure = false;
+
 static void add_del_bridges(const struct ovsrec_open_vswitch *);
 static void bridge_del_ofprotos(void);
 static bool bridge_add_ofprotos(struct bridge *);
@@ -155,8 +166,10 @@ static size_t bridge_get_controllers(const struct bridge *br,
                                      struct ovsrec_controller ***controllersp);
 static void bridge_add_del_ports(struct bridge *,
                                  const unsigned long int *splinter_vlans);
-static void bridge_add_ofproto_ports(struct bridge *);
-static void bridge_del_ofproto_ports(struct bridge *);
+static void bridge_add_ofproto_ports(struct bridge *,
+                                     long long int *port_action_timer);
+static void bridge_del_ofproto_ports(struct bridge *,
+                                     long long int *port_action_timer);
 static void bridge_refresh_ofp_port(struct bridge *);
 static void bridge_configure_datapath_id(struct bridge *);
 static void bridge_configure_flow_eviction_threshold(struct bridge *);
@@ -395,6 +408,7 @@ static void
 bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
 {
     unsigned long int *splinter_vlans;
+    long long int port_action_timer;
     struct sockaddr_in *managers;
     struct bridge *br, *next;
     int sflow_bridge_number;
@@ -402,6 +416,9 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
 
     COVERAGE_INC(bridge_reconfigure);
 
+    port_action_timer = LLONG_MAX;
+    need_reconfigure = false;
+
     /* Create and destroy "struct bridge"s, "struct port"s, and "struct
      * iface"s according to 'ovs_cfg', with only very minimal configuration
      * otherwise.
@@ -424,7 +441,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
     bridge_del_ofprotos();
     HMAP_FOR_EACH (br, node, &all_bridges) {
         if (br->ofproto) {
-            bridge_del_ofproto_ports(br);
+            bridge_del_ofproto_ports(br, &port_action_timer);
         }
     }
 
@@ -437,7 +454,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
     HMAP_FOR_EACH_SAFE (br, next, node, &all_bridges) {
         if (!br->ofproto) {
             if (bridge_add_ofprotos(br)) {
-                bridge_del_ofproto_ports(br);
+                bridge_del_ofproto_ports(br, &port_action_timer);
             } else {
                 bridge_destroy(br);
             }
@@ -445,7 +462,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
     }
     HMAP_FOR_EACH (br, node, &all_bridges) {
         bridge_refresh_ofp_port(br);
-        bridge_add_ofproto_ports(br);
+        bridge_add_ofproto_ports(br, &port_action_timer);
     }
 
     /* Complete the configuration. */
@@ -481,9 +498,11 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
     }
     free(managers);
 
-    /* ovs-vswitchd has completed initialization, so allow the process that
-     * forked us to exit successfully. */
-    daemonize_complete();
+    if (!need_reconfigure) {
+        /* ovs-vswitchd has completed initialization, so allow the process that
+         * forked us to exit successfully. */
+        daemonize_complete();
+    }
 }
 
 /* Iterate over all ofprotos and delete any of them that do not have a
@@ -1032,6 +1051,23 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg)
     shash_destroy(&new_br);
 }
 
+static bool
+may_port_action(long long int *port_action_timer)
+{
+    if (need_reconfigure) {
+        return false;
+    }
+
+    time_refresh();
+    if (*port_action_timer == LLONG_MAX) {
+        *port_action_timer = time_msec() + OFP_PORT_ACTION_WINDOW;
+    } else if (time_msec() > *port_action_timer) {
+        need_reconfigure = true;
+        return false;
+    }
+    return true;
+}
+
 /* Delete each ofproto port on 'br' that doesn't have a corresponding "struct
  * iface".
  *
@@ -1039,7 +1075,8 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg)
  * port already belongs to a different datapath, so we must do all port
  * deletions before any port additions. */
 static void
-bridge_del_ofproto_ports(struct bridge *br)
+bridge_del_ofproto_ports(struct bridge *br,
+                         long long int *port_action_timer)
 {
     struct ofproto_port_dump dump;
     struct ofproto_port ofproto_port;
@@ -1070,10 +1107,16 @@ bridge_del_ofproto_ports(struct bridge *br)
                 || !strcmp(netdev_get_type(iface->netdev), type))) {
             continue;
         }
-        error = ofproto_port_del(br->ofproto, ofproto_port.ofp_port);
-        if (error) {
-            VLOG_WARN("bridge %s: failed to remove %s interface (%s)",
-                      br->name, name, strerror(error));
+
+        if (may_port_action(port_action_timer)) {
+            error = ofproto_port_del(br->ofproto, ofproto_port.ofp_port);
+            if (error) {
+                VLOG_WARN("bridge %s: failed to remove %s interface (%s)",
+                          br->name, name, strerror(error));
+            } else {
+                VLOG_INFO("bridge %s: removed interface %s (%d)", br->name,
+                          name, ofproto_port.ofp_port);
+            }
         }
         if (iface) {
             netdev_close(iface->netdev);
@@ -1131,7 +1174,8 @@ bridge_refresh_ofp_port(struct bridge *br)
  * Delete any "struct iface" for which this fails.
  * Delete any "struct port" that thereby ends up with no ifaces. */
 static void
-bridge_add_ofproto_ports(struct bridge *br)
+bridge_add_ofproto_ports(struct bridge *br,
+                         long long int *port_action_timer)
 {
     struct port *port, *next_port;
 
@@ -1142,6 +1186,12 @@ bridge_add_ofproto_ports(struct bridge *br)
         LIST_FOR_EACH_SAFE (iface, next_iface, port_elem, &port->ifaces) {
             int error;
 
+            if (iface->ofp_port < 0 && !may_port_action(port_action_timer)) {
+                iface_clear_db_record(iface->cfg);
+                iface_destroy(iface);
+                continue;
+            }
+
             /* Open the netdev. */
             if (!iface->netdev) {
                 error = netdev_open(iface->name, iface->type, &iface->netdev);
@@ -1186,6 +1236,8 @@ bridge_add_ofproto_ports(struct bridge *br)
                 error = ofproto_port_add(br->ofproto, iface->netdev,
                                          &ofp_port);
                 if (!error) {
+                    VLOG_INFO("bridge %s: added interface %s (%d)", br->name,
+                              iface->name, ofp_port);
                     iface_set_ofp_port(iface, ofp_port);
                 } else {
                     netdev_close(iface->netdev);
@@ -1214,6 +1266,8 @@ bridge_add_ofproto_ports(struct bridge *br)
                 }
                 if (!ofproto_port_query_by_name(br->ofproto, port->name,
                                                 &ofproto_port)) {
+                    VLOG_INFO("bridge %s: removed interface %s (%d)",
+                              br->name, port->name, ofproto_port.ofp_port);
                     ofproto_port_del(br->ofproto, ofproto_port.ofp_port);
                     ofproto_port_destroy(&ofproto_port);
                 }
@@ -1221,8 +1275,11 @@ bridge_add_ofproto_ports(struct bridge *br)
                 iface_destroy(iface);
             }
         }
+
         if (list_is_empty(&port->ifaces)) {
-            VLOG_WARN("%s port has no interfaces, dropping", port->name);
+            if (!need_reconfigure) {
+                VLOG_WARN("%s port has no interfaces, dropping", port->name);
+            }
             port_destroy(port);
             continue;
         }
@@ -1236,6 +1293,9 @@ bridge_add_ofproto_ports(struct bridge *br)
 
                 error = netdev_open(port->name, "internal", &netdev);
                 if (!error) {
+                    /* There are unlikely to be a great number of fake
+                     * interfaces so we don't bother rate limiting their
+                     * creation. */
                     ofproto_port_add(br->ofproto, netdev, NULL);
                     netdev_close(netdev);
                 } else {
@@ -1923,7 +1983,8 @@ bridge_run(void)
         }
     }
 
-    if (ovsdb_idl_get_seqno(idl) != idl_seqno || vlan_splinters_changed) {
+    if (need_reconfigure || ovsdb_idl_get_seqno(idl) != idl_seqno
+        || vlan_splinters_changed) {
         idl_seqno = ovsdb_idl_get_seqno(idl);
         if (cfg) {
             struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
@@ -2028,6 +2089,11 @@ void
 bridge_wait(void)
 {
     ovsdb_idl_wait(idl);
+
+    if (need_reconfigure) {
+        poll_immediate_wake();
+    }
+
     if (!hmap_is_empty(&all_bridges)) {
         struct bridge *br;
 
@@ -2618,9 +2684,6 @@ port_create(struct bridge *br, const struct ovsrec_port *cfg)
     list_init(&port->ifaces);
 
     hmap_insert(&br->ports, &port->hmap_node, hash_string(port->name, 0));
-
-    VLOG_INFO("created port %s on bridge %s", port->name, br->name);
-
     return port;
 }
 
@@ -2713,9 +2776,6 @@ port_destroy(struct port *port)
         }
 
         hmap_remove(&br->ports, &port->hmap_node);
-
-        VLOG_INFO("destroyed port %s on bridge %s", port->name, br->name);
-
         free(port->name);
         free(port);
     }