Assuming this makes it in, will be my 500th commit. Ethan
On Sun, Apr 22, 2012 at 14:22, Ethan Jackson <et...@nicira.com> wrote: > The existing bridge_reconfigure() implementation is sub-optimal. > When adding lots of new ports, on every pass through the run loop > it allocates a bunch of "struct iface"s and "struct port"s, only to > destroy them when out of time. Additionally, when there are errors > adding or deleting ports, it can fail to converge. Instead it will > attempt and fail to add the same set of ports forever. > > This patch rewrites bridge_reconfigure() using a new strategy. > Whenever the database changes, some initial book keeping is done, > and a list of future work is compiled. The bridge begins whittling > down this list, and stops processing database changes until > finished. > > Bug #10902. > Signed-off-by: Ethan Jackson <et...@nicira.com> > --- > vswitchd/bridge.c | 772 > ++++++++++++++++++++++++++++++----------------------- > 1 file changed, 439 insertions(+), 333 deletions(-) > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 15f6cb7..d7a9e88 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -60,6 +60,19 @@ VLOG_DEFINE_THIS_MODULE(bridge); > > COVERAGE_DEFINE(bridge_reconfigure); > > +/* Configuration of an uninstantiated iface. */ > +struct if_cfg { > + struct list list_node; /* Node in bridge's if_cfg_queue. */ > + const struct ovsrec_interface *cfg; /* Interface record. */ > + const struct ovsrec_port *parent; /* Parent port record. */ > +}; > + > +/* OpenFlow port slated for removal from ofproto. */ > +struct ofpp_inmate { > + struct list list_node; /* Node in bridge's ofpp_death_row. */ > + uint16_t ofp_port; /* Port to be deleted. */ > +}; > + > struct iface { > /* These members are always valid. */ > struct list port_elem; /* Element in struct port's "ifaces" list. */ > @@ -113,6 +126,9 @@ struct bridge { > struct hmap ifaces; /* "struct iface"s indexed by ofp_port. */ > struct hmap iface_by_name; /* "struct iface"s indexed by name. */ > > + struct list ofpp_death_row; /* "struct ofpp_inmate"s slated for removal. > */ > + struct list if_cfg_queue; /* "struct if_cfg"s slated for creation. */ > + > /* Port mirroring. */ > struct hmap mirrors; /* "struct mirror" indexed by UUID. */ > > @@ -151,11 +167,11 @@ static long long int db_limiter = LLONG_MIN; > * 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 bool reconfiguring = false; > > static void add_del_bridges(const struct ovsrec_open_vswitch *); > -static void bridge_del_ofprotos(void); > -static bool bridge_add_ofprotos(struct bridge *); > +static void bridge_update_ofprotos(void); > +static void bridge_populate_ofpp_death_row(struct bridge *); > static void bridge_create(const struct ovsrec_bridge *); > static void bridge_destroy(struct bridge *); > static struct bridge *bridge_lookup(const char *name); > @@ -165,10 +181,6 @@ 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 *, > - 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 *); > @@ -187,6 +199,9 @@ static void bridge_pick_local_hw_addr(struct bridge *, > static uint64_t bridge_pick_datapath_id(struct bridge *, > const uint8_t bridge_ea[ETH_ADDR_LEN], > struct iface *hw_addr_iface); > +static void bridge_queue_if_cfg(struct bridge *, > + const struct ovsrec_interface *, > + const struct ovsrec_port *); > static uint64_t dpid_from_hash(const void *, size_t nbytes); > static bool bridge_has_bond_fake_iface(const struct bridge *, > const char *name); > @@ -195,7 +210,6 @@ static bool port_is_bond_fake_iface(const struct port *); > static unixctl_cb_func qos_unixctl_show; > > static struct port *port_create(struct bridge *, const struct ovsrec_port *); > -static void port_add_ifaces(struct port *); > static void port_del_ifaces(struct port *); > static void port_destroy(struct port *); > static struct port *port_lookup(const struct bridge *, const char *name); > @@ -216,6 +230,9 @@ static void mirror_refresh_stats(struct mirror *); > static void iface_configure_lacp(struct iface *, struct lacp_slave_settings > *); > static struct iface *iface_create(struct port *port, > const struct ovsrec_interface *if_cfg); > +static struct iface *iface_from_if_cfg(struct bridge *, struct if_cfg *); > +static void iface_refresh_type(struct iface *); > +static void iface_init(struct iface *); > static void iface_destroy(struct iface *); > static struct iface *iface_lookup(const struct bridge *, const char *name); > static struct iface *iface_find(const char *name); > @@ -407,23 +424,18 @@ 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; > - size_t n_managers; > + struct bridge *br; > > COVERAGE_INC(bridge_reconfigure); > > - port_action_timer = LLONG_MAX; > - need_reconfigure = false; > + reconfiguring = true; > > - /* Create and destroy "struct bridge"s, "struct port"s, and "struct > - * iface"s according to 'ovs_cfg', with only very minimal configuration > - * otherwise. > + /* Destroy "struct bridge"s, "struct port"s, and "struct iface"s > according > + * to 'ovs_cfg' while update the "if_cfg_queue", with only very minimal > + * configuration otherwise. > * > - * This is mostly an update to bridge data structures. Very little is > - * pushed down to ofproto or lower layers. */ > + * This is mostly an update to bridge data structures. Nothing is pushed > + * down to ofproto or lower layers. */ > add_del_bridges(ovs_cfg); > splinter_vlans = collect_splinter_vlans(ovs_cfg); > HMAP_FOR_EACH (br, node, &all_bridges) { > @@ -431,39 +443,93 @@ bridge_reconfigure(const struct ovsrec_open_vswitch > *ovs_cfg) > } > free(splinter_vlans); > > - /* Delete all datapaths and datapath ports that are no longer configured. > - * > - * The kernel will reject any attempt to add a given port to a datapath > if > - * that port already belongs to a different datapath, so we must do all > - * port deletions before any port additions. A datapath always has a > - * "local port" so we must delete not-configured datapaths too. */ > - bridge_del_ofprotos(); > + /* Delete datapaths that are no longer configured, and create ones which > + * don't exist but should. */ > + bridge_update_ofprotos(); > + > + /* Make sure each "struct iface" has a correct ofp_port in its ofproto. > */ > HMAP_FOR_EACH (br, node, &all_bridges) { > - if (br->ofproto) { > - bridge_del_ofproto_ports(br, &port_action_timer); > + bridge_refresh_ofp_port(br); > + } > + > + /* Schedule ofp_ports which don't belong for deletion. */ > + HMAP_FOR_EACH (br, node, &all_bridges) { > + bridge_populate_ofpp_death_row(br); > + } > + > + /* Clear database records for "if_cfg"s which haven't been instantiated. > */ > + HMAP_FOR_EACH (br, node, &all_bridges) { > + struct if_cfg *if_cfg; > + > + LIST_FOR_EACH (if_cfg, list_node, &br->if_cfg_queue) { > + iface_clear_db_record(if_cfg->cfg); > } > } > +} > > - /* Create datapaths and datapath ports that are missing. > - * > - * After this is done, we have our final set of bridges, ports, and > - * interfaces. Every "struct bridge" has an ofproto, every "struct port" > - * has at least one iface, every "struct iface" has a valid ofp_port and > - * netdev. */ > - HMAP_FOR_EACH_SAFE (br, next, node, &all_bridges) { > - if (!br->ofproto) { > - if (bridge_add_ofprotos(br)) { > - bridge_del_ofproto_ports(br, &port_action_timer); > - } else { > - bridge_destroy(br); > +static bool > +bridge_reconfigure_ofp(void) > +{ > + long long int deadline; > + struct bridge *br; > + bool done = true; > + > + /* Do any work needed to complete configuration. Don't consider > ourselves > + * done unless no work was needed in this iteration. This guarantees > that > + * ofproto_run() has a chance to respond to newly added ports. */ > + > + time_refresh(); > + deadline = time_msec() + OFP_PORT_ACTION_WINDOW; > + > + /* The kernel will reject any attempt to add a given port to a datapath > if > + * that port already belongs to a different datapath, so we must do all > + * port deletions before any port additions. */ > + HMAP_FOR_EACH (br, node, &all_bridges) { > + struct ofpp_inmate *inmate, *next; > + > + LIST_FOR_EACH_SAFE (inmate, next, list_node, &br->ofpp_death_row) { > + ofproto_port_del(br->ofproto, inmate->ofp_port); > + list_remove(&inmate->list_node); > + free(inmate); > + done = false; > + > + time_refresh(); > + if (time_msec() >= deadline) { > + return done; > } > } > } > + > HMAP_FOR_EACH (br, node, &all_bridges) { > - bridge_refresh_ofp_port(br); > - bridge_add_ofproto_ports(br, &port_action_timer); > + struct if_cfg *if_cfg, *next; > + > + LIST_FOR_EACH_SAFE (if_cfg, next, list_node, &br->if_cfg_queue) { > + iface_init(iface_from_if_cfg(br, if_cfg)); > + list_remove(&if_cfg->list_node); > + free(if_cfg); > + done = false; > + > + time_refresh(); > + if (time_msec() >= deadline) { > + return done; > + } > + } > } > > + return done; > +} > + > +static bool > +bridge_reconfigure_continue(const struct ovsrec_open_vswitch *ovs_cfg) > +{ > + struct sockaddr_in *managers; > + int sflow_bridge_number; > + size_t n_managers; > + struct bridge *br; > + bool done; > + > + done = bridge_reconfigure_ofp(); > + > /* Complete the configuration. */ > sflow_bridge_number = 0; > collect_in_band_managers(ovs_cfg, &managers, &n_managers); > @@ -497,22 +563,27 @@ bridge_reconfigure(const struct ovsrec_open_vswitch > *ovs_cfg) > } > free(managers); > > - if (!need_reconfigure) { > + if (done) { > /* ovs-vswitchd has completed initialization, so allow the process > that > * forked us to exit successfully. */ > daemonize_complete(); > + reconfiguring = false; > } > + > + return done; > } > > -/* Iterate over all ofprotos and delete any of them that do not have a > - * configured bridge or that are the wrong type. */ > +/* Delete ofprotos which aren't configured or have the wrong type. Create > + * ofprotos which don't exist but need to. */ > static void > -bridge_del_ofprotos(void) > +bridge_update_ofprotos(void) > { > + struct bridge *br, *next; > struct sset names; > struct sset types; > const char *type; > > + /* Delete ofprotos with no bridge or with the wrong type. */ > sset_init(&names); > sset_init(&types); > ofproto_enumerate_types(&types); > @@ -521,7 +592,7 @@ bridge_del_ofprotos(void) > > ofproto_enumerate_names(type, &names); > SSET_FOR_EACH (name, &names) { > - struct bridge *br = bridge_lookup(name); > + br = bridge_lookup(name); > if (!br || strcmp(type, br->type)) { > ofproto_delete(name, type); > } > @@ -529,17 +600,39 @@ bridge_del_ofprotos(void) > } > sset_destroy(&names); > sset_destroy(&types); > -} > > -static bool > -bridge_add_ofprotos(struct bridge *br) > -{ > - int error = ofproto_create(br->name, br->type, &br->ofproto); > - if (error) { > - VLOG_ERR("failed to create bridge %s: %s", br->name, > strerror(error)); > - return false; > + /* Suppose we have an ofproto named "a" with a port "b" and we need to > + * create an ofproto named "b". Since port "b" exists on "a", creation > of > + * ofproto "b" will fail because it won't be able to make an internal > port. > + * To avoid this problem, we remove all ports from datapaths which have > the > + * same name as a different ofproto. This situation is unlikely to occur > + * frequently, so we don't rate limit it for simplicity. */ > + HMAP_FOR_EACH (br, node, &all_bridges) { > + if (br->ofproto) { > + struct ofproto_port ofproto_port; > + struct ofproto_port_dump dump; > + > + OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) { > + struct bridge *ofp_br = bridge_lookup(ofproto_port.name); > + > + if (ofp_br && ofp_br != br) { > + ofproto_port_del(br->ofproto, ofproto_port.ofp_port); > + } > + } > + } > + } > + > + /* Add ofprotos for bridges which don't have one yet. */ > + HMAP_FOR_EACH_SAFE (br, next, node, &all_bridges) { > + if (!br->ofproto) { > + int error = ofproto_create(br->name, br->type, &br->ofproto); > + if (error) { > + VLOG_ERR("failed to create bridge %s: %s", br->name, > + strerror(error)); > + bridge_destroy(br); > + } > + } > } > - return true; > } > > static void > @@ -1050,80 +1143,6 @@ 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". > - * > - * The kernel will reject any attempt to add a given port to a datapath if > that > - * 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, > - long long int *port_action_timer) > -{ > - struct ofproto_port_dump dump; > - struct ofproto_port ofproto_port; > - > - OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) { > - const char *name = ofproto_port.name; > - struct iface *iface; > - const char *type; > - int error; > - > - /* Ignore the local port. We can't change it anyhow. */ > - if (!strcmp(name, br->name)) { > - continue; > - } > - > - /* Get the type that 'ofproto_port' should have (ordinarily the > - * type of its corresponding iface) or NULL if it should be > - * deleted. */ > - iface = iface_lookup(br, name); > - type = (iface ? iface->type > - : bridge_has_bond_fake_iface(br, name) ? "internal" > - : NULL); > - > - /* If it's the wrong type then delete the ofproto port. */ > - if (type > - && !strcmp(ofproto_port.type, type) > - && (!iface || !iface->netdev > - || !strcmp(netdev_get_type(iface->netdev), type))) { > - continue; > - } > - > - 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); > - iface->netdev = NULL; > - } > - } > -} > - > static void > iface_set_ofp_port(struct iface *iface, int ofp_port) > { > @@ -1136,13 +1155,45 @@ iface_set_ofp_port(struct iface *iface, int ofp_port) > } > > static void > +bridge_ofproto_port_del(struct bridge *br, struct ofproto_port ofproto_port) > +{ > + int error = ofproto_port_del(br->ofproto, ofproto_port.ofp_port); > + if (error) { > + VLOG_WARN("bridge %s: failed to remove %s interface (%s)", > + br->name, ofproto_port.name, strerror(error)); > + } else { > + VLOG_INFO("bridge %s: removed interface %s (%d)", br->name, > + ofproto_port.name, ofproto_port.ofp_port); > + } > +} > + > +/* Update bridges "if_cfg"s, "struct port"s, and "struct iface"s to be > + * consistent with the ofp_ports in "br->ofproto". */ > +static void > bridge_refresh_ofp_port(struct bridge *br) > { > struct ofproto_port_dump dump; > struct ofproto_port ofproto_port; > - struct port *port; > + struct port *port, *port_next; > + struct if_cfg *if_cfg, *if_cfg_next; > + > + /* Find any "if_cfg"s which already exist in the datapath and promote > them > + * to full fledged "struct iface"s. */ > + LIST_FOR_EACH_SAFE (if_cfg, if_cfg_next, list_node, &br->if_cfg_queue) { > + if (!ofproto_port_query_by_name(br->ofproto, if_cfg->cfg->name, > + &ofproto_port)) { > + struct iface *iface = iface_from_if_cfg(br, if_cfg); > + > + iface_set_ofp_port(iface, ofproto_port.ofp_port); > + iface_init(iface); > > - /* Clear all the "ofp_port"es. */ > + ofproto_port_destroy(&ofproto_port); > + list_remove(&if_cfg->list_node); > + free(if_cfg); > + } > + } > + > + /* Clear each "struct iface"s ofp_port so we can get it's correct value. > */ > hmap_clear(&br->ifaces); > HMAP_FOR_EACH (port, hmap_node, &br->ports) { > struct iface *iface; > @@ -1162,149 +1213,163 @@ bridge_refresh_ofp_port(struct bridge *br) > } else if (iface_from_ofp_port(br, ofproto_port.ofp_port)) { > VLOG_WARN("bridge %s: interface %"PRIu16" reported twice", > br->name, ofproto_port.ofp_port); > - } else { > + } else if (!strcmp(ofproto_port.type, iface->type)) { > iface_set_ofp_port(iface, ofproto_port.ofp_port); > + } else { > + /* Port has incorrect type so delete it later. */ > + } > + } else if (bridge_has_bond_fake_iface(br, ofproto_port.name) > + && strcmp(ofproto_port.type, "internal")) { > + /* Bond fake iface with the wrong type. */ > + bridge_ofproto_port_del(br, ofproto_port); > + } > + } > + > + /* Some ifaces may not have "ofp_port"s in ofproto and therefore don't > + * deserve to have "struct iface"s. Demote these to "if_cfg"s so that > + * later they can be added to ofproto. */ > + HMAP_FOR_EACH_SAFE (port, port_next, hmap_node, &br->ports) { > + struct iface *iface, *iface_next; > + > + LIST_FOR_EACH_SAFE (iface, iface_next, port_elem, &port->ifaces) { > + if (iface->ofp_port < 0) { > + bridge_queue_if_cfg(br, iface->cfg, port->cfg); > + iface_destroy(iface); > } > } > + > + if (list_is_empty(&port->ifaces)) { > + port_destroy(port); > + } > } > } > > -/* Add an ofproto port for any "struct iface" that doesn't have one. > - * 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, > - long long int *port_action_timer) > +bridge_populate_ofpp_death_row(struct bridge *br) > { > - struct port *port, *next_port; > + struct ofproto_port_dump dump; > + struct ofproto_port ofproto_port; > > - HMAP_FOR_EACH_SAFE (port, next_port, hmap_node, &br->ports) { > - struct iface *iface, *next_iface; > - struct ofproto_port ofproto_port; > + OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) { > + if (!iface_lookup(br, ofproto_port.name)) { > + struct ofpp_inmate *inmate = xmalloc(sizeof *inmate); > + inmate->ofp_port = ofproto_port.ofp_port; > + list_push_front(&br->ofpp_death_row, &inmate->list_node); > + } > + } > +} > > - LIST_FOR_EACH_SAFE (iface, next_iface, port_elem, &port->ifaces) { > - int error; > +/* Initialize 'iface' with a netdev and ofp_port if necessary. */ > +static void > +iface_init(struct iface *iface) > +{ > + struct port *port = iface->port; > + struct bridge *br = port->bridge; > + struct ofproto_port ofproto_port; > + int error; > > - if (iface->ofp_port < 0 && !may_port_action(port_action_timer)) { > - iface_clear_db_record(iface->cfg); > - iface_destroy(iface); > - continue; > - } > + assert(!iface->netdev); > + error = netdev_open(iface->name, iface->type, &iface->netdev); > + if (error) { > + VLOG_WARN("could not open network device %s (%s)", iface->name, > + strerror(error)); > + } > > - /* Open the netdev. */ > - if (!iface->netdev) { > - error = netdev_open(iface->name, iface->type, > &iface->netdev); > - if (error) { > - VLOG_WARN("could not open network device %s (%s)", > - iface->name, strerror(error)); > - } > + if (iface->netdev > + && port->cfg->vlan_mode > + && !strcmp(port->cfg->vlan_mode, "splinter")) { > + netdev_turn_flags_on(iface->netdev, NETDEV_UP, true); > + } > > - if (iface->netdev > - && port->cfg->vlan_mode > - && !strcmp(port->cfg->vlan_mode, "splinter")) { > - netdev_turn_flags_on(iface->netdev, NETDEV_UP, true); > - } > - } else { > - error = 0; > - } > + /* Configure the netdev. */ > + if (iface->netdev) { > + struct shash args; > > - /* Configure the netdev. */ > - if (iface->netdev) { > - struct shash args; > - > - shash_init(&args); > - shash_from_ovs_idl_map(iface->cfg->key_options, > - iface->cfg->value_options, > - iface->cfg->n_options, &args); > - error = netdev_set_config(iface->netdev, &args); > - shash_destroy(&args); > - > - if (error) { > - VLOG_WARN("could not configure network device %s (%s)", > - iface->name, strerror(error)); > - netdev_close(iface->netdev); > - iface->netdev = NULL; > - } > - } > + shash_init(&args); > + shash_from_ovs_idl_map(iface->cfg->key_options, > + iface->cfg->value_options, > + iface->cfg->n_options, &args); > + error = netdev_set_config(iface->netdev, &args); > + shash_destroy(&args); > > - /* Add the port, if necessary. */ > - if (iface->netdev && iface->ofp_port < 0) { > - uint16_t ofp_port; > - int error; > - > - 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); > - iface->netdev = NULL; > - } > - } > + if (error) { > + VLOG_WARN("could not configure network device %s (%s)", > + iface->name, strerror(error)); > + netdev_close(iface->netdev); > + iface->netdev = NULL; > + } > + } > > - /* Populate stats columns in new Interface rows. */ > - if (iface->netdev && iface->need_refresh) { > - iface_refresh_stats(iface); > - iface_refresh_status(iface); > - iface->need_refresh = false; > - } > + /* Add the port, if necessary. */ > + if (iface->netdev && iface->ofp_port < 0) { > + uint16_t ofp_port; > + int error; > > - /* Delete the iface if we failed. */ > - if (iface->netdev && iface->ofp_port >= 0) { > - VLOG_DBG("bridge %s: interface %s is on port %d", > - br->name, iface->name, iface->ofp_port); > - } else { > - if (iface->netdev) { > - VLOG_ERR("bridge %s: missing %s interface, dropping", > - br->name, iface->name); > - } else { > - /* We already reported a related error, don't bother > - * duplicating it. */ > - } > - 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); > - } > - iface_clear_db_record(iface->cfg); > - iface_destroy(iface); > - } > + 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); > + iface->netdev = NULL; > } > + } > > - if (list_is_empty(&port->ifaces)) { > - if (!need_reconfigure) { > - VLOG_WARN("%s port has no interfaces, dropping", port->name); > - } > - port_destroy(port); > - continue; > + /* Populate stats columns in new Interface rows. */ > + if (iface->netdev && iface->need_refresh) { > + iface_refresh_stats(iface); > + iface_refresh_status(iface); > + iface->need_refresh = false; > + } > + > + /* Delete the iface if we failed. */ > + if (iface->netdev && iface->ofp_port >= 0) { > + VLOG_DBG("bridge %s: interface %s is on port %d", > + br->name, iface->name, iface->ofp_port); > + } else { > + if (iface->netdev) { > + VLOG_ERR("bridge %s: missing %s interface, dropping", > + br->name, iface->name); > + } else { > + /* We already reported a related error, don't bother > + * duplicating it. */ > } > + 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); > + bridge_ofproto_port_del(br, ofproto_port); > + ofproto_port_destroy(&ofproto_port); > + } > + iface_clear_db_record(iface->cfg); > + iface_destroy(iface); > + } > > - /* Add bond fake iface if necessary. */ > - if (port_is_bond_fake_iface(port)) { > - if (ofproto_port_query_by_name(br->ofproto, port->name, > - &ofproto_port)) { > - struct netdev *netdev; > - int error; > - > - 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 { > - VLOG_WARN("could not open network device %s (%s)", > - port->name, strerror(error)); > - } > + if (list_is_empty(&port->ifaces)) { > + port_destroy(port); > + return; > + } > + > + /* Add bond fake iface if necessary. */ > + if (port_is_bond_fake_iface(port)) { > + if (ofproto_port_query_by_name(br->ofproto, port->name, > + &ofproto_port)) { > + struct netdev *netdev; > + int error; > + > + error = netdev_open(port->name, "internal", &netdev); > + if (!error) { > + ofproto_port_add(br->ofproto, netdev, NULL); > + netdev_close(netdev); > } else { > - /* Already exists, nothing to do. */ > - ofproto_port_destroy(&ofproto_port); > + VLOG_WARN("could not open network device %s (%s)", > + port->name, strerror(error)); > } > + } else { > + /* Already exists, nothing to do. */ > + ofproto_port_destroy(&ofproto_port); > } > } > } > @@ -1929,26 +1994,30 @@ bridge_run_fast(void) > void > bridge_run(void) > { > + static const struct ovsrec_open_vswitch null_cfg; > const struct ovsrec_open_vswitch *cfg; > > bool vlan_splinters_changed; > struct bridge *br; > > /* (Re)configure if necessary. */ > - ovsdb_idl_run(idl); > - if (ovsdb_idl_is_lock_contended(idl)) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - struct bridge *br, *next_br; > + if (!reconfiguring) { > + ovsdb_idl_run(idl); > > - VLOG_ERR_RL(&rl, "another ovs-vswitchd process is running, " > - "disabling this process until it goes away"); > + if (ovsdb_idl_is_lock_contended(idl)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + struct bridge *br, *next_br; > > - HMAP_FOR_EACH_SAFE (br, next_br, node, &all_bridges) { > - bridge_destroy(br); > + VLOG_ERR_RL(&rl, "another ovs-vswitchd process is running, " > + "disabling this process until it goes away"); > + > + HMAP_FOR_EACH_SAFE (br, next_br, node, &all_bridges) { > + bridge_destroy(br); > + } > + return; > + } else if (!ovsdb_idl_has_lock(idl)) { > + return; > } > - return; > - } else if (!ovsdb_idl_has_lock(idl)) { > - return; > } > cfg = ovsrec_open_vswitch_first(idl); > > @@ -1982,26 +2051,37 @@ bridge_run(void) > } > } > > - if (need_reconfigure || ovsdb_idl_get_seqno(idl) != idl_seqno > - || vlan_splinters_changed) { > + if (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); > > bridge_reconfigure(cfg); > > - ovsrec_open_vswitch_set_cur_cfg(cfg, cfg->next_cfg); > ovsdb_idl_txn_commit(txn); > ovsdb_idl_txn_destroy(txn); /* XXX */ > } else { > /* We still need to reconfigure to avoid dangling pointers to > * now-destroyed ovsrec structures inside bridge data. */ > - static const struct ovsrec_open_vswitch null_cfg; > - > bridge_reconfigure(&null_cfg); > } > } > > + if (reconfiguring) { > + struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl); > + > + if (cfg) { > + if (bridge_reconfigure_continue(cfg)) { > + ovsrec_open_vswitch_set_cur_cfg(cfg, cfg->next_cfg); > + } > + } else { > + bridge_reconfigure_continue(&null_cfg); > + } > + > + ovsdb_idl_txn_commit(txn); > + ovsdb_idl_txn_destroy(txn); /* XXX */ > + } > + > /* Refresh system and interface stats if necessary. */ > if (time_msec() >= stats_timer) { > if (cfg) { > @@ -2089,7 +2169,7 @@ bridge_wait(void) > { > ovsdb_idl_wait(idl); > > - if (need_reconfigure) { > + if (reconfiguring) { > poll_immediate_wake(); > } > > @@ -2223,6 +2303,9 @@ bridge_create(const struct ovsrec_bridge *br_cfg) > hmap_init(&br->iface_by_name); > hmap_init(&br->mirrors); > > + list_init(&br->if_cfg_queue); > + list_init(&br->ofpp_death_row); > + > hmap_insert(&all_bridges, &br->node, hash_string(br->name, 0)); > } > > @@ -2232,6 +2315,8 @@ bridge_destroy(struct bridge *br) > if (br) { > struct mirror *mirror, *next_mirror; > struct port *port, *next_port; > + struct if_cfg *if_cfg, *next_if_cfg; > + struct ofpp_inmate *inmate, *next_inmate; > > HMAP_FOR_EACH_SAFE (port, next_port, hmap_node, &br->ports) { > port_destroy(port); > @@ -2239,6 +2324,19 @@ bridge_destroy(struct bridge *br) > HMAP_FOR_EACH_SAFE (mirror, next_mirror, hmap_node, &br->mirrors) { > mirror_destroy(mirror); > } > + > + LIST_FOR_EACH_SAFE (inmate, next_inmate, list_node, > + &br->ofpp_death_row) { > + list_remove(&inmate->list_node); > + free(inmate); > + } > + > + LIST_FOR_EACH_SAFE (if_cfg, next_if_cfg, list_node, > + &br->if_cfg_queue) { > + list_remove(&if_cfg->list_node); > + free(if_cfg); > + } > + > hmap_remove(&all_bridges, &br->node); > ofproto_destroy(br->ofproto); > hmap_destroy(&br->ifaces); > @@ -2330,17 +2428,54 @@ bridge_get_controllers(const struct bridge *br, > return n_controllers; > } > > -/* Adds and deletes "struct port"s and "struct iface"s under 'br' to match > - * those configured in 'br->cfg'. */ > +static void > +bridge_queue_if_cfg(struct bridge *br, > + const struct ovsrec_interface *cfg, > + const struct ovsrec_port *parent) > +{ > + struct if_cfg *if_cfg = xmalloc(sizeof *if_cfg); > + > + if_cfg->cfg = cfg; > + if_cfg->parent = parent; > + list_insert(&br->if_cfg_queue, &if_cfg->list_node); > +} > + > +static void > +bridge_update_ifaces(struct bridge *br, struct shash *new_ports) > +{ > + struct shash_node *port_node; > + > + SHASH_FOR_EACH (port_node, new_ports) { > + const struct ovsrec_port *port = port_node->data; > + size_t i; > + > + for (i = 0; i < port->n_interfaces; i++) { > + const struct ovsrec_interface *cfg = port->interfaces[i]; > + struct iface *iface = iface_lookup(br, cfg->name); > + > + if (iface) { > + iface->cfg = cfg; > + iface_refresh_type(iface); > + } else { > + bridge_queue_if_cfg(br, cfg, port); > + } > + } > + } > +} > + > +/* Deletes "struct port"s and "struct iface"s under 'br' which aren't > + * consistent with 'br->cfg'. Updates 'br->if_cfg_queue' with interfaces > which > + * 'br' needs to complete it's configuration. */ > static void > bridge_add_del_ports(struct bridge *br, > const unsigned long int *splinter_vlans) > { > struct port *port, *next; > - struct shash_node *node; > struct shash new_ports; > size_t i; > > + assert(list_is_empty(&br->if_cfg_queue)); > + > /* Collect new ports. */ > shash_init(&new_ports); > for (i = 0; i < br->cfg->n_ports; i++) { > @@ -2382,21 +2517,8 @@ bridge_add_del_ports(struct bridge *br, > } > } > > - /* Create new ports. > - * Add new interfaces to existing ports. */ > - SHASH_FOR_EACH (node, &new_ports) { > - struct port *port = port_lookup(br, node->name); > - if (!port) { > - struct ovsrec_port *cfg = node->data; > - port = port_create(br, cfg); > - } > - port_add_ifaces(port); > - if (list_is_empty(&port->ifaces)) { > - VLOG_WARN("bridge %s: port %s has no interfaces, dropping", > - br->name, port->name); > - port_destroy(port); > - } > - } > + bridge_update_ifaces(br, &new_ports); > + > shash_destroy(&new_ports); > } > > @@ -2718,51 +2840,6 @@ port_del_ifaces(struct port *port) > sset_destroy(&new_ifaces); > } > > -/* Adds new interfaces to 'port' and updates 'type' and 'cfg' members of > - * existing ones. */ > -static void > -port_add_ifaces(struct port *port) > -{ > - struct shash new_ifaces; > - struct shash_node *node; > - size_t i; > - > - /* Collect new ifaces. */ > - shash_init(&new_ifaces); > - for (i = 0; i < port->cfg->n_interfaces; i++) { > - const struct ovsrec_interface *cfg = port->cfg->interfaces[i]; > - if (strcmp(cfg->type, "null") > - && !shash_add_once(&new_ifaces, cfg->name, cfg)) { > - VLOG_WARN("port %s: %s specified twice as port interface", > - port->name, cfg->name); > - iface_clear_db_record(cfg); > - } > - } > - > - /* Create new interfaces. > - * Update interface types and 'cfg' members. */ > - SHASH_FOR_EACH (node, &new_ifaces) { > - const struct ovsrec_interface *cfg = node->data; > - const char *iface_name = node->name; > - struct iface *iface; > - > - iface = iface_lookup(port->bridge, iface_name); > - if (!iface) { > - iface = iface_create(port, cfg); > - } else { > - iface->cfg = cfg; > - } > - > - /* Determine interface type. The local port always has type > - * "internal". Other ports take their type from the database and > - * default to "system" if none is specified. */ > - iface->type = (!strcmp(iface_name, port->bridge->name) ? "internal" > - : cfg->type[0] ? cfg->type > - : "system"); > - } > - shash_destroy(&new_ifaces); > -} > - > static void > port_destroy(struct port *port) > { > @@ -3016,6 +3093,35 @@ iface_create(struct port *port, const struct > ovsrec_interface *if_cfg) > } > > static void > +iface_refresh_type(struct iface *iface) > +{ > + /* Determine interface type. The local port always has type > + * "internal". Other ports take their type from the database and > + * default to "system" if none is specified. */ > + iface->type = (!strcmp(iface->name, iface->port->bridge->name) ? > "internal" > + : iface->cfg->type[0] ? iface->cfg->type > + : "system"); > +} > + > +static struct iface * > +iface_from_if_cfg(struct bridge *br, struct if_cfg *if_cfg) > +{ > + struct iface *iface; > + struct port *port; > + > + port = port_lookup(br, if_cfg->parent->name); > + if (!port) { > + port = port_create(br, if_cfg->parent); > + } > + > + assert(!iface_lookup(br, if_cfg->cfg->name)); > + iface = iface_create(port, if_cfg->cfg); > + iface_refresh_type(iface); > + > + return iface; > +} > + > +static void > iface_destroy(struct iface *iface) > { > if (iface) { > -- > 1.7.9.6 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev