[ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 01:27:25PM -0800, Jarno Rajahalme wrote:
> 
> On Dec 10, 2013, at 11:20 PM, Ben Pfaff <blp at 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 <jrajahalme at 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++) {