[ovs-dev] [PATCH] bond: Remove stable bond mode.
Justin Pettit
jpettit at nicira.com
Fri Aug 17 00:03:36 UTC 2012
I haven't looked at the patch, but do you think it's worth mentioning in NEWS?
--Justin
On Aug 16, 2012, at 3:40 PM, Ethan Jackson <ethan at nicira.com> wrote:
> Stable bond modes are an obsolete attempt to replicate the
> functionality contained in the bundle action. They are ugly and of
> questionable usefulness, hence their removal.
>
> Signed-off-by: Ethan Jackson <ethan at nicira.com>
> ---
> lib/bond.c | 82 +++-------------------------------------------
> lib/bond.h | 4 +--
> ofproto/ofproto-dpif.c | 12 ++-----
> ofproto/ofproto.h | 1 -
> vswitchd/bridge.c | 22 ++-----------
> vswitchd/vswitch.ovsschema | 6 ++--
> vswitchd/vswitch.xml | 25 --------------
> 7 files changed, 15 insertions(+), 137 deletions(-)
>
> diff --git a/lib/bond.c b/lib/bond.c
> index 7178416..b7be705 100644
> --- a/lib/bond.c
> +++ b/lib/bond.c
> @@ -74,9 +74,6 @@ struct bond_slave {
> struct list bal_node; /* In bond_rebalance()'s 'bals' list. */
> struct list entries; /* 'struct bond_entry's assigned here. */
> uint64_t tx_bytes; /* Sum across 'tx_bytes' of entries. */
> -
> - /* BM_STABLE specific bonding info. */
> - uint32_t stb_id; /* ID used for 'stb_slaves' ordering. */
> };
>
> /* A bond, that is, a set of network devices grouped to improve performance or
> @@ -103,9 +100,6 @@ struct bond {
> long long int next_rebalance; /* Next rebalancing time. */
> bool send_learning_packets;
>
> - /* BM_STABLE specific bonding info. */
> - tag_type stb_tag; /* Tag associated with this bond. */
> -
> /* Legacy compatibility. */
> long long int next_fake_iface_update; /* LLONG_MAX if disabled. */
>
> @@ -146,8 +140,6 @@ bond_mode_from_string(enum bond_mode *balance, const char *s)
> *balance = BM_TCP;
> } else if (!strcmp(s, bond_mode_to_string(BM_SLB))) {
> *balance = BM_SLB;
> - } else if (!strcmp(s, bond_mode_to_string(BM_STABLE))) {
> - *balance = BM_STABLE;
> } else if (!strcmp(s, bond_mode_to_string(BM_AB))) {
> *balance = BM_AB;
> } else {
> @@ -164,8 +156,6 @@ bond_mode_to_string(enum bond_mode balance) {
> return "balance-tcp";
> case BM_SLB:
> return "balance-slb";
> - case BM_STABLE:
> - return "stable";
> case BM_AB:
> return "active-backup";
> }
> @@ -186,7 +176,6 @@ bond_create(const struct bond_settings *s)
> bond = xzalloc(sizeof *bond);
> hmap_init(&bond->slaves);
> bond->no_slaves_tag = tag_create_random();
> - bond->stb_tag = tag_create_random();
> bond->next_fake_iface_update = LLONG_MAX;
>
> bond_reconfigure(bond, s);
> @@ -296,17 +285,12 @@ bond_slave_set_netdev__(struct bond_slave *slave, struct netdev *netdev)
> * bond. If 'slave_' already exists within 'bond' then this function
> * reconfigures the existing slave.
> *
> - * 'stb_id' is used in BM_STABLE bonds to guarantee consistent slave choices
> - * across restarts and distributed vswitch instances. It should be unique per
> - * slave, and preferably consistent across restarts and reconfigurations.
> - *
> * 'netdev' must be the network device that 'slave_' represents. It is owned
> * by the client, so the client must not close it before either unregistering
> * 'slave_' or destroying 'bond'.
> */
> void
> -bond_slave_register(struct bond *bond, void *slave_, uint32_t stb_id,
> - struct netdev *netdev)
> +bond_slave_register(struct bond *bond, void *slave_, struct netdev *netdev)
> {
> struct bond_slave *slave = bond_slave_lookup(bond, slave_);
>
> @@ -324,11 +308,6 @@ bond_slave_register(struct bond *bond, void *slave_, uint32_t stb_id,
> bond_enable_slave(slave, netdev_get_carrier(netdev), NULL);
> }
>
> - if (slave->stb_id != stb_id) {
> - slave->stb_id = stb_id;
> - bond->bond_revalidate = true;
> - }
> -
> bond_slave_set_netdev__(slave, netdev);
>
> free(slave->name);
> @@ -432,18 +411,11 @@ bond_run(struct bond *bond, struct tag_set *tags, enum lacp_status lacp_status)
>
> if (bond->bond_revalidate) {
> bond->bond_revalidate = false;
> -
> bond_entry_reset(bond);
> - if (bond->balance != BM_STABLE) {
> - struct bond_slave *slave;
> -
> - HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> - tag_set_add(tags, slave->tag);
> - }
> - } else {
> - tag_set_add(tags, bond->stb_tag);
> - }
> tag_set_add(tags, bond->no_slaves_tag);
> + HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> + tag_set_add(tags, slave->tag);
> + }
> }
>
> /* Invalidate any tags required by */
> @@ -488,7 +460,6 @@ static bool
> may_send_learning_packets(const struct bond *bond)
> {
> return bond->lacp_status == LACP_DISABLED
> - && bond->balance != BM_STABLE
> && bond->active_slave;
> }
>
> @@ -613,9 +584,6 @@ bond_check_admissibility(struct bond *bond, const void *slave_,
> * exception is if we locked the learning table to avoid reflections on
> * bond slaves. */
> return BV_DROP_IF_MOVED;
> -
> - case BM_STABLE:
> - return BV_ACCEPT;
> }
>
> NOT_REACHED();
> @@ -639,7 +607,7 @@ bond_choose_output_slave(struct bond *bond, const struct flow *flow,
> {
> struct bond_slave *slave = choose_output_slave(bond, flow, vlan);
> if (slave) {
> - *tags |= bond->balance == BM_STABLE ? bond->stb_tag : slave->tag;
> + *tags |= slave->tag;
> return slave->aux;
> } else {
> *tags |= bond->no_slaves_tag;
> @@ -1284,7 +1252,6 @@ bond_slave_lookup(struct bond *bond, const void *slave_)
> static void
> bond_enable_slave(struct bond_slave *slave, bool enable, struct tag_set *tags)
> {
> - struct bond *bond = slave->bond;
> slave->delay_expires = LLONG_MAX;
> if (enable != slave->enabled) {
> slave->enabled = enable;
> @@ -1297,10 +1264,6 @@ bond_enable_slave(struct bond_slave *slave, bool enable, struct tag_set *tags)
> VLOG_WARN("interface %s: enabled", slave->name);
> slave->tag = tag_create_random();
> }
> -
> - if (bond->balance == BM_STABLE) {
> - bond->bond_revalidate = true;
> - }
> }
> }
>
> @@ -1374,35 +1337,6 @@ lookup_bond_entry(const struct bond *bond, const struct flow *flow,
> return &bond->hash[bond_hash(bond, flow, vlan) & BOND_MASK];
> }
>
> -/* This function uses Highest Random Weight hashing to choose an output slave.
> - * This approach only reassigns a minimal number of flows when slaves are
> - * enabled or disabled. Unfortunately, it has O(n) performance against the
> - * number of slaves. There exist algorithms which are O(1), but have slightly
> - * more complex implementations and require the use of memory. This may need
> - * to be reimplemented if it becomes a performance bottleneck. */
> -static struct bond_slave *
> -choose_stb_slave(const struct bond *bond, uint32_t flow_hash)
> -{
> - struct bond_slave *best, *slave;
> - uint32_t best_hash;
> -
> - best = NULL;
> - best_hash = 0;
> - HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> - if (slave->enabled) {
> - uint32_t hash;
> -
> - hash = hash_2words(flow_hash, slave->stb_id);
> - if (!best || hash > best_hash) {
> - best = slave;
> - best_hash = hash;
> - }
> - }
> - }
> -
> - return best;
> -}
> -
> static struct bond_slave *
> choose_output_slave(const struct bond *bond, const struct flow *flow,
> uint16_t vlan)
> @@ -1419,9 +1353,6 @@ choose_output_slave(const struct bond *bond, const struct flow *flow,
> case BM_AB:
> return bond->active_slave;
>
> - case BM_STABLE:
> - return choose_stb_slave(bond, bond_hash_tcp(flow, vlan, bond->basis));
> -
> case BM_TCP:
> if (bond->lacp_status != LACP_NEGOTIATED) {
> /* Must have LACP negotiations for TCP balanced bonds. */
> @@ -1429,9 +1360,6 @@ choose_output_slave(const struct bond *bond, const struct flow *flow,
> }
> /* Fall Through. */
> case BM_SLB:
> - if (!bond_is_balanced(bond)) {
> - return choose_stb_slave(bond, bond_hash(bond, flow, vlan));
> - }
> e = lookup_bond_entry(bond, flow, vlan);
> if (!e->slave || !e->slave->enabled) {
> e->slave = CONTAINER_OF(hmap_random_node(&bond->slaves),
> diff --git a/lib/bond.h b/lib/bond.h
> index 7329db7..6190081 100644
> --- a/lib/bond.h
> +++ b/lib/bond.h
> @@ -32,7 +32,6 @@ enum lacp_status;
> enum bond_mode {
> BM_TCP, /* Transport Layer Load Balance. */
> BM_SLB, /* Source Load Balance. */
> - BM_STABLE, /* Stable. */
> BM_AB /* Active Backup. */
> };
>
> @@ -65,8 +64,7 @@ struct bond *bond_create(const struct bond_settings *);
> void bond_destroy(struct bond *);
>
> bool bond_reconfigure(struct bond *, const struct bond_settings *);
> -void bond_slave_register(struct bond *, void *slave_,
> - uint32_t stable_id, struct netdev *);
> +void bond_slave_register(struct bond *, void *slave_, struct netdev *);
> void bond_slave_set_netdev(struct bond *, void *slave_, struct netdev *);
> void bond_slave_unregister(struct bond *, const void *slave);
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index a7e85de..45c2abc 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -490,7 +490,6 @@ struct ofport_dpif {
> struct list bundle_node; /* In struct ofbundle's "ports" list. */
> struct cfm *cfm; /* Connectivity Fault Management, if any. */
> tag_type tag; /* Tag associated with this port. */
> - uint32_t bond_stable_id; /* stable_id to use as bond slave, or 0. */
> bool may_enable; /* May be enabled in bonds. */
> long long int carrier_seq; /* Carrier status changes. */
>
> @@ -1785,8 +1784,7 @@ bundle_del_port(struct ofport_dpif *port)
>
> static bool
> bundle_add_port(struct ofbundle *bundle, uint32_t ofp_port,
> - struct lacp_slave_settings *lacp,
> - uint32_t bond_stable_id)
> + struct lacp_slave_settings *lacp)
> {
> struct ofport_dpif *port;
>
> @@ -1813,8 +1811,6 @@ bundle_add_port(struct ofbundle *bundle, uint32_t ofp_port,
> lacp_slave_register(bundle->lacp, port, lacp);
> }
>
> - port->bond_stable_id = bond_stable_id;
> -
> return true;
> }
>
> @@ -1922,8 +1918,7 @@ bundle_set(struct ofproto *ofproto_, void *aux,
> ok = true;
> for (i = 0; i < s->n_slaves; i++) {
> if (!bundle_add_port(bundle, s->slaves[i],
> - s->lacp ? &s->lacp_slaves[i] : NULL,
> - s->bond_stable_ids ? s->bond_stable_ids[i] : 0)) {
> + s->lacp ? &s->lacp_slaves[i] : NULL)) {
> ok = false;
> }
> }
> @@ -2023,8 +2018,7 @@ bundle_set(struct ofproto *ofproto_, void *aux,
> }
>
> LIST_FOR_EACH (port, bundle_node, &bundle->ports) {
> - bond_slave_register(bundle->bond, port, port->bond_stable_id,
> - port->up.netdev);
> + bond_slave_register(bundle->bond, port, port->up.netdev);
> }
> } else {
> bond_destroy(bundle->bond);
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 32a00f2..825f386 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -273,7 +273,6 @@ struct ofproto_bundle_settings {
> bool use_priority_tags; /* Use 802.1p tag for frames in VLAN 0? */
>
> struct bond_settings *bond; /* Must be nonnull iff if n_slaves > 1. */
> - uint32_t *bond_stable_ids; /* Array of n_slaves elements. */
>
> struct lacp_settings *lacp; /* Nonnull to enable LACP. */
> struct lacp_slave_settings *lacp_slaves; /* Array of n_slaves elements. */
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index bd8e772..7ff02b8 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -210,8 +210,7 @@ static struct port *port_lookup(const struct bridge *, const char *name);
> static void port_configure(struct port *);
> static struct lacp_settings *port_configure_lacp(struct port *,
> struct lacp_settings *);
> -static void port_configure_bond(struct port *, struct bond_settings *,
> - uint32_t *bond_stable_ids);
> +static void port_configure_bond(struct port *, struct bond_settings *);
> static bool port_is_synthetic(const struct port *);
>
> static void reconfigure_system_stats(const struct ovsrec_open_vswitch *);
> @@ -710,11 +709,9 @@ port_configure(struct port *port)
> /* Get bond settings. */
> if (s.n_slaves > 1) {
> s.bond = &bond_settings;
> - s.bond_stable_ids = xmalloc(s.n_slaves * sizeof *s.bond_stable_ids);
> - port_configure_bond(port, &bond_settings, s.bond_stable_ids);
> + port_configure_bond(port, &bond_settings);
> } else {
> s.bond = NULL;
> - s.bond_stable_ids = NULL;
>
> LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> netdev_set_miimon_interval(iface->netdev, 0);
> @@ -728,7 +725,6 @@ port_configure(struct port *port)
> free(s.slaves);
> free(s.trunks);
> free(s.lacp_slaves);
> - free(s.bond_stable_ids);
> }
>
> /* Pick local port hardware address and datapath ID for 'br'. */
> @@ -2966,13 +2962,11 @@ iface_configure_lacp(struct iface *iface, struct lacp_slave_settings *s)
> }
>
> static void
> -port_configure_bond(struct port *port, struct bond_settings *s,
> - uint32_t *bond_stable_ids)
> +port_configure_bond(struct port *port, struct bond_settings *s)
> {
> const char *detect_s;
> struct iface *iface;
> int miimon_interval;
> - size_t i;
>
> s->name = port->name;
> s->balance = BM_AB;
> @@ -3024,17 +3018,7 @@ port_configure_bond(struct port *port, struct bond_settings *s,
>
> s->fake_iface = port->cfg->bond_fake_iface;
>
> - i = 0;
> LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> - long long stable_id;
> -
> - stable_id = smap_get_int(&iface->cfg->other_config, "bond-stable-id",
> - 0);
> - if (stable_id <= 0 || stable_id >= UINT32_MAX) {
> - stable_id = iface->ofp_port;
> - }
> - bond_stable_ids[i++] = stable_id;
> -
> netdev_set_miimon_interval(iface->netdev, miimon_interval);
> }
> }
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index bbfb01f..8f6fcdf 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
> {"name": "Open_vSwitch",
> - "version": "6.10.0",
> - "cksum": "3699312094 16958",
> + "version": "6.11.0",
> + "cksum": "854639069 16948",
> "tables": {
> "Open_vSwitch": {
> "columns": {
> @@ -130,7 +130,7 @@
> "min": 0, "max": 1}},
> "bond_mode": {
> "type": {"key": {"type": "string",
> - "enum": ["set", ["balance-tcp", "balance-slb", "active-backup", "stable"]]},
> + "enum": ["set", ["balance-tcp", "balance-slb", "active-backup"]]},
> "min": 0, "max": 1}},
> "lacp": {
> "type": {"key": {"type": "string",
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 7b30ce8..7021c3a 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -831,21 +831,6 @@
> information such as destination MAC address, IP address, and TCP
> port.
> </dd>
> -
> - <dt><code>stable</code></dt>
> - <dd>
> - <p>Attempts to always assign a given flow to the same slave
> - consistently. In an effort to maintain stability, no load
> - balancing is done. Uses a similar hashing strategy to
> - <code>balance-tcp</code>, always taking into account L3 and L4
> - fields even if LACP negotiations are unsuccessful. </p>
> - <p>Slave selection decisions are made based on <ref table="Interface"
> - column="other_config" key="bond-stable-id"/> if set. Otherwise,
> - OpenFlow port number is used. Decisions are consistent across all
> - <code>ovs-vswitchd</code> instances with equivalent
> - <ref table="Interface" column="other_config" key="bond-stable-id"/>
> - values.</p>
> - </dd>
> </dl>
>
> <p>These columns apply only to bonded ports. Their values are
> @@ -1860,16 +1845,6 @@
> </group>
>
> <group title="Bonding Configuration">
> - <column name="other_config" key="bond-stable-id"
> - type='{"type": "integer", "minInteger": 1}'>
> - Used in <code>stable</code> bond mode to make slave
> - selection decisions. Allocating <ref column="other_config"
> - key="bond-stable-id"/> values consistently across interfaces
> - participating in a bond will guarantee consistent slave selection
> - decisions across <code>ovs-vswitchd</code> instances when using
> - <code>stable</code> bonding mode.
> - </column>
> -
> <column name="other_config" key="lacp-port-id"
> type='{"type": "integer", "minInteger": 1, "maxInteger": 65535}'>
> The LACP port ID of this <ref table="Interface"/>. Port IDs are
> --
> 1.7.11.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list