flow: Only un-wildcard relevant IP headers.
[sliver-openvswitch.git] / lib / bond.c
index 79fc18c..183c37b 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011 Nicira Networks.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
 #include <limits.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <math.h>
 
 #include "coverage.h"
 #include "dynamic-string.h"
 #include "flow.h"
 #include "hmap.h"
+#include "lacp.h"
 #include "list.h"
 #include "netdev.h"
 #include "odp-util.h"
@@ -49,7 +51,7 @@ VLOG_DEFINE_THIS_MODULE(bond);
 struct bond_entry {
     struct bond_slave *slave;   /* Assigned slave, NULL if unassigned. */
     uint64_t tx_bytes;          /* Count of bytes recently transmitted. */
-    tag_type tag;               /* Tag for entry<->slave association. */
+    tag_type tag;               /* Tag for entry<->facet association. */
     struct list list_node;      /* In bond_slave's 'entries' list. */
 };
 
@@ -73,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
@@ -92,7 +91,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. */
 
@@ -102,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. */
 
@@ -112,6 +108,8 @@ struct bond {
      * where we can't otherwise provide revalidation feedback to the client.
      * That's only unixctl commands now; I hope no other cases will arise. */
     struct tag_set unixctl_tags;
+
+    int ref_cnt;
 };
 
 static struct hmap all_bonds = HMAP_INITIALIZER(&all_bonds);
@@ -122,7 +120,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,
@@ -133,7 +130,8 @@ static struct bond_entry *lookup_bond_entry(const struct bond *,
 static tag_type bond_get_active_slave_tag(const struct bond *);
 static struct bond_slave *choose_output_slave(const struct bond *,
                                               const struct flow *,
-                                              uint16_t vlan);
+                                              struct flow_wildcards *,
+                                              uint16_t vlan, tag_type *tags);
 static void bond_update_fake_slave_stats(struct bond *);
 
 /* Attempts to parse 's' as the name of a bond balancing mode.  If successful,
@@ -146,8 +144,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 +160,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,8 +180,8 @@ 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->ref_cnt = 1;
 
     bond_reconfigure(bond, s);
 
@@ -196,9 +190,19 @@ bond_create(const struct bond_settings *s)
     return bond;
 }
 
+struct bond *
+bond_ref(const struct bond *bond_)
+{
+    struct bond *bond = CONST_CAST(struct bond *, bond_);
+
+    ovs_assert(bond->ref_cnt > 0);
+    bond->ref_cnt++;
+    return bond;
+}
+
 /* Frees 'bond'. */
 void
-bond_destroy(struct bond *bond)
+bond_unref(struct bond *bond)
 {
     struct bond_slave *slave, *next_slave;
 
@@ -206,6 +210,11 @@ bond_destroy(struct bond *bond)
         return;
     }
 
+    ovs_assert(bond->ref_cnt > 0);
+    if (--bond->ref_cnt) {
+        return;
+    }
+
     hmap_remove(&all_bonds, &bond->hmap_node);
 
     HMAP_FOR_EACH_SAFE (slave, next_slave, hmap_node, &bond->slaves) {
@@ -246,7 +255,11 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
 
     bond->updelay = s->up_delay;
     bond->downdelay = s->down_delay;
-    bond->rebalance_interval = s->rebalance_interval;
+
+    if (bond->rebalance_interval != s->rebalance_interval) {
+        bond->rebalance_interval = s->rebalance_interval;
+        revalidate = true;
+    }
 
     if (bond->balance != s->balance) {
         bond->balance = s->balance;
@@ -292,17 +305,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_);
 
@@ -320,11 +328,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);
@@ -402,12 +405,14 @@ 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);
 
-    bond->lacp_negotiated = lacp_negotiated;
+    if (bond->lacp_status != lacp_status) {
+        bond->lacp_status = lacp_status;
+        bond->bond_revalidate = true;
+    }
 
     /* Enable slaves based on link status and LACP feedback. */
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
@@ -424,22 +429,13 @@ 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;
+        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);
     }
@@ -485,8 +481,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_SLB || bond->balance == BM_AB)
+        && bond->active_slave;
 }
 
 /* Returns true if 'bond' needs the client to send out packets to assist with
@@ -495,8 +492,9 @@ may_send_learning_packets(const struct bond *bond)
  * is located.  For each MAC that has been learned on a port other than 'bond',
  * it should call bond_compose_learning_packet().
  *
- * This function will only return true if 'bond' is in SLB mode and LACP is not
- * negotiated.  Otherwise sending learning packets isn't necessary.
+ * This function will only return true if 'bond' is in SLB or active-backup
+ * mode and LACP is not negotiated.  Otherwise sending learning packets isn't
+ * necessary.
  *
  * Calling this function resets the state that it checks. */
 bool
@@ -519,17 +517,17 @@ bond_compose_learning_packet(struct bond *bond,
 {
     struct bond_slave *slave;
     struct ofpbuf *packet;
+    tag_type tags = 0;
     struct flow flow;
 
-    assert(may_send_learning_packets(bond));
+    ovs_assert(may_send_learning_packets(bond));
 
     memset(&flow, 0, sizeof flow);
     memcpy(flow.dl_src, eth_src, ETH_ADDR_LEN);
-    slave = choose_output_slave(bond, &flow, vlan);
+    slave = choose_output_slave(bond, &flow, NULL, vlan, &tags);
 
     packet = ofpbuf_new(0);
-    compose_benign_packet(packet, "Open vSwitch Bond Failover", 0xf177,
-                          eth_src);
+    compose_rarp(packet, eth_src);
     if (vlan) {
         eth_push_vlan(packet, htons(vlan));
     }
@@ -559,19 +557,28 @@ bond_check_admissibility(struct bond *bond, const void *slave_,
 {
     struct bond_slave *slave = bond_slave_lookup(bond, slave_);
 
+    if (!slave) {
+        return BV_DROP;
+    }
+
     /* LACP bonds have very loose admissibility restrictions because we can
      * 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. */
     if (eth_addr_is_multicast(eth_dst)) {
         *tags |= bond_get_active_slave_tag(bond);
-        if (bond->active_slave != bond_slave_lookup(bond, slave_)) {
+        if (bond->active_slave != slave) {
             return BV_DROP;
         }
     }
@@ -592,10 +599,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
@@ -604,9 +612,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();
@@ -623,14 +628,19 @@ bond_check_admissibility(struct bond *bond, const void *slave_,
  * packet belongs to (so for an access port it will be the access port's VLAN).
  *
  * Adds a tag to '*tags' that associates the flow with the returned slave.
+ *
+ * If 'wc' is non-NULL, bitwise-OR's 'wc' with the set of bits that were
+ * significant in the selection.  At some point earlier, 'wc' should
+ * have been initialized (e.g., by flow_wildcards_init_catchall()).
  */
 void *
 bond_choose_output_slave(struct bond *bond, const struct flow *flow,
-                         uint16_t vlan, tag_type *tags)
+                         struct flow_wildcards *wc, uint16_t vlan,
+                         tag_type *tags)
 {
-    struct bond_slave *slave = choose_output_slave(bond, flow, vlan);
+    struct bond_slave *slave = choose_output_slave(bond, flow, wc, 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;
@@ -643,7 +653,8 @@ bond_choose_output_slave(struct bond *bond, const struct flow *flow,
 static bool
 bond_is_balanced(const struct bond *bond)
 {
-    return bond->balance == BM_SLB || bond->balance == BM_TCP;
+    return bond->rebalance_interval
+        && (bond->balance == BM_SLB || bond->balance == BM_TCP);
 }
 
 /* Notifies 'bond' that 'n_bytes' bytes were sent in 'flow' within 'vlan'. */
@@ -725,7 +736,8 @@ bond_shift_load(struct bond_entry *hash, struct bond_slave *to,
     hash->tag = tag_create_random();
 }
 
-/* Pick and returns a bond_entry to migrate to 'to' (the least-loaded slave),
+/* Picks and returns a bond_entry to migrate from 'from' (the most heavily
+ * loaded bond slave) to a bond slave that has 'to_tx_bytes' bytes of load,
  * given that doing so must decrease the ratio of the load on the two slaves by
  * at least 0.1.  Returns NULL if there is no appropriate entry.
  *
@@ -754,8 +766,12 @@ choose_entry_to_migrate(const struct bond_slave *from, uint64_t to_tx_bytes)
         delta = e->tx_bytes;
         old_ratio = (double)from->tx_bytes / to_tx_bytes;
         new_ratio = (double)(from->tx_bytes - delta) / (to_tx_bytes + delta);
-        if (old_ratio - new_ratio > 0.1) {
-            /* Would decrease the ratio, move it. */
+        if (old_ratio - new_ratio > 0.1
+            && fabs(new_ratio - 1.0) < fabs(old_ratio - 1.0)) {
+            /* We're aiming for an ideal ratio of 1, meaning both the 'from'
+               and 'to' slave have the same load.  Therefore, we only move an
+               entry if it decreases the load on 'from', and brings us closer
+               to equal traffic load. */
             return e;
         }
     }
@@ -842,8 +858,8 @@ bond_rebalance(struct bond *bond, struct tag_set *tags)
             break;
         }
 
-        /* 'from' is carrying significantly more load than 'to', and that load
-         * is split across at least two different hashes. */
+        /* 'from' is carrying significantly more load than 'to'.  Pick a hash
+         * to move from 'from' to 'to'. */
         e = choose_entry_to_migrate(from, to->tx_bytes);
         if (e) {
             bond_shift_load(e, to, tags);
@@ -931,7 +947,7 @@ bond_unixctl_list(struct unixctl_conn *conn,
         }
         ds_put_char(&ds, '\n');
     }
-    unixctl_command_reply(conn, 200, ds_cstr(&ds));
+    unixctl_command_reply(conn, ds_cstr(&ds));
     ds_destroy(&ds);
 }
 
@@ -947,11 +963,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);
@@ -962,8 +973,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);
@@ -1024,7 +1048,7 @@ bond_unixctl_show(struct unixctl_conn *conn,
         const struct bond *bond = bond_find(argv[1]);
 
         if (!bond) {
-            unixctl_command_reply(conn, 501, "no such bond");
+            unixctl_command_reply_error(conn, "no such bond");
             return;
         }
         bond_print_details(&ds, bond);
@@ -1036,7 +1060,7 @@ bond_unixctl_show(struct unixctl_conn *conn,
         }
     }
 
-    unixctl_command_reply(conn, 200, ds_cstr(&ds));
+    unixctl_command_reply(conn, ds_cstr(&ds));
     ds_destroy(&ds);
 }
 
@@ -1055,30 +1079,30 @@ bond_unixctl_migrate(struct unixctl_conn *conn,
 
     bond = bond_find(bond_s);
     if (!bond) {
-        unixctl_command_reply(conn, 501, "no such bond");
+        unixctl_command_reply_error(conn, "no such bond");
         return;
     }
 
     if (bond->balance != BM_SLB) {
-        unixctl_command_reply(conn, 501, "not an SLB bond");
+        unixctl_command_reply_error(conn, "not an SLB bond");
         return;
     }
 
     if (strspn(hash_s, "0123456789") == strlen(hash_s)) {
         hash = atoi(hash_s) & BOND_MASK;
     } else {
-        unixctl_command_reply(conn, 501, "bad hash");
+        unixctl_command_reply_error(conn, "bad hash");
         return;
     }
 
     slave = bond_lookup_slave(bond, slave_s);
     if (!slave) {
-        unixctl_command_reply(conn, 501, "no such slave");
+        unixctl_command_reply_error(conn, "no such slave");
         return;
     }
 
     if (!slave->enabled) {
-        unixctl_command_reply(conn, 501, "cannot migrate to disabled slave");
+        unixctl_command_reply_error(conn, "cannot migrate to disabled slave");
         return;
     }
 
@@ -1086,7 +1110,7 @@ bond_unixctl_migrate(struct unixctl_conn *conn,
     tag_set_add(&bond->unixctl_tags, entry->tag);
     entry->slave = slave;
     entry->tag = tag_create_random();
-    unixctl_command_reply(conn, 200, "migrated");
+    unixctl_command_reply(conn, "migrated");
 }
 
 static void
@@ -1101,18 +1125,18 @@ bond_unixctl_set_active_slave(struct unixctl_conn *conn,
 
     bond = bond_find(bond_s);
     if (!bond) {
-        unixctl_command_reply(conn, 501, "no such bond");
+        unixctl_command_reply_error(conn, "no such bond");
         return;
     }
 
     slave = bond_lookup_slave(bond, slave_s);
     if (!slave) {
-        unixctl_command_reply(conn, 501, "no such slave");
+        unixctl_command_reply_error(conn, "no such slave");
         return;
     }
 
     if (!slave->enabled) {
-        unixctl_command_reply(conn, 501, "cannot make disabled slave active");
+        unixctl_command_reply_error(conn, "cannot make disabled slave active");
         return;
     }
 
@@ -1123,9 +1147,9 @@ bond_unixctl_set_active_slave(struct unixctl_conn *conn,
         VLOG_INFO("bond %s: active interface is now %s",
                   bond->name, slave->name);
         bond->send_learning_packets = true;
-        unixctl_command_reply(conn, 200, "done");
+        unixctl_command_reply(conn, "done");
     } else {
-        unixctl_command_reply(conn, 200, "no change");
+        unixctl_command_reply(conn, "no change");
     }
 }
 
@@ -1139,18 +1163,18 @@ enable_slave(struct unixctl_conn *conn, const char *argv[], bool enable)
 
     bond = bond_find(bond_s);
     if (!bond) {
-        unixctl_command_reply(conn, 501, "no such bond");
+        unixctl_command_reply_error(conn, "no such bond");
         return;
     }
 
     slave = bond_lookup_slave(bond, slave_s);
     if (!slave) {
-        unixctl_command_reply(conn, 501, "no such slave");
+        unixctl_command_reply_error(conn, "no such slave");
         return;
     }
 
     bond_enable_slave(slave, enable, &bond->unixctl_tags);
-    unixctl_command_reply(conn, 501, enable ? "enabled" : "disabled");
+    unixctl_command_reply(conn, enable ? "enabled" : "disabled");
 }
 
 static void
@@ -1184,7 +1208,7 @@ bond_unixctl_hash(struct unixctl_conn *conn, int argc, const char *argv[],
 
     if (vlan_s) {
         if (sscanf(vlan_s, "%u", &vlan) != 1) {
-            unixctl_command_reply(conn, 501, "invalid vlan");
+            unixctl_command_reply_error(conn, "invalid vlan");
             return;
         }
     } else {
@@ -1193,7 +1217,7 @@ bond_unixctl_hash(struct unixctl_conn *conn, int argc, const char *argv[],
 
     if (basis_s) {
         if (sscanf(basis_s, "%"PRIu32, &basis) != 1) {
-            unixctl_command_reply(conn, 501, "invalid basis");
+            unixctl_command_reply_error(conn, "invalid basis");
             return;
         }
     } else {
@@ -1205,10 +1229,10 @@ bond_unixctl_hash(struct unixctl_conn *conn, int argc, const char *argv[],
         hash = bond_hash_src(mac, vlan, basis) & BOND_MASK;
 
         hash_cstr = xasprintf("%u", hash);
-        unixctl_command_reply(conn, 200, hash_cstr);
+        unixctl_command_reply(conn, hash_cstr);
         free(hash_cstr);
     } else {
-        unixctl_command_reply(conn, 501, "invalid mac");
+        unixctl_command_reply_error(conn, "invalid mac");
     }
 }
 
@@ -1266,23 +1290,18 @@ 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;
         if (!slave->enabled) {
-            VLOG_WARN("interface %s: disabled", slave->name);
+            VLOG_INFO("interface %s: disabled", slave->name);
             if (tags) {
                 tag_set_add(tags, slave->tag);
             }
         } else {
-            VLOG_WARN("interface %s: enabled", slave->name);
+            VLOG_INFO("interface %s: enabled", slave->name);
             slave->tag = tag_create_random();
         }
-
-        if (bond->balance == BM_STABLE) {
-            bond->bond_revalidate = true;
-        }
     }
 }
 
@@ -1302,7 +1321,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) {
@@ -1321,13 +1340,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)
 {
@@ -1349,9 +1361,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);
+    ovs_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));
 }
@@ -1363,51 +1375,35 @@ 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, const struct flow *flow,
-                 uint16_t vlan)
-{
-    struct bond_slave *best, *slave;
-    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);
-            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)
+                    struct flow_wildcards *wc, uint16_t vlan, tag_type *tags)
 {
     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;
+        }
+        if (wc) {
+            flow_mask_hash_fields(flow, wc, NX_HASH_FIELDS_SYMMETRIC_L4);
+        }
+        /* Fall Through. */
+    case BM_SLB:
+        if (wc) {
+            flow_mask_hash_fields(flow, wc, NX_HASH_FIELDS_ETH_SRC);
+        }
         e = lookup_bond_entry(bond, flow, vlan);
         if (!e->slave || !e->slave->enabled) {
             e->slave = CONTAINER_OF(hmap_random_node(&bond->slaves),
@@ -1417,6 +1413,7 @@ choose_output_slave(const struct bond *bond, const struct flow *flow,
             }
             e->tag = tag_create_random();
         }
+        *tags |= e->tag;
         return e->slave;
 
     default:
@@ -1474,7 +1471,7 @@ bond_choose_active_slave(struct bond *bond, struct tag_set *tags)
 
         bond->send_learning_packets = true;
     } else if (old_active_slave) {
-        VLOG_WARN_RL(&rl, "bond %s: all interfaces disabled", bond->name);
+        VLOG_INFO_RL(&rl, "bond %s: all interfaces disabled", bond->name);
     }
 }