Here's an incremental. --- vswitchd/bridge.c | 382 +++++++++++++++++++++++++---------------------------- 1 file changed, 177 insertions(+), 205 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index d7a9e88..d0a0300 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -62,14 +62,14 @@ COVERAGE_DEFINE(bridge_reconfigure); /* Configuration of an uninstantiated iface. */ struct if_cfg { - struct list list_node; /* Node in bridge's if_cfg_queue. */ + 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_inmate { - struct list list_node; /* Node in bridge's ofpp_death_row. */ +struct ofpp_garbage { + struct list list_node; /* Node in bridge's ofpp_garbage. */ uint16_t ofp_port; /* Port to be deleted. */ }; @@ -126,8 +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. */ + 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. */ @@ -171,7 +172,6 @@ static bool reconfiguring = false; static void add_del_bridges(const struct ovsrec_open_vswitch *); 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); @@ -228,14 +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 struct iface *iface_from_if_cfg(struct bridge *, struct if_cfg *); +static void iface_create(struct bridge *, struct if_cfg *, int ofp_port); 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); +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 *); @@ -428,6 +426,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) COVERAGE_INC(bridge_reconfigure); + assert(!reconfiguring); reconfiguring = true; /* Destroy "struct bridge"s, "struct port"s, and "struct iface"s according @@ -452,16 +451,11 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) 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) { + HMAP_FOR_EACH (if_cfg, hmap_node, &br->if_cfg_todo) { iface_clear_db_record(if_cfg->cfg); } } @@ -472,7 +466,6 @@ 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 @@ -481,21 +474,20 @@ bridge_reconfigure_ofp(void) 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. */ + /* 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; + struct ofpp_garbage *garbage, *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; + 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 done; + return false; } } } @@ -503,20 +495,19 @@ bridge_reconfigure_ofp(void) HMAP_FOR_EACH (br, node, &all_bridges) { 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); + HMAP_FOR_EACH_SAFE (if_cfg, next, hmap_node, &br->if_cfg_todo) { + iface_create(br, if_cfg, -1); + hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node); free(if_cfg); - done = false; time_refresh(); if (time_msec() >= deadline) { - return done; + return false; } } } - return done; + return true; } static bool @@ -528,6 +519,7 @@ bridge_reconfigure_continue(const struct ovsrec_open_vswitch *ovs_cfg) struct bridge *br; bool done; + assert(reconfiguring); done = bridge_reconfigure_ofp(); /* Complete the configuration. */ @@ -601,36 +593,41 @@ bridge_update_ofprotos(void) sset_destroy(&names); sset_destroy(&types); - /* 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) { + /* 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; - struct ofproto_port_dump dump; - OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) { - struct bridge *ofp_br = bridge_lookup(ofproto_port.name); + if (!br2->ofproto) { + continue; + } - if (ofp_br && ofp_br != br) { - ofproto_port_del(br->ofproto, ofproto_port.ofp_port); + 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); } } - } - /* 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); - } + 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); } } } @@ -1175,25 +1172,8 @@ bridge_refresh_ofp_port(struct bridge *br) struct ofproto_port_dump dump; struct ofproto_port ofproto_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); - - 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. */ + /* 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; @@ -1203,7 +1183,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) { @@ -1218,10 +1200,22 @@ bridge_refresh_ofp_port(struct bridge *br) } 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); + } 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); + hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node); + free(if_cfg); + } 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); + } } } @@ -1244,31 +1238,40 @@ bridge_refresh_ofp_port(struct bridge *br) } } +/* Initialize 'iface' with a netdev and ofp_port if necessary. */ +/* 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. */ static void -bridge_populate_ofpp_death_row(struct bridge *br) +iface_create(struct bridge *br, struct if_cfg *if_cfg, int ofp_port) { - struct ofproto_port_dump dump; struct ofproto_port ofproto_port; + struct iface *iface; + struct port *port; + int error; - 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); - } + assert(!iface_lookup(br, if_cfg->cfg->name)); + + port = port_lookup(br, if_cfg->parent->name); + if (!port) { + port = port_create(br, if_cfg->parent); } -} -/* 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; + 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); + } - 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, @@ -1305,8 +1308,7 @@ iface_init(struct iface *iface) uint16_t ofp_port; int error; - error = ofproto_port_add(br->ofproto, iface->netdev, - &ofp_port); + 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); @@ -1996,6 +1998,7 @@ 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; @@ -2039,36 +2042,36 @@ 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) { - struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl); - bridge_reconfigure(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. */ - bridge_reconfigure(&null_cfg); + 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 (reconfiguring) { - struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl); + if (!reconf_txn) { + reconf_txn = ovsdb_idl_txn_create(idl); + } if (cfg) { if (bridge_reconfigure_continue(cfg)) { @@ -2077,9 +2080,12 @@ bridge_run(void) } else { bridge_reconfigure_continue(&null_cfg); } + } - ovsdb_idl_txn_commit(txn); - ovsdb_idl_txn_destroy(txn); /* XXX */ + 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. */ @@ -2303,8 +2309,8 @@ 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_init(&br->if_cfg_todo); + list_init(&br->ofpp_garbage); hmap_insert(&all_bridges, &br->node, hash_string(br->name, 0)); } @@ -2316,7 +2322,7 @@ bridge_destroy(struct bridge *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; + struct ofpp_garbage *garbage, *next_garbage; HMAP_FOR_EACH_SAFE (port, next_port, hmap_node, &br->ports) { port_destroy(port); @@ -2324,18 +2330,15 @@ 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); + 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); @@ -2343,6 +2346,7 @@ bridge_destroy(struct bridge *br) 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); @@ -2437,44 +2441,23 @@ bridge_queue_if_cfg(struct bridge *br, 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); - } - } - } + 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 it's configuration. */ + * '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 new_ports; size_t i; - assert(list_is_empty(&br->if_cfg_queue)); + assert(hmap_is_empty(&br->if_cfg_todo)); /* Collect new ports. */ shash_init(&new_ports); @@ -2517,7 +2500,23 @@ bridge_add_del_ports(struct bridge *br, } } - bridge_update_ifaces(br, &new_ports); + + 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); } @@ -3068,30 +3067,6 @@ port_is_synthetic(const struct port *port) /* Interface functions. */ -static struct iface * -iface_create(struct port *port, const struct ovsrec_interface *if_cfg) -{ - 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; -} - static void iface_refresh_type(struct iface *iface) { @@ -3103,24 +3078,6 @@ iface_refresh_type(struct iface *iface) : "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) { @@ -3176,6 +3133,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) { -- 1.7.9.6 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev