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

Ethan Jackson ethan at nicira.com
Tue Feb 5 03:13:35 UTC 2013


Stable bond mode, along with autopath, were trying to implement
functionality close to what we get from the bundle action.
Unfortunately, they are quite clunky, and generally less useful
than bundle, so they're being removed.

Signed-off-by: Ethan Jackson <ethan at nicira.com>
---
 NEWS                       |    1 +
 lib/bond.c                 |   65 ++++++++------------------------------------
 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, 23 insertions(+), 115 deletions(-)

diff --git a/NEWS b/NEWS
index 1164962..490212d 100644
--- a/NEWS
+++ b/NEWS
@@ -28,6 +28,7 @@ post-v1.9.0
     - Patch ports are implemented in userspace.
     - Tunneling requires the version of the kernel module paired with Open
       vSwitch 1.9.0 or later.
+    - Stable bond mode has been removed.
 
 
 v1.9.0 - xx xxx xxxx
diff --git a/lib/bond.c b/lib/bond.c
index c74bcfb..8b1dbc8 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;
-        }
     }
 }
 
@@ -1394,18 +1354,20 @@ lookup_bond_entry(const struct bond *bond, const struct flow *flow,
  * 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)
+choose_hash_slave(const struct bond *bond, const struct flow *flow,
+                  uint16_t vlan)
 {
     struct bond_slave *best, *slave;
-    uint32_t best_hash;
+    uint32_t best_hash, flow_hash;
 
     best = NULL;
     best_hash = 0;
+    flow_hash = bond_hash(bond, flow, vlan);
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
         if (slave->enabled) {
             uint32_t hash;
 
-            hash = hash_2words(flow_hash, slave->stb_id);
+            hash = hash_pointer(slave, flow_hash);
             if (!best || hash > best_hash) {
                 best = slave;
                 best_hash = hash;
@@ -1432,9 +1394,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. */
@@ -1447,7 +1406,7 @@ choose_output_slave(const struct bond *bond, const struct flow *flow,
             e->slave = CONTAINER_OF(hmap_random_node(&bond->slaves),
                                     struct bond_slave, hmap_node);
             if (!e->slave->enabled) {
-                e->slave = choose_stb_slave(bond, bond_hash(bond, flow, vlan));
+                e->slave = choose_hash_slave(bond, flow, vlan);
             }
             e->tag = tag_create_random();
         }
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 b1ec3fb..41e63d8 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