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 <[email protected]> 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 <[email protected]> > --- > 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 > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
