From: Ethan Jackson Date: Sat, 21 Apr 2012 02:11:35 +0000 (-0700) Subject: bridge: Refactor bridge_reconfigure(). X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bae7208e91a08e355d24eb6173170f12dd458776;p=openvswitch bridge: Refactor bridge_reconfigure(). The existing bridge_reconfigure() implementation is suboptimal. 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 bookkeeping 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 --- diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 15f6cb7d..077d9ee1 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 hmap_node hmap_node; /* Node in bridge's if_cfg_todo. */ + const struct ovsrec_interface *cfg; /* Interface record. */ + const struct ovsrec_port *parent; /* Parent port record. */ +}; + +/* OpenFlow port slated for removal from ofproto. */ +struct ofpp_garbage { + struct list list_node; /* Node in bridge's ofpp_garbage. */ + 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,10 @@ 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_garbage; /* "struct ofpp_garbage" slated for removal. */ + struct hmap if_cfg_todo; /* "struct if_cfg"s slated for creation. + Indexed on 'cfg->name'. */ + /* Port mirroring. */ struct hmap mirrors; /* "struct mirror" indexed by UUID. */ @@ -151,11 +168,10 @@ 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_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); @@ -214,11 +228,12 @@ static bool mirror_configure(struct mirror *); 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 void iface_create(struct bridge *, struct if_cfg *, int ofp_port); +static void iface_refresh_type(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); +static struct if_cfg *if_cfg_lookup(const struct bridge *, const char *name); static struct iface *iface_from_ofp_port(const struct bridge *, uint16_t ofp_port); static void iface_set_mac(struct iface *); @@ -407,23 +422,19 @@ 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; + assert(!reconfiguring); + 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 +442,79 @@ 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); + } + + /* Clear database records for "if_cfg"s which haven't been instantiated. */ + HMAP_FOR_EACH (br, node, &all_bridges) { + struct if_cfg *if_cfg; + + HMAP_FOR_EACH (if_cfg, hmap_node, &br->if_cfg_todo) { + 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; + + 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_garbage *garbage, *next; + + LIST_FOR_EACH_SAFE (garbage, next, list_node, &br->ofpp_garbage) { + ofproto_port_del(br->ofproto, garbage->ofp_port); + list_remove(&garbage->list_node); + free(garbage); + + time_refresh(); + if (time_msec() >= deadline) { + return false; } } } + 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; + + HMAP_FOR_EACH_SAFE (if_cfg, next, hmap_node, &br->if_cfg_todo) { + iface_create(br, if_cfg, -1); + time_refresh(); + if (time_msec() >= deadline) { + return false; + } + } } + return true; +} + +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; + + assert(reconfiguring); + done = bridge_reconfigure_ofp(); + /* Complete the configuration. */ sflow_bridge_number = 0; collect_in_band_managers(ovs_cfg, &managers, &n_managers); @@ -497,22 +548,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 +577,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 +585,44 @@ 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; + /* Add ofprotos for bridges which don't have one yet. */ + HMAP_FOR_EACH_SAFE (br, next, node, &all_bridges) { + struct bridge *br2; + int error; + + if (br->ofproto) { + continue; + } + + /* Remove ports from any datapath with the same name as 'br'. If we + * don't do this, creating 'br''s ofproto will fail because a port with + * the same name as its local port already exists. */ + HMAP_FOR_EACH (br2, node, &all_bridges) { + struct ofproto_port ofproto_port; + + if (!br2->ofproto) { + continue; + } + + if (!ofproto_port_query_by_name(br2->ofproto, br->name, + &ofproto_port)) { + error = ofproto_port_del(br2->ofproto, ofproto_port.ofp_port); + if (error) { + VLOG_ERR("failed to delete port %s: %s", ofproto_port.name, + strerror(error)); + } + ofproto_port_destroy(&ofproto_port); + } + } + + 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 +1133,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) { @@ -1135,14 +1144,29 @@ iface_set_ofp_port(struct iface *iface, int ofp_port) iface_set_ofport(iface->cfg, 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; - /* Clear all the "ofp_port"es. */ + /* Clear each "struct iface"s ofp_port so we can get its correct value. */ hmap_clear(&br->ifaces); HMAP_FOR_EACH (port, hmap_node, &br->ports) { struct iface *iface; @@ -1152,7 +1176,9 @@ bridge_refresh_ofp_port(struct bridge *br) } } - /* Obtain the correct "ofp_port"s from ofproto. */ + /* Obtain the correct "ofp_port"s from ofproto. Find any if_cfg's which + * already exist in the datapath and promote them to full fledged "struct + * iface"s. Mark ports in the datapath which don't belong as garbage. */ OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) { struct iface *iface = iface_lookup(br, ofproto_port.name); if (iface) { @@ -1162,149 +1188,185 @@ 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 { + struct if_cfg *if_cfg = if_cfg_lookup(br, ofproto_port.name); + + if (if_cfg) { + iface_create(br, if_cfg, ofproto_port.ofp_port); + } 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); + } else { + struct ofpp_garbage *garbage = xmalloc(sizeof *garbage); + garbage->ofp_port = ofproto_port.ofp_port; + list_push_front(&br->ofpp_garbage, &garbage->list_node); } } } + + /* 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. */ +/* Creates a new iface on 'br' based on 'if_cfg'. The new iface has OpenFlow + * port number 'ofp_port'. If ofp_port is negative, an OpenFlow port is + * automatically allocated for the iface. Takes ownership of and + * deallocates 'if_cfg'. */ static void -bridge_add_ofproto_ports(struct bridge *br, - long long int *port_action_timer) +iface_create(struct bridge *br, struct if_cfg *if_cfg, int ofp_port) { - struct port *port, *next_port; + struct ofproto_port ofproto_port; + struct iface *iface; + struct port *port; + int error; - HMAP_FOR_EACH_SAFE (port, next_port, hmap_node, &br->ports) { - struct iface *iface, *next_iface; - struct ofproto_port ofproto_port; + assert(!iface_lookup(br, if_cfg->cfg->name)); - LIST_FOR_EACH_SAFE (iface, next_iface, port_elem, &port->ifaces) { - int error; + port = port_lookup(br, if_cfg->parent->name); + if (!port) { + port = port_create(br, if_cfg->parent); + } - if (iface->ofp_port < 0 && !may_port_action(port_action_timer)) { - iface_clear_db_record(iface->cfg); - iface_destroy(iface); - continue; - } + iface = xzalloc(sizeof *iface); + iface->port = port; + iface->name = xstrdup(if_cfg->cfg->name); + iface->ofp_port = -1; + iface->netdev = NULL; + iface->cfg = if_cfg->cfg; + iface->need_refresh = true; + hmap_insert(&br->iface_by_name, &iface->name_node, + hash_string(iface->name, 0)); + list_push_back(&port->ifaces, &iface->port_elem); + iface_refresh_type(iface); + if (ofp_port >= 0) { + iface_set_ofp_port(iface, ofp_port); + } - /* 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)); - } + hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node); + free(if_cfg); + if_cfg = NULL; - 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; - } + error = netdev_open(iface->name, iface->type, &iface->netdev); + if (error) { + VLOG_WARN("could not open network device %s (%s)", iface->name, + strerror(error)); + } - /* Configure the netdev. */ - if (iface->netdev) { - struct shash args; + if (iface->netdev + && port->cfg->vlan_mode + && !strcmp(port->cfg->vlan_mode, "splinter")) { + netdev_turn_flags_on(iface->netdev, NETDEV_UP, true); + } - 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); + /* Configure the netdev. */ + if (iface->netdev) { + struct shash 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 new_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, &new_ofp_port); + if (!error) { + VLOG_INFO("bridge %s: added interface %s (%d)", br->name, + iface->name, ofp_port); + iface_set_ofp_port(iface, new_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 +1991,31 @@ bridge_run_fast(void) void bridge_run(void) { + static const struct ovsrec_open_vswitch null_cfg; const struct ovsrec_open_vswitch *cfg; + struct ovsdb_idl_txn *reconf_txn = NULL; 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); @@ -1970,38 +2037,51 @@ bridge_run(void) stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert); } - /* If VLAN splinters are in use, then we need to reconfigure if VLAN usage - * has changed. */ - vlan_splinters_changed = false; - if (vlan_splinters_enabled_anywhere) { - HMAP_FOR_EACH (br, node, &all_bridges) { - if (ofproto_has_vlan_usage_changed(br->ofproto)) { - vlan_splinters_changed = true; - break; + if (!reconfiguring) { + /* If VLAN splinters are in use, then we need to reconfigure if VLAN + * usage has changed. */ + vlan_splinters_changed = false; + if (vlan_splinters_enabled_anywhere) { + HMAP_FOR_EACH (br, node, &all_bridges) { + if (ofproto_has_vlan_usage_changed(br->ofproto)) { + vlan_splinters_changed = true; + break; + } + } + } + + if (ovsdb_idl_get_seqno(idl) != idl_seqno || vlan_splinters_changed) { + idl_seqno = ovsdb_idl_get_seqno(idl); + if (cfg) { + reconf_txn = ovsdb_idl_txn_create(idl); + bridge_reconfigure(cfg); + } else { + /* We still need to reconfigure to avoid dangling pointers to + * now-destroyed ovsrec structures inside bridge data. */ + bridge_reconfigure(&null_cfg); } } } - if (need_reconfigure || ovsdb_idl_get_seqno(idl) != idl_seqno - || vlan_splinters_changed) { - idl_seqno = ovsdb_idl_get_seqno(idl); + if (reconfiguring) { 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 */ + if (!reconf_txn) { + reconf_txn = ovsdb_idl_txn_create(idl); + } + if (bridge_reconfigure_continue(cfg)) { + ovsrec_open_vswitch_set_cur_cfg(cfg, cfg->next_cfg); + } } 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); + bridge_reconfigure_continue(&null_cfg); } } + if (reconf_txn) { + ovsdb_idl_txn_commit(reconf_txn); + ovsdb_idl_txn_destroy(reconf_txn); + reconf_txn = NULL; + } + /* 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); + hmap_init(&br->if_cfg_todo); + list_init(&br->ofpp_garbage); + 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_garbage *garbage, *next_garbage; HMAP_FOR_EACH_SAFE (port, next_port, hmap_node, &br->ports) { port_destroy(port); @@ -2239,12 +2324,23 @@ bridge_destroy(struct bridge *br) HMAP_FOR_EACH_SAFE (mirror, next_mirror, hmap_node, &br->mirrors) { mirror_destroy(mirror); } + HMAP_FOR_EACH_SAFE (if_cfg, next_if_cfg, hmap_node, &br->if_cfg_todo) { + hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node); + free(if_cfg); + } + LIST_FOR_EACH_SAFE (garbage, next_garbage, list_node, + &br->ofpp_garbage) { + list_remove(&garbage->list_node); + free(garbage); + } + hmap_remove(&all_bridges, &br->node); ofproto_destroy(br->ofproto); hmap_destroy(&br->ifaces); hmap_destroy(&br->ports); hmap_destroy(&br->iface_by_name); hmap_destroy(&br->mirrors); + hmap_destroy(&br->if_cfg_todo); free(br->name); free(br->type); free(br); @@ -2330,17 +2426,33 @@ 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; + hmap_insert(&br->if_cfg_todo, &if_cfg->hmap_node, + hash_string(if_cfg->cfg->name, 0)); +} + +/* 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 its configuration. */ static void bridge_add_del_ports(struct bridge *br, const unsigned long int *splinter_vlans) { + struct shash_node *port_node; struct port *port, *next; - struct shash_node *node; struct shash new_ports; size_t i; + assert(hmap_is_empty(&br->if_cfg_todo)); + /* Collect new ports. */ shash_init(&new_ports); for (i = 0; i < br->cfg->n_ports; i++) { @@ -2382,21 +2494,23 @@ 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); + 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); + } } } + shash_destroy(&new_ports); } @@ -2718,51 +2832,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) { @@ -2991,28 +3060,15 @@ port_is_synthetic(const struct port *port) /* Interface functions. */ -static struct iface * -iface_create(struct port *port, const struct ovsrec_interface *if_cfg) +static void +iface_refresh_type(struct iface *iface) { - struct bridge *br = port->bridge; - struct iface *iface; - char *name = if_cfg->name; - - iface = xzalloc(sizeof *iface); - iface->port = port; - iface->name = xstrdup(name); - iface->ofp_port = -1; - iface->netdev = NULL; - iface->cfg = if_cfg; - iface->need_refresh = true; - - hmap_insert(&br->iface_by_name, &iface->name_node, hash_string(name, 0)); - - list_push_back(&port->ifaces, &iface->port_elem); - - VLOG_DBG("attached network device %s to port %s", iface->name, port->name); - - return 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 void @@ -3070,6 +3126,21 @@ iface_find(const char *name) return NULL; } +static struct if_cfg * +if_cfg_lookup(const struct bridge *br, const char *name) +{ + struct if_cfg *if_cfg; + + HMAP_FOR_EACH_WITH_HASH (if_cfg, hmap_node, hash_string(name, 0), + &br->if_cfg_todo) { + if (!strcmp(if_cfg->cfg->name, name)) { + return if_cfg; + } + } + + return NULL; +} + static struct iface * iface_from_ofp_port(const struct bridge *br, uint16_t ofp_port) {