[ovs-dev] [PATCHv2] bridge: Rate limit port creations and deletions.
Ethan Jackson
ethan at nicira.com
Wed Apr 11 20:04:37 UTC 2012
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 at nicira.com>
---
I've tested this version on my virtual machine, but still need to try it on a
real server before merging it. I don't expect it to need to change so I'm
sending it out for review now.
---
vswitchd/bridge.c | 108 ++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 86 insertions(+), 22 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index e5b546b..dcf781d 100644
--- 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,10 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
COVERAGE_INC(bridge_reconfigure);
+ time_refresh();
+ port_action_timer = time_msec() + OFP_PORT_ACTION_WINDOW;
+ 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 +442,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 +455,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 +463,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 +499,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 +1052,26 @@ 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 (time_msec() < port_action_timer) {
+ time_refresh();
+ }
+
+ if (time_msec() < port_action_timer) {
+ return true;
+ }
+
+ /* Guarantee at least one port is acted on per run loop. */
+ if (!need_reconfigure) {
+ need_reconfigure = true;
+ return true;
+ }
+
+ return false;
+}
+
/* Delete each ofproto port on 'br' that doesn't have a corresponding "struct
* iface".
*
@@ -1039,7 +1079,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 +1111,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 +1178,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 +1190,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 +1240,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 +1270,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 +1279,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 +1297,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 +1987,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 +2093,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 +2688,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 +2780,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);
}
--
1.7.9.6
More information about the dev
mailing list