I've decided to drop this in favor of the strategy mentioned below.
On Thu, Aug 16, 2012 at 5:06 PM, Ben Pfaff <[email protected]> wrote: > I proposed to Ethan via IM that we should do this kind of non-urgent > feature removal in a couple of steps: > > - Mention on ovs-dev that we're deprecating it. > > - Add to NEWS that we're deprecating it and will remove it at > some date, perhaps 6 months away. > > - After that time, if no one has complained, remove it. > > Another step that perhaps we should add is to log a warning or error > the first time that such a deprecated feature is used. > > On Thu, Aug 16, 2012 at 05:03:36PM -0700, Justin Pettit wrote: >> 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 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
