Does that mean we have to buy you another cookie?
--Justin
On Apr 22, 2012, at 6:25 PM, Ethan Jackson <et...@nicira.com> wrote:
> 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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
