[ovs-dev] [PATCH 2/2] bridge: Refactor bridge_reconfigure().

Ethan Jackson ethan at nicira.com
Mon Apr 23 01:44:56 UTC 2012


Oh no, I plan to buy myself a glass of scotch and a slice of pie.
1000 commits would be a cookie.  I think we owe Ben two.

Ethan

On Sun, Apr 22, 2012 at 18:38, Justin Pettit <jpettit at nicira.com> wrote:
> Does that mean we have to buy you another cookie?
>
> --Justin
>
>
> On Apr 22, 2012, at 6:25 PM, Ethan Jackson <ethan at nicira.com> wrote:
>
>> Assuming this makes it in, will be my 500th commit.
>>
>> Ethan
>>
>> On Sun, Apr 22, 2012 at 14:22, Ethan Jackson <ethan at 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 <ethan at 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 at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list