[ovs-dev] [bond 6/7] lacp: Require successful LACP negotiations when configured.

Ethan Jackson ethan at nicira.com
Fri Jan 13 19:56:49 UTC 2012


Here's the original version of the patch that I'd written.  I had remvoed the
LACP_CONFIGURED enum because I don't think it's strictly necessary.  The same
behavior can be achieved by dropping all traffic on non-enabled slaves in the
admissibility function.  However, I think this patch is *much* easier to
understand so I'm proposing it now.

Ethan

---
In the original Open vSwitch LACP implementation, when no slaves
found a LACP partner, the LACP module would attach all of them.
This allowed the LACP bond to fall back to a standard bond when
partnered with a non-LACP switch.  In practice, this has caused
confusion with marginal benefit, so this feature is removed with
this patch.

Signed-off-by: Ethan Jackson <ethan at nicira.com>
---
 NEWS                   |    3 ++
 lib/bond.c             |   91 ++++++++++++++++++++++++++++--------------------
 lib/bond.h             |    3 +-
 lib/lacp.c             |   30 ++++++++--------
 lib/lacp.h             |    8 ++++-
 ofproto/ofproto-dpif.c |    2 +-
 tests/lacp.at          |    2 +-
 vswitchd/vswitch.xml   |    8 ++--
 8 files changed, 86 insertions(+), 61 deletions(-)

diff --git a/NEWS b/NEWS
index b628e29..8ff87c4 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,8 @@
 post-v1.5.0
 ------------------------
+    - bonding
+        - LACP bonds no longer fall back to balance-slb when negotiations fail.
+          Instead they drop traffic.
 
 
 v1.5.0 - xx xxx xxxx
diff --git a/lib/bond.c b/lib/bond.c
index a2105ca..50a1d5d 100644
--- a/lib/bond.c
+++ b/lib/bond.c
@@ -26,6 +26,7 @@
 #include "dynamic-string.h"
 #include "flow.h"
 #include "hmap.h"
+#include "lacp.h"
 #include "list.h"
 #include "netdev.h"
 #include "odp-util.h"
@@ -92,7 +93,7 @@ struct bond {
     struct bond_slave *active_slave;
     tag_type no_slaves_tag;     /* Tag for flows when all slaves disabled. */
     int updelay, downdelay;     /* Delay before slave goes up/down, in ms. */
-    bool lacp_negotiated;       /* LACP negotiations were successful. */
+    enum lacp_status lacp_status; /* Status of LACP negotiations. */
     bool bond_revalidate;       /* True if flows need revalidation. */
     uint32_t basis;             /* Basis for flow hash function. */
 
@@ -122,7 +123,6 @@ static void bond_enable_slave(struct bond_slave *, bool enable,
                               struct tag_set *);
 static void bond_link_status_update(struct bond_slave *, struct tag_set *);
 static void bond_choose_active_slave(struct bond *, struct tag_set *);
-static bool bond_is_tcp_hash(const struct bond *);
 static unsigned int bond_hash_src(const uint8_t mac[ETH_ADDR_LEN],
                                   uint16_t vlan, uint32_t basis);
 static unsigned int bond_hash_tcp(const struct flow *, uint16_t vlan,
@@ -402,13 +402,12 @@ bond_slave_set_may_enable(struct bond *bond, void *slave_, bool may_enable)
  *
  * The caller should check bond_should_send_learning_packets() afterward. */
 void
-bond_run(struct bond *bond, struct tag_set *tags, bool lacp_negotiated)
+bond_run(struct bond *bond, struct tag_set *tags, enum lacp_status lacp_status)
 {
     struct bond_slave *slave;
-    bool is_tcp_hash = bond_is_tcp_hash(bond);
 
-    if (bond->lacp_negotiated != lacp_negotiated) {
-        bond->lacp_negotiated = lacp_negotiated;
+    if (bond->lacp_status != lacp_status) {
+        bond->lacp_status = lacp_status;
         bond->bond_revalidate = true;
     }
 
@@ -427,10 +426,6 @@ bond_run(struct bond *bond, struct tag_set *tags, bool lacp_negotiated)
         bond->next_fake_iface_update = time_msec() + 1000;
     }
 
-    if (is_tcp_hash != bond_is_tcp_hash(bond)) {
-        bond->bond_revalidate = true;
-    }
-
     if (bond->bond_revalidate) {
         bond->bond_revalidate = false;
 
@@ -488,8 +483,9 @@ bond_wait(struct bond *bond)
 static bool
 may_send_learning_packets(const struct bond *bond)
 {
-    return !bond->lacp_negotiated && bond->balance != BM_AB &&
-           bond->active_slave;
+    return bond->lacp_status == LACP_DISABLED
+        && bond->balance != BM_AB
+        && bond->active_slave;
 }
 
 /* Returns true if 'bond' needs the client to send out packets to assist with
@@ -566,9 +562,14 @@ bond_check_admissibility(struct bond *bond, const void *slave_,
      * assume the remote switch is aware of the bond and will "do the right
      * thing".  However, as a precaution we drop packets on disabled slaves
      * because no correctly implemented partner switch should be sending
-     * packets to them. */
-    if (bond->lacp_negotiated) {
-        return slave->enabled ? BV_ACCEPT : BV_DROP;
+     * packets to them.
+     *
+     * If LACP is configured, but LACP negotiations have been unsuccessful, we
+     * drop all incoming traffic. */
+    switch (bond->lacp_status) {
+    case LACP_NEGOTIATED: return slave->enabled ? BV_ACCEPT : BV_DROP;
+    case LACP_CONFIGURED: return BV_DROP;
+    case LACP_DISABLED: break;
     }
 
     /* Drop all multicast packets on inactive slaves. */
@@ -595,10 +596,11 @@ bond_check_admissibility(struct bond *bond, const void *slave_,
         return BV_ACCEPT;
 
     case BM_TCP:
-        /* TCP balancing has degraded to SLB (otherwise the
-         * bond->lacp_negotiated check above would have processed this).
-         *
-         * Fall through. */
+        /* TCP balanced bonds require successful LACP negotiated. Based on the
+         * above check, LACP is off on this bond.  Therfore, we drop all
+         * incoming traffic. */
+        return BV_DROP;
+
     case BM_SLB:
         /* Drop all packets for which we have learned a different input port,
          * because we probably sent the packet on one slave and got it back on
@@ -950,11 +952,6 @@ bond_print_details(struct ds *ds, const struct bond *bond)
     ds_put_format(ds, "bond_mode: %s\n",
                   bond_mode_to_string(bond->balance));
 
-    if (bond->balance != BM_AB) {
-        ds_put_format(ds, "bond-hash-algorithm: %s\n",
-                      bond_is_tcp_hash(bond) ? "balance-tcp" : "balance-slb");
-    }
-
     ds_put_format(ds, "bond-hash-basis: %"PRIu32"\n", bond->basis);
 
     ds_put_format(ds, "updelay: %d ms\n", bond->updelay);
@@ -965,8 +962,21 @@ bond_print_details(struct ds *ds, const struct bond *bond)
                       bond->next_rebalance - time_msec());
     }
 
-    ds_put_format(ds, "lacp_negotiated: %s\n",
-                  bond->lacp_negotiated ? "true" : "false");
+    ds_put_cstr(ds, "lacp_status: ");
+    switch (bond->lacp_status) {
+    case LACP_NEGOTIATED:
+        ds_put_cstr(ds, "negotiated\n");
+        break;
+    case LACP_CONFIGURED:
+        ds_put_cstr(ds, "configured\n");
+        break;
+    case LACP_DISABLED:
+        ds_put_cstr(ds, "off\n");
+        break;
+    default:
+        ds_put_cstr(ds, "<unknown>\n");
+        break;
+    }
 
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
         shash_add(&slave_shash, slave->name, slave);
@@ -1305,7 +1315,7 @@ bond_link_status_update(struct bond_slave *slave, struct tag_set *tags)
             VLOG_INFO_RL(&rl, "interface %s: will not be %s",
                          slave->name, up ? "disabled" : "enabled");
         } else {
-            int delay = (bond->lacp_negotiated ? 0
+            int delay = (bond->lacp_status != LACP_DISABLED ? 0
                          : up ? bond->updelay : bond->downdelay);
             slave->delay_expires = time_msec() + delay;
             if (delay) {
@@ -1324,13 +1334,6 @@ bond_link_status_update(struct bond_slave *slave, struct tag_set *tags)
     }
 }
 
-static bool
-bond_is_tcp_hash(const struct bond *bond)
-{
-    return (bond->balance == BM_TCP && bond->lacp_negotiated)
-        || bond->balance == BM_STABLE;
-}
-
 static unsigned int
 bond_hash_src(const uint8_t mac[ETH_ADDR_LEN], uint16_t vlan, uint32_t basis)
 {
@@ -1352,9 +1355,9 @@ bond_hash_tcp(const struct flow *flow, uint16_t vlan, uint32_t basis)
 static unsigned int
 bond_hash(const struct bond *bond, const struct flow *flow, uint16_t vlan)
 {
-    assert(bond->balance != BM_AB);
+    assert(bond->balance == BM_TCP || bond->balance == BM_SLB);
 
-    return (bond_is_tcp_hash(bond)
+    return (bond->balance == BM_TCP
             ? bond_hash_tcp(flow, vlan, bond->basis)
             : bond_hash_src(flow->dl_src, vlan, bond->basis));
 }
@@ -1381,7 +1384,7 @@ choose_stb_slave(const struct bond *bond, const struct flow *flow,
 
     best = NULL;
     best_hash = 0;
-    flow_hash = bond_hash(bond, flow, vlan);
+    flow_hash = bond_hash_tcp(flow, vlan, bond->basis);
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
         if (slave->enabled) {
             uint32_t hash;
@@ -1403,14 +1406,26 @@ choose_output_slave(const struct bond *bond, const struct flow *flow,
 {
     struct bond_entry *e;
 
+    if (bond->lacp_status == LACP_CONFIGURED) {
+        /* LACP has been configured on this bond but negotiations were
+         * unsuccussful.  Drop all traffic. */
+        return NULL;
+    }
+
     switch (bond->balance) {
     case BM_AB:
         return bond->active_slave;
 
     case BM_STABLE:
         return choose_stb_slave(bond, flow, vlan);
-    case BM_SLB:
+
     case BM_TCP:
+        if (bond->lacp_status != LACP_NEGOTIATED) {
+            /* Must have LACP negotiations for TCP balanced bonds. */
+            return NULL;
+        }
+        /* Fall Through. */
+    case BM_SLB:
         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 1958029..9eb1b8f 100644
--- a/lib/bond.h
+++ b/lib/bond.h
@@ -26,6 +26,7 @@
 struct flow;
 struct netdev;
 struct ofpbuf;
+enum lacp_status;
 
 /* How flows are balanced among bond slaves. */
 enum bond_mode {
@@ -68,7 +69,7 @@ void bond_slave_register(struct bond *, void *slave_,
 void bond_slave_set_netdev(struct bond *, void *slave_, struct netdev *);
 void bond_slave_unregister(struct bond *, const void *slave);
 
-void bond_run(struct bond *, struct tag_set *, bool lacp_negotiated);
+void bond_run(struct bond *, struct tag_set *, enum lacp_status);
 void bond_wait(struct bond *);
 
 void bond_slave_set_may_enable(struct bond *, void *slave_, bool may_enable);
diff --git a/lib/lacp.c b/lib/lacp.c
index 244b86e..c51fac6 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -301,12 +301,17 @@ lacp_process_packet(struct lacp *lacp, const void *slave_,
     }
 }
 
-/* Returns true if 'lacp' has successfully negotiated with its partner.  False
- * if 'lacp' is NULL. */
-bool
-lacp_negotiated(const struct lacp *lacp)
+/* Returns the lacp_status of the given 'lacp' object (which may be NULL). */
+enum lacp_status
+lacp_status(const struct lacp *lacp)
 {
-    return lacp ? lacp->negotiated : false;
+    if (!lacp) {
+        return LACP_DISABLED;
+    } else if (lacp->negotiated) {
+        return LACP_NEGOTIATED;
+    } else {
+        return LACP_CONFIGURED;
+    }
 }
 
 /* Registers 'slave_' as subordinate to 'lacp'.  This should be called at least
@@ -384,12 +389,8 @@ lacp_slave_may_enable(const struct lacp *lacp, const void *slave_)
         struct slave *slave = slave_lookup(lacp, slave_);
 
         /* The slave may be enabled if it's attached to an aggregator and its
-         * partner is synchronized.  The only exception is defaulted slaves.
-         * They are not required to have synchronized partners because they
-         * have no partners at all.  They will only be attached if negotiations
-         * failed on all slaves in the bond. */
-        return slave->attached && (slave->partner.state & LACP_STATE_SYNC
-                                   || slave->status == LACP_DEFAULTED);
+         * partner is synchronized.*/
+        return slave->attached && (slave->partner.state & LACP_STATE_SYNC);
     } else {
         return true;
     }
@@ -504,14 +505,13 @@ lacp_update_attached(struct lacp *lacp)
     HMAP_FOR_EACH (slave, node, &lacp->slaves) {
         struct lacp_info pri;
 
-        slave->attached = true;
+        slave->attached = false;
 
         /* XXX: In the future allow users to configure the expected system ID.
          * For now just special case loopback. */
         if (eth_addr_equals(slave->partner.sys_id, slave->lacp->sys_id)) {
             VLOG_WARN_RL(&rl, "slave %s: Loopback detected. Slave is "
                          "connected to its own bond", slave->name);
-            slave->attached = false;
             continue;
         }
 
@@ -519,6 +519,7 @@ lacp_update_attached(struct lacp *lacp)
             continue;
         }
 
+        slave->attached = true;
         slave_get_priority(slave, &pri);
 
         if (!lead || memcmp(&pri, &lead_pri, sizeof pri) < 0) {
@@ -531,8 +532,7 @@ lacp_update_attached(struct lacp *lacp)
 
     if (lead) {
         HMAP_FOR_EACH (slave, node, &lacp->slaves) {
-            if (slave->status == LACP_DEFAULTED
-                || lead->partner.key != slave->partner.key
+            if (lead->partner.key != slave->partner.key
                 || !eth_addr_equals(lead->partner.sys_id,
                                     slave->partner.sys_id)) {
                 slave->attached = false;
diff --git a/lib/lacp.h b/lib/lacp.h
index 1d717d6..293fc45 100644
--- a/lib/lacp.h
+++ b/lib/lacp.h
@@ -29,6 +29,12 @@ enum lacp_time {
     LACP_TIME_CUSTOM                  /* Nonstandard custom mode. */
 };
 
+enum lacp_status {
+    LACP_NEGOTIATED,                  /* Successful LACP negotations. */
+    LACP_CONFIGURED,                  /* LACP is enabled but not negotiated. */
+    LACP_DISABLED                     /* LACP is not enabled. */
+};
+
 struct lacp_settings {
     char *name;                       /* Name (for debugging). */
     uint8_t id[ETH_ADDR_LEN];         /* System ID. Must be nonzero. */
@@ -48,7 +54,7 @@ bool lacp_is_active(const struct lacp *);
 
 void lacp_process_packet(struct lacp *, const void *slave,
                          const struct ofpbuf *packet);
-bool lacp_negotiated(const struct lacp *);
+enum lacp_status lacp_status(const struct lacp *);
 
 struct lacp_slave_settings {
     char *name;                       /* Name (for debugging). */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6ecf71b..34dc903 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1866,7 +1866,7 @@ bundle_run(struct ofbundle *bundle)
         }
 
         bond_run(bundle->bond, &bundle->ofproto->revalidate_set,
-                 lacp_negotiated(bundle->lacp));
+                 lacp_status(bundle->lacp));
         if (bond_should_send_learning_packets(bundle->bond)) {
             bundle_send_learning_packets(bundle);
         }
diff --git a/tests/lacp.at b/tests/lacp.at
index 16daa3d..947eb9c 100644
--- a/tests/lacp.at
+++ b/tests/lacp.at
@@ -104,7 +104,7 @@ bond_mode: active-backup
 bond-hash-basis: 0
 updelay: 0 ms
 downdelay: 0 ms
-lacp_negotiated: true
+lacp_status: negotiated
 
 slave p1: disabled
 	may_enable: false
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 585f678..dcc25d1 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -757,8 +757,7 @@
 
       <p>
         The following modes require the upstream switch to support 802.3ad with
-        successful LACP negotiation.  If LACP negotiation fails then
-        <code>balance-slb</code> style flow hashing is used as a fallback:
+        successful LACP negotiation:
       </p>
 
       <dl>
@@ -861,8 +860,9 @@
           connected to.  <code>active</code> ports are allowed to initiate LACP
           negotiations.  <code>passive</code> ports are allowed to participate
           in LACP negotiations initiated by a remote switch, but not allowed to
-          initiate such negotiations themselves.  Defaults to <code>off</code>
-          if unset.
+          initiate such negotiations themselves.  If LACP is enabled on a port
+          whose partner switch does not support LACP, the bond will be
+          disabled.  Defaults to <code>off</code> if unset.
         </column>
 
         <column name="other_config" key="lacp-system-id">
-- 
1.7.7.1




More information about the dev mailing list