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

Ethan Jackson ethan at nicira.com
Tue Feb 5 20:56:43 UTC 2013


Since I've dropped the first patch, I think choose_hash_slave() is no longer
necessary.  In this version of the patch, I've removed it.

---
 NEWS                       |    1 +
 lib/bond.c                 |   87 +++-----------------------------------------
 lib/bond.h                 |    4 +-
 ofproto/ofproto-dpif.c     |   12 ++----
 ofproto/ofproto.h          |    1 -
 vswitchd/bridge.c          |   23 ++----------
 vswitchd/vswitch.ovsschema |    6 +--
 vswitchd/vswitch.xml       |   26 -------------
 8 files changed, 17 insertions(+), 143 deletions(-)

diff --git a/NEWS b/NEWS
index 186e1a3..58c0fcc 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
 post-v1.10.0
 ---------------------
+    - Stable bond mode has been removed.
 
 
 v1.10.0 - xx xxx xxxx
diff --git a/lib/bond.c b/lib/bond.c
index 06680ee..ef7d28d 100644
--- a/lib/bond.c
+++ b/lib/bond.c
@@ -75,9 +75,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
@@ -104,9 +101,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. */
 
@@ -147,8 +141,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 {
@@ -165,8 +157,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";
     }
@@ -187,7 +177,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);
@@ -256,12 +245,6 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
     if (bond->balance != s->balance) {
         bond->balance = s->balance;
         revalidate = true;
-
-        if (bond->balance == BM_STABLE) {
-            VLOG_WARN_ONCE("Stable bond mode is deprecated and may be removed"
-                           " in February 2013. Please email"
-                           " dev at openvswitch.org with concerns.");
-        }
     }
 
     if (bond->basis != s->basis) {
@@ -303,17 +286,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_);
 
@@ -331,11 +309,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);
@@ -438,17 +411,12 @@ bond_run(struct bond *bond, struct tag_set *tags, enum lacp_status lacp_status)
     }
 
     if (bond->bond_revalidate) {
-        bond->bond_revalidate = false;
+        struct bond_slave *slave;
 
+        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);
+        HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
+            tag_set_add(tags, slave->tag);
         }
         tag_set_add(tags, bond->no_slaves_tag);
     }
@@ -621,9 +589,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();
@@ -647,7 +612,7 @@ bond_choose_output_slave(struct bond *bond, const struct flow *flow,
 {
     struct bond_slave *slave = choose_output_slave(bond, flow, vlan, tags);
     if (slave) {
-        *tags |= bond->balance == BM_STABLE ? bond->stb_tag : slave->tag;
+        *tags |= slave->tag;
         return slave->aux;
     } else {
         *tags |= bond->no_slaves_tag;
@@ -1297,7 +1262,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;
@@ -1310,10 +1274,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;
-        }
     }
 }
 
@@ -1387,35 +1347,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, tag_type *tags)
@@ -1432,9 +1363,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. */
@@ -1442,9 +1370,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 f91d3c3..4668994 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -510,7 +510,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. */
     struct tnl_port *tnl_port;  /* Tunnel handle, or null. */
@@ -2215,8 +2214,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;
 
@@ -2243,8 +2241,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;
 }
 
@@ -2352,8 +2348,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;
         }
     }
@@ -2453,8 +2448,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 413472a..9e55f65 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -282,7 +282,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 607ae5e..f5a4366 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -212,8 +212,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 *);
@@ -761,12 +760,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);
         }
@@ -779,7 +775,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'. */
@@ -3111,13 +3106,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;
@@ -3169,17 +3162,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 16125a5..a7901a8 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@
 {"name": "Open_vSwitch",
- "version": "6.11.3",
- "cksum": "2234602985 17310",
+ "version": "7.0.0",
+ "cksum": "3537583872 17299",
  "tables": {
    "Open_vSwitch": {
      "columns": {
@@ -134,7 +134,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 18643c2..cd9a59c 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -858,22 +858,6 @@
           information such as destination MAC address, IP address, and TCP
           port.
         </dd>
-
-        <dt><code>stable</code></dt>
-        <dd>
-          <p>Deprecated and slated for removal in February 2013.</p>
-          <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
@@ -1910,16 +1894,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.9.5




More information about the dev mailing list