On Wed, Dec 11, 2013 at 01:27:25PM -0800, Jarno Rajahalme wrote:
>
> On Dec 10, 2013, at 11:20 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > This greatly simplifies the reconfiguration code, making it much easier
> > to understand and modify. The old multi-pass configuration had the
> > property that it didn't delay block packet processing as much, but that's
> > not much of a worry anymore now that latency critical activities have
> > been moved outside the main thread.
> >
>
> You?ll find two minor comments below, but otherwise seems good to
> me. I have no history with this code, though, so with that caveat:
>
> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
This commit is really unpolished because I posted it when I was really
tired. Sorry about that.
I folded in the following, which helps I think. There is probably
still room for better function naming, at the very least.
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index fcf7efb..ed16860 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -178,8 +178,16 @@ static unixctl_cb_func bridge_unixctl_dump_flows;
static unixctl_cb_func bridge_unixctl_reconnect;
static size_t bridge_get_controllers(const struct bridge *br,
struct ovsrec_controller ***controllersp);
+static void bridge_collect_wanted_ports(struct bridge *,
+ const unsigned long *splinter_vlans,
+ struct shash *wanted_ports);
+static void bridge_delete_ofprotos(void);
+static void bridge_delete_or_reconfigure_ports(struct bridge *);
static void bridge_del_ports(struct bridge *,
const struct shash *wanted_ports);
+static void bridge_add_ports(struct bridge *,
+ const struct shash *wanted_ports);
+
static void bridge_configure_flow_miss_model(const char *opt);
static void bridge_configure_datapath_id(struct bridge *);
static void bridge_configure_netflow(struct bridge *);
@@ -466,14 +474,6 @@ collect_in_band_managers(const struct ovsrec_open_vswitch
*ovs_cfg,
*n_managersp = n_managers;
}
-static void bridge_collect_wanted_ports(struct bridge *,
- const unsigned long *splinter_vlans,
- struct shash *wanted_ports);
-static void bridge_delete_ofprotos(void);
-static void bridge_delete_ports(struct bridge *);
-static void bridge_add_ports(struct bridge *,
- const struct shash *wanted_ports);
-
static void
bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
{
@@ -508,17 +508,30 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
*ovs_cfg)
}
free(splinter_vlans);
- /* Delete datapaths and ports that are no longer configured. Attempt to
- * reconfigure existing ports to their desired configurations, or delete
- * them if not possible. */
+ /* Start pushing configuration changes down to the ofproto layer:
+ *
+ * - Delete ofprotos that are no longer configured.
+ *
+ * - Delete ports that are no longer configured.
+ *
+ * - Reconfigure existing ports to their desired configurations, or
+ * delete them if not possible.
+ *
+ * We have to do all the deletions before we can do any additions, because
+ * the ports to be added might require resources that will be freed up by
+ * deletions (they might especially overlap in name). */
bridge_delete_ofprotos();
HMAP_FOR_EACH (br, node, &all_bridges) {
if (br->ofproto) {
- bridge_delete_ports(br);
+ bridge_delete_or_reconfigure_ports(br);
}
}
- /* Add new ofprotos and ports. */
+ /* Finish pushing configuration changes to the ofproto layer:
+ *
+ * - Create ofprotos that are missing.
+ *
+ * - Add ports that are missing. */
HMAP_FOR_EACH_SAFE (br, next, node, &all_bridges) {
if (!br->ofproto) {
int error;
@@ -623,11 +636,14 @@ add_ofp_port(ofp_port_t port, ofp_port_t *ports, size_t
*n, size_t *allocated)
}
static void
-bridge_delete_ports(struct bridge *br)
+bridge_delete_or_reconfigure_ports(struct bridge *br)
{
struct ofproto_port ofproto_port;
struct ofproto_port_dump dump;
+ /* List of "ofp_port"s to delete. We make a list instead of deleting them
+ * right away because ofproto implementations aren't necessarily able to
+ * iterate through a changing list of ports in an entirely robust way. */
ofp_port_t *del;
size_t n, allocated;
size_t i;
@@ -654,9 +670,12 @@ bridge_delete_ports(struct bridge *br)
if (strcmp(ofproto_port.type, iface->type)
|| netdev_set_config(iface->netdev, &iface->cfg->options)) {
+ /* The interface is the wrong type or can't be configured.
+ * Delete it. */
goto delete;
}
+ /* Keep it. */
continue;
delete:
@@ -2630,7 +2649,7 @@ bridge_collect_wanted_ports(struct bridge *br,
struct shash *wanted_ports)
{
size_t i;
-
+
shash_init(wanted_ports);
for (i = 0; i < br->cfg->n_ports; i++) {
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
