[ovs-dev] [PATCH] bond: Remove stable bond mode.

Ethan Jackson ethan at nicira.com
Sat Aug 18 01:45:30 UTC 2012


I've decided to drop this in favor of the strategy mentioned below.

On Thu, Aug 16, 2012 at 5:06 PM, Ben Pfaff <blp at nicira.com> 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 <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
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list