netdev-dpdk: fix mbuf leaks
[cascardo/ovs.git] / lib / lacp.c
index 2b77996..c5d0fb1 100644 (file)
@@ -1,4 +1,4 @@
-/* Copyright (c) 2011 Nicira Networks
+/* Copyright (c) 2011, 2012, 2013, 2014, 2015 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 <config.h>
 #include "lacp.h"
 
-#include <assert.h>
 #include <stdlib.h>
 
+#include "connectivity.h"
 #include "dynamic-string.h"
 #include "hash.h"
 #include "hmap.h"
-#include "ofpbuf.h"
+#include "dp-packet.h"
+#include "ovs-atomic.h"
 #include "packets.h"
 #include "poll-loop.h"
+#include "seq.h"
 #include "shash.h"
 #include "timer.h"
 #include "timeval.h"
 #include "unixctl.h"
-#include "vlog.h"
+#include "openvswitch/vlog.h"
 
 VLOG_DEFINE_THIS_MODULE(lacp);
 
@@ -48,14 +50,15 @@ VLOG_DEFINE_THIS_MODULE(lacp);
 #define LACP_RX_MULTIPLIER 3    /* Multiply by TX rate to get RX rate. */
 
 #define LACP_INFO_LEN 15
+OVS_PACKED(
 struct lacp_info {
     ovs_be16 sys_priority;            /* System priority. */
-    uint8_t sys_id[ETH_ADDR_LEN];     /* System ID. */
+    struct eth_addr sys_id;           /* System ID. */
     ovs_be16 key;                     /* Operational key. */
     ovs_be16 port_priority;           /* Port priority. */
     ovs_be16 port_id;                 /* Port ID. */
     uint8_t state;                    /* State mask.  See LACP_STATE macros. */
-} __attribute__((packed));
+});
 BUILD_ASSERT_DECL(LACP_INFO_LEN == sizeof(struct lacp_info));
 
 #define LACP_PDU_LEN 110
@@ -77,7 +80,7 @@ struct lacp_pdu {
     uint8_t collector_len;    /* Always 16. */
     ovs_be16 collector_delay; /* Maximum collector delay. Set to UINT16_MAX. */
     uint8_t z3[64];           /* Combination of several fields.  Always 0. */
-} __attribute__((packed));
+};
 BUILD_ASSERT_DECL(LACP_PDU_LEN == sizeof(struct lacp_pdu));
 \f
 /* Implementation. */
@@ -89,20 +92,21 @@ enum slave_status {
 };
 
 struct lacp {
-    struct list node;             /* Node in all_lacps list. */
+    struct ovs_list node;         /* Node in all_lacps list. */
     char *name;                   /* Name of this lacp object. */
-    uint8_t sys_id[ETH_ADDR_LEN]; /* System ID. */
+    struct eth_addr sys_id;       /* System ID. */
     uint16_t sys_priority;        /* System Priority. */
     bool active;                  /* Active or Passive. */
 
     struct hmap slaves;      /* Slaves this LACP object controls. */
     struct slave *key_slave; /* Slave whose ID will be the aggregation key. */
 
-    enum lacp_time lacp_time;  /* Fast, Slow or Custom LACP time. */
-    long long int custom_time; /* LACP_TIME_CUSTOM transmission rate. */
+    bool fast;               /* True if using fast probe interval. */
     bool negotiated;         /* True if LACP negotiations were successful. */
     bool update;             /* True if lacp_update() needs to be called. */
-    bool heartbeat;          /* LACP heartbeat mode. */
+    bool fallback_ab; /* True if fallback to active-backup on LACP failure. */
+
+    struct ovs_refcount ref_cnt;
 };
 
 struct slave {
@@ -121,23 +125,33 @@ struct slave {
     struct lacp_info ntt_actor;   /* Used to decide if we Need To Transmit. */
     struct timer tx;              /* Next message transmission timer. */
     struct timer rx;              /* Expected message receive timer. */
+
+    uint32_t count_rx_pdus;       /* dot3adAggPortStatsLACPDUsRx */
+    uint32_t count_rx_pdus_bad;   /* dot3adAggPortStatsIllegalRx */
+    uint32_t count_tx_pdus;       /* dot3adAggPortStatsLACPDUsTx */
 };
 
-static struct list all_lacps = LIST_INITIALIZER(&all_lacps);
+static struct ovs_mutex mutex;
+static struct ovs_list all_lacps__ = OVS_LIST_INITIALIZER(&all_lacps__);
+static struct ovs_list *const all_lacps OVS_GUARDED_BY(mutex) = &all_lacps__;
 
-static void lacp_update_attached(struct lacp *);
+static void lacp_update_attached(struct lacp *) OVS_REQUIRES(mutex);
 
-static void slave_destroy(struct slave *);
-static void slave_set_defaulted(struct slave *);
-static void slave_set_expired(struct slave *);
-static void slave_get_actor(struct slave *, struct lacp_info *actor);
-static void slave_get_priority(struct slave *, struct lacp_info *priority);
-static bool slave_may_tx(const struct slave *);
-static struct slave *slave_lookup(const struct lacp *, const void *slave);
-static bool info_tx_equal(struct lacp_info *, struct lacp_info *);
+static void slave_destroy(struct slave *) OVS_REQUIRES(mutex);
+static void slave_set_defaulted(struct slave *) OVS_REQUIRES(mutex);
+static void slave_set_expired(struct slave *) OVS_REQUIRES(mutex);
+static void slave_get_actor(struct slave *, struct lacp_info *actor)
+    OVS_REQUIRES(mutex);
+static void slave_get_priority(struct slave *, struct lacp_info *priority)
+    OVS_REQUIRES(mutex);
+static bool slave_may_tx(const struct slave *)
+    OVS_REQUIRES(mutex);
+static struct slave *slave_lookup(const struct lacp *, const void *slave)
+    OVS_REQUIRES(mutex);
+static bool info_tx_equal(struct lacp_info *, struct lacp_info *)
+    OVS_REQUIRES(mutex);
 
-static void lacp_unixctl_show(struct unixctl_conn *, const char *args,
-                              void *aux);
+static unixctl_cb_func lacp_unixctl_show;
 
 /* Populates 'pdu' with a LACP PDU comprised of 'actor' and 'partner'. */
 static void
@@ -167,11 +181,12 @@ compose_lacp_pdu(const struct lacp_info *actor,
  * supported by OVS.  Otherwise, it returns a pointer to the lacp_pdu contained
  * within 'b'. */
 static const struct lacp_pdu *
-parse_lacp_packet(const struct ofpbuf *b)
+parse_lacp_packet(const struct dp_packet *p)
 {
     const struct lacp_pdu *pdu;
 
-    pdu = ofpbuf_at(b, (uint8_t *)b->l3 - (uint8_t *)b->data, LACP_PDU_LEN);
+    pdu = dp_packet_at(p, (uint8_t *)dp_packet_l3(p) - (uint8_t *)dp_packet_data(p),
+                    LACP_PDU_LEN);
 
     if (pdu && pdu->subtype == 1
         && pdu->actor_type == 1 && pdu->actor_len == 20
@@ -188,28 +203,62 @@ parse_lacp_packet(const struct ofpbuf *b)
 void
 lacp_init(void)
 {
-    unixctl_command_register("lacp/show", "[port]", lacp_unixctl_show, NULL);
+    unixctl_command_register("lacp/show", "[port]", 0, 1,
+                             lacp_unixctl_show, NULL);
+}
+
+static void
+lacp_lock(void) OVS_ACQUIRES(mutex)
+{
+    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+
+    if (ovsthread_once_start(&once)) {
+        ovs_mutex_init_recursive(&mutex);
+        ovsthread_once_done(&once);
+    }
+    ovs_mutex_lock(&mutex);
+}
+
+static void
+lacp_unlock(void) OVS_RELEASES(mutex)
+{
+    ovs_mutex_unlock(&mutex);
 }
 
 /* Creates a LACP object. */
 struct lacp *
-lacp_create(void)
+lacp_create(void) OVS_EXCLUDED(mutex)
 {
     struct lacp *lacp;
 
     lacp = xzalloc(sizeof *lacp);
     hmap_init(&lacp->slaves);
-    list_push_back(&all_lacps, &lacp->node);
+    ovs_refcount_init(&lacp->ref_cnt);
+
+    lacp_lock();
+    list_push_back(all_lacps, &lacp->node);
+    lacp_unlock();
+    return lacp;
+}
+
+struct lacp *
+lacp_ref(const struct lacp *lacp_)
+{
+    struct lacp *lacp = CONST_CAST(struct lacp *, lacp_);
+    if (lacp) {
+        ovs_refcount_ref(&lacp->ref_cnt);
+    }
     return lacp;
 }
 
 /* Destroys 'lacp' and its slaves. Does nothing if 'lacp' is NULL. */
 void
-lacp_destroy(struct lacp *lacp)
+lacp_unref(struct lacp *lacp) OVS_EXCLUDED(mutex)
 {
-    if (lacp) {
+    if (lacp && ovs_refcount_unref_relaxed(&lacp->ref_cnt) == 1) {
         struct slave *slave, *next;
 
+        lacp_lock();
         HMAP_FOR_EACH_SAFE (slave, next, node, &lacp->slaves) {
             slave_destroy(slave);
         }
@@ -218,40 +267,51 @@ lacp_destroy(struct lacp *lacp)
         list_remove(&lacp->node);
         free(lacp->name);
         free(lacp);
+        lacp_unlock();
     }
 }
 
 /* Configures 'lacp' with settings from 's'. */
 void
 lacp_configure(struct lacp *lacp, const struct lacp_settings *s)
+    OVS_EXCLUDED(mutex)
 {
-    assert(!eth_addr_is_zero(s->id));
+    ovs_assert(!eth_addr_is_zero(s->id));
 
+    lacp_lock();
     if (!lacp->name || strcmp(s->name, lacp->name)) {
         free(lacp->name);
         lacp->name = xstrdup(s->name);
     }
 
     if (!eth_addr_equals(lacp->sys_id, s->id)
-        || lacp->sys_priority != s->priority
-        || lacp->heartbeat != s->heartbeat) {
-        memcpy(lacp->sys_id, s->id, ETH_ADDR_LEN);
+        || lacp->sys_priority != s->priority) {
+        lacp->sys_id = s->id;
         lacp->sys_priority = s->priority;
-        lacp->heartbeat = s->heartbeat;
         lacp->update = true;
     }
 
     lacp->active = s->active;
-    lacp->lacp_time = s->lacp_time;
-    lacp->custom_time = MAX(TIME_UPDATE_INTERVAL, s->custom_time);
+    lacp->fast = s->fast;
+
+    if (lacp->fallback_ab != s->fallback_ab_cfg) {
+        lacp->fallback_ab = s->fallback_ab_cfg;
+        lacp->update = true;
+    }
+
+    lacp_unlock();
 }
 
 /* Returns true if 'lacp' is configured in active mode, false if 'lacp' is
  * configured for passive mode. */
 bool
-lacp_is_active(const struct lacp *lacp)
+lacp_is_active(const struct lacp *lacp) OVS_EXCLUDED(mutex)
 {
-    return lacp->active;
+    bool ret;
+    lacp_lock();
+    ret = lacp->active;
+    lacp_unlock();
+    return ret;
 }
 
 /* Processes 'packet' which was received on 'slave_'.  This function should be
@@ -259,33 +319,30 @@ lacp_is_active(const struct lacp *lacp)
  */
 void
 lacp_process_packet(struct lacp *lacp, const void *slave_,
-                    const struct ofpbuf *packet)
+                    const struct dp_packet *packet)
+    OVS_EXCLUDED(mutex)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-    struct slave *slave = slave_lookup(lacp, slave_);
     const struct lacp_pdu *pdu;
     long long int tx_rate;
+    struct slave *slave;
+
+    lacp_lock();
+    slave = slave_lookup(lacp, slave_);
+    if (!slave) {
+        goto out;
+    }
+    slave->count_rx_pdus++;
 
     pdu = parse_lacp_packet(packet);
     if (!pdu) {
+       slave->count_rx_pdus_bad++;
         VLOG_WARN_RL(&rl, "%s: received an unparsable LACP PDU.", lacp->name);
-        return;
-    }
-
-    switch (lacp->lacp_time) {
-    case LACP_TIME_FAST:
-        tx_rate = LACP_FAST_TIME_TX;
-        break;
-    case LACP_TIME_SLOW:
-        tx_rate = LACP_SLOW_TIME_TX;
-        break;
-    case LACP_TIME_CUSTOM:
-        tx_rate = lacp->custom_time;
-        break;
-    default: NOT_REACHED();
+        goto out;
     }
 
     slave->status = LACP_CURRENT;
+    tx_rate = lacp->fast ? LACP_FAST_TIME_TX : LACP_SLOW_TIME_TX;
     timer_set_duration(&slave->rx, LACP_RX_MULTIPLIER * tx_rate);
 
     slave->ntt_actor = pdu->partner;
@@ -297,14 +354,27 @@ lacp_process_packet(struct lacp *lacp, const void *slave_,
         lacp->update = true;
         slave->partner = pdu->actor;
     }
+
+out:
+    lacp_unlock();
 }
 
-/* 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) OVS_EXCLUDED(mutex)
 {
-    return lacp ? lacp->negotiated : false;
+    if (lacp) {
+        enum lacp_status ret;
+
+        lacp_lock();
+        ret = lacp->negotiated ? LACP_NEGOTIATED : LACP_CONFIGURED;
+        lacp_unlock();
+        return ret;
+    } else {
+        /* Don't take 'mutex'.  It might not even be initialized, since we
+         * don't know that any lacp object has been created. */
+        return LACP_DISABLED;
+    }
 }
 
 /* Registers 'slave_' as subordinate to 'lacp'.  This should be called at least
@@ -313,9 +383,12 @@ lacp_negotiated(const struct lacp *lacp)
 void
 lacp_slave_register(struct lacp *lacp, void *slave_,
                     const struct lacp_slave_settings *s)
+    OVS_EXCLUDED(mutex)
 {
-    struct slave *slave = slave_lookup(lacp, slave_);
+    struct slave *slave;
 
+    lacp_lock();
+    slave = slave_lookup(lacp, slave_);
     if (!slave) {
         slave = xzalloc(sizeof *slave);
         slave->lacp = lacp;
@@ -346,30 +419,58 @@ lacp_slave_register(struct lacp *lacp, void *slave_,
             slave_set_expired(slave);
         }
     }
+    lacp_unlock();
 }
 
 /* Unregisters 'slave_' with 'lacp'.  */
 void
 lacp_slave_unregister(struct lacp *lacp, const void *slave_)
+    OVS_EXCLUDED(mutex)
 {
-    struct slave *slave = slave_lookup(lacp, slave_);
+    struct slave *slave;
 
+    lacp_lock();
+    slave = slave_lookup(lacp, slave_);
     if (slave) {
         slave_destroy(slave);
         lacp->update = true;
     }
+    lacp_unlock();
 }
 
 /* This function should be called whenever the carrier status of 'slave_' has
- * changed. */
+ * changed.  If 'lacp' is null, this function has no effect.*/
 void
 lacp_slave_carrier_changed(const struct lacp *lacp, const void *slave_)
+    OVS_EXCLUDED(mutex)
 {
-    struct slave *slave = slave_lookup(lacp, slave_);
+    struct slave *slave;
+    if (!lacp) {
+        return;
+    }
+
+    lacp_lock();
+    slave = slave_lookup(lacp, slave_);
+    if (!slave) {
+        goto out;
+    }
 
     if (slave->status == LACP_CURRENT || slave->lacp->active) {
         slave_set_expired(slave);
     }
+
+out:
+    lacp_unlock();
+}
+
+static bool
+slave_may_enable__(struct slave *slave) OVS_REQUIRES(mutex)
+{
+    /* The slave may be enabled if it's attached to an aggregator and its
+     * partner is synchronized.*/
+    return slave->attached && (slave->partner.state & LACP_STATE_SYNC
+            || (slave->lacp && slave->lacp->fallback_ab
+                && slave->status == LACP_DEFAULTED));
 }
 
 /* This function should be called before enabling 'slave_' to send or receive
@@ -377,52 +478,58 @@ lacp_slave_carrier_changed(const struct lacp *lacp, const void *slave_)
  * convenience, returns true if 'lacp' is NULL. */
 bool
 lacp_slave_may_enable(const struct lacp *lacp, const void *slave_)
+    OVS_EXCLUDED(mutex)
 {
     if (lacp) {
-        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);
+        struct slave *slave;
+        bool ret;
+
+        lacp_lock();
+        slave = slave_lookup(lacp, slave_);
+        ret = slave ? slave_may_enable__(slave) : false;
+        lacp_unlock();
+        return ret;
     } else {
         return true;
     }
 }
 
-/* Returns the port ID used for 'slave_' in LACP communications. */
-uint16_t
-lacp_slave_get_port_id(const struct lacp *lacp, const void *slave_)
-{
-    struct slave *slave = slave_lookup(lacp, slave_);
-    return slave->port_id;
-}
-
 /* Returns true if partner information on 'slave_' is up to date.  'slave_'
  * not being current, generally indicates a connectivity problem, or a
  * misconfigured (or broken) partner. */
 bool
 lacp_slave_is_current(const struct lacp *lacp, const void *slave_)
+    OVS_EXCLUDED(mutex)
 {
-    return slave_lookup(lacp, slave_)->status != LACP_DEFAULTED;
+    struct slave *slave;
+    bool ret;
+
+    lacp_lock();
+    slave = slave_lookup(lacp, slave_);
+    ret = slave ? slave->status != LACP_DEFAULTED : false;
+    lacp_unlock();
+    return ret;
 }
 
 /* This function should be called periodically to update 'lacp'. */
 void
-lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu)
+lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex)
 {
     struct slave *slave;
 
+    lacp_lock();
     HMAP_FOR_EACH (slave, node, &lacp->slaves) {
         if (timer_expired(&slave->rx)) {
+            enum slave_status old_status = slave->status;
+
             if (slave->status == LACP_CURRENT) {
                 slave_set_expired(slave);
             } else if (slave->status == LACP_EXPIRED) {
                 slave_set_defaulted(slave);
             }
+            if (slave->status != old_status) {
+                seq_change(connectivity_seq_get());
+            }
         }
     }
 
@@ -447,26 +554,26 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu)
             slave->ntt_actor = actor;
             compose_lacp_pdu(&actor, &slave->partner, &pdu);
             send_pdu(slave->aux, &pdu, sizeof pdu);
+           slave->count_tx_pdus++;
 
-            if (lacp->lacp_time == LACP_TIME_CUSTOM) {
-                duration = lacp->custom_time;
-            } else {
-                duration = (slave->partner.state & LACP_STATE_TIME
-                            ? LACP_FAST_TIME_TX
-                            : LACP_SLOW_TIME_TX);
-            }
+            duration = (slave->partner.state & LACP_STATE_TIME
+                        ? LACP_FAST_TIME_TX
+                        : LACP_SLOW_TIME_TX);
 
             timer_set_duration(&slave->tx, duration);
+            seq_change(connectivity_seq_get());
         }
     }
+    lacp_unlock();
 }
 
 /* Causes poll_block() to wake up when lacp_run() needs to be called again. */
 void
-lacp_wait(struct lacp *lacp)
+lacp_wait(struct lacp *lacp) OVS_EXCLUDED(mutex)
 {
     struct slave *slave;
 
+    lacp_lock();
     HMAP_FOR_EACH (slave, node, &lacp->slaves) {
         if (slave_may_tx(slave)) {
             timer_wait(&slave->tx);
@@ -476,6 +583,7 @@ lacp_wait(struct lacp *lacp)
             timer_wait(&slave->rx);
         }
     }
+    lacp_unlock();
 }
 \f
 /* Static Helpers. */
@@ -483,40 +591,36 @@ lacp_wait(struct lacp *lacp)
 /* Updates the attached status of all slaves controlled by 'lacp' and sets its
  * negotiated parameter to true if any slaves are attachable. */
 static void
-lacp_update_attached(struct lacp *lacp)
+lacp_update_attached(struct lacp *lacp) OVS_REQUIRES(mutex)
 {
     struct slave *lead, *slave;
     struct lacp_info lead_pri;
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 10);
 
-    if (lacp->heartbeat) {
-        HMAP_FOR_EACH (slave, node, &lacp->slaves) {
-            slave->attached = slave->status != LACP_DEFAULTED;
-        }
-        return;
-    }
-
     lacp->update = false;
 
     lead = NULL;
     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;
         }
 
         if (slave->status == LACP_DEFAULTED) {
+            if (lacp->fallback_ab) {
+                slave->attached = true;
+            }
             continue;
         }
 
+        slave->attached = true;
         slave_get_priority(slave, &pri);
 
         if (!lead || memcmp(&pri, &lead_pri, sizeof pri) < 0) {
@@ -529,7 +633,7 @@ lacp_update_attached(struct lacp *lacp)
 
     if (lead) {
         HMAP_FOR_EACH (slave, node, &lacp->slaves) {
-            if (slave->status == LACP_DEFAULTED
+            if ((lacp->fallback_ab && slave->status == LACP_DEFAULTED)
                 || lead->partner.key != slave->partner.key
                 || !eth_addr_equals(lead->partner.sys_id,
                                     slave->partner.sys_id)) {
@@ -540,7 +644,7 @@ lacp_update_attached(struct lacp *lacp)
 }
 
 static void
-slave_destroy(struct slave *slave)
+slave_destroy(struct slave *slave) OVS_REQUIRES(mutex)
 {
     if (slave) {
         struct lacp *lacp = slave->lacp;
@@ -564,7 +668,7 @@ slave_destroy(struct slave *slave)
 }
 
 static void
-slave_set_defaulted(struct slave *slave)
+slave_set_defaulted(struct slave *slave) OVS_REQUIRES(mutex)
 {
     memset(&slave->partner, 0, sizeof slave->partner);
 
@@ -573,24 +677,18 @@ slave_set_defaulted(struct slave *slave)
 }
 
 static void
-slave_set_expired(struct slave *slave)
+slave_set_expired(struct slave *slave) OVS_REQUIRES(mutex)
 {
-    struct lacp *lacp = slave->lacp;
-
     slave->status = LACP_EXPIRED;
     slave->partner.state |= LACP_STATE_TIME;
     slave->partner.state &= ~LACP_STATE_SYNC;
 
-    /* The spec says we should wait LACP_RX_MULTIPLIER * LACP_FAST_TIME_TX.
-     * This doesn't make sense when using custom times which can be much
-     * smaller than LACP_FAST_TIME. */
-    timer_set_duration(&slave->rx, (lacp->lacp_time == LACP_TIME_CUSTOM
-                                    ? lacp->custom_time
-                                    : LACP_RX_MULTIPLIER * LACP_FAST_TIME_TX));
+    timer_set_duration(&slave->rx, LACP_RX_MULTIPLIER * LACP_FAST_TIME_TX);
 }
 
 static void
 slave_get_actor(struct slave *slave, struct lacp_info *actor)
+    OVS_REQUIRES(mutex)
 {
     struct lacp *lacp = slave->lacp;
     uint16_t key;
@@ -600,7 +698,7 @@ slave_get_actor(struct slave *slave, struct lacp_info *actor)
         state |= LACP_STATE_ACT;
     }
 
-    if (lacp->lacp_time != LACP_TIME_SLOW) {
+    if (lacp->fast) {
         state |= LACP_STATE_TIME;
     }
 
@@ -616,7 +714,7 @@ slave_get_actor(struct slave *slave, struct lacp_info *actor)
         state |= LACP_STATE_EXP;
     }
 
-    if (lacp->heartbeat || hmap_count(&lacp->slaves) > 1) {
+    if (hmap_count(&lacp->slaves) > 1) {
         state |= LACP_STATE_AGG;
     }
 
@@ -634,7 +732,7 @@ slave_get_actor(struct slave *slave, struct lacp_info *actor)
     actor->port_priority = htons(slave->port_priority);
     actor->port_id = htons(slave->port_id);
     actor->sys_priority = htons(lacp->sys_priority);
-    memcpy(&actor->sys_id, lacp->sys_id, ETH_ADDR_LEN);
+    actor->sys_id = lacp->sys_id;
 }
 
 /* Given 'slave', populates 'priority' with data representing its LACP link
@@ -643,6 +741,7 @@ slave_get_actor(struct slave *slave, struct lacp_info *actor)
  * link. */
 static void
 slave_get_priority(struct slave *slave, struct lacp_info *priority)
+    OVS_REQUIRES(mutex)
 {
     uint16_t partner_priority, actor_priority;
 
@@ -667,13 +766,13 @@ slave_get_priority(struct slave *slave, struct lacp_info *priority)
 }
 
 static bool
-slave_may_tx(const struct slave *slave)
+slave_may_tx(const struct slave *slave) OVS_REQUIRES(mutex)
 {
     return slave->lacp->active || slave->status != LACP_DEFAULTED;
 }
 
 static struct slave *
-slave_lookup(const struct lacp *lacp, const void *slave_)
+slave_lookup(const struct lacp *lacp, const void *slave_) OVS_REQUIRES(mutex)
 {
     struct slave *slave;
 
@@ -710,11 +809,11 @@ info_tx_equal(struct lacp_info *a, struct lacp_info *b)
 }
 \f
 static struct lacp *
-lacp_find(const char *name)
+lacp_find(const char *name) OVS_REQUIRES(mutex)
 {
     struct lacp *lacp;
 
-    LIST_FOR_EACH (lacp, node, &all_lacps) {
+    LIST_FOR_EACH (lacp, node, all_lacps) {
         if (!strcmp(lacp->name, name)) {
             return lacp;
         }
@@ -760,7 +859,7 @@ ds_put_lacp_state(struct ds *ds, uint8_t state)
 }
 
 static void
-lacp_print_details(struct ds *ds, struct lacp *lacp)
+lacp_print_details(struct ds *ds, struct lacp *lacp) OVS_REQUIRES(mutex)
 {
     struct shash slave_shash = SHASH_INITIALIZER(&slave_shash);
     const struct shash_node **sorted_slaves = NULL;
@@ -770,9 +869,6 @@ lacp_print_details(struct ds *ds, struct lacp *lacp)
 
     ds_put_format(ds, "---- %s ----\n", lacp->name);
     ds_put_format(ds, "\tstatus: %s", lacp->active ? "active" : "passive");
-    if (lacp->heartbeat) {
-        ds_put_cstr(ds, " heartbeat");
-    }
     if (lacp->negotiated) {
         ds_put_cstr(ds, " negotiated");
     }
@@ -782,25 +878,19 @@ lacp_print_details(struct ds *ds, struct lacp *lacp)
     ds_put_format(ds, "\tsys_priority: %u\n", lacp->sys_priority);
     ds_put_cstr(ds, "\taggregation key: ");
     if (lacp->key_slave) {
-        ds_put_format(ds, "%u", lacp->key_slave->port_id);
+        ds_put_format(ds, "%u", lacp->key_slave->key
+                                ? lacp->key_slave->key
+                                : lacp->key_slave->port_id);
     } else {
         ds_put_cstr(ds, "none");
     }
     ds_put_cstr(ds, "\n");
 
     ds_put_cstr(ds, "\tlacp_time: ");
-    switch (lacp->lacp_time) {
-    case LACP_TIME_FAST:
+    if (lacp->fast) {
         ds_put_cstr(ds, "fast\n");
-        break;
-    case LACP_TIME_SLOW:
+    } else {
         ds_put_cstr(ds, "slow\n");
-        break;
-    case LACP_TIME_CUSTOM:
-        ds_put_format(ds, "custom (%lld)\n", lacp->custom_time);
-        break;
-    default:
-        ds_put_cstr(ds, "unknown\n");
     }
 
     HMAP_FOR_EACH (slave, node, &lacp->slaves) {
@@ -825,13 +915,15 @@ lacp_print_details(struct ds *ds, struct lacp *lacp)
             status = "defaulted";
             break;
         default:
-            NOT_REACHED();
+            OVS_NOT_REACHED();
         }
 
         ds_put_format(ds, "\nslave: %s: %s %s\n", slave->name, status,
                       slave->attached ? "attached" : "detached");
         ds_put_format(ds, "\tport_id: %u\n", slave->port_id);
         ds_put_format(ds, "\tport_priority: %u\n", slave->port_priority);
+        ds_put_format(ds, "\tmay_enable: %s\n", (slave_may_enable__(slave)
+                                                 ? "true" : "false"));
 
         ds_put_format(ds, "\n\tactor sys_id: " ETH_ADDR_FMT "\n",
                       ETH_ADDR_ARGS(actor.sys_id));
@@ -867,25 +959,80 @@ lacp_print_details(struct ds *ds, struct lacp *lacp)
 }
 
 static void
-lacp_unixctl_show(struct unixctl_conn *conn,
-                  const char *args, void *aux OVS_UNUSED)
+lacp_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[],
+                  void *aux OVS_UNUSED) OVS_EXCLUDED(mutex)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
     struct lacp *lacp;
 
-    if (strlen(args)) {
-        lacp = lacp_find(args);
+    lacp_lock();
+    if (argc > 1) {
+        lacp = lacp_find(argv[1]);
         if (!lacp) {
-            unixctl_command_reply(conn, 501, "no such lacp object");
-            return;
+            unixctl_command_reply_error(conn, "no such lacp object");
+            goto out;
         }
         lacp_print_details(&ds, lacp);
     } else {
-        LIST_FOR_EACH (lacp, node, &all_lacps) {
+        LIST_FOR_EACH (lacp, node, all_lacps) {
             lacp_print_details(&ds, lacp);
         }
     }
 
-    unixctl_command_reply(conn, 200, ds_cstr(&ds));
+    unixctl_command_reply(conn, ds_cstr(&ds));
     ds_destroy(&ds);
+
+out:
+    lacp_unlock();
+}
+
+/* Extract a snapshot of the current state and counters for a slave port.
+   Return false if the slave is not active. */
+bool
+lacp_get_slave_stats(const struct lacp *lacp, const void *slave_, struct lacp_slave_stats *stats)
+    OVS_EXCLUDED(mutex)
+{
+    struct slave *slave;
+    struct lacp_info actor;
+    bool ret;
+
+    ovs_mutex_lock(&mutex);
+
+    slave = slave_lookup(lacp, slave_);
+    if (slave) {
+       ret = true;
+       slave_get_actor(slave, &actor);
+       stats->dot3adAggPortActorSystemID = actor.sys_id;
+       stats->dot3adAggPortPartnerOperSystemID = slave->partner.sys_id;
+       stats->dot3adAggPortAttachedAggID = (lacp->key_slave->key ?
+                                            lacp->key_slave->key :
+                                            lacp->key_slave->port_id);
+
+       /* Construct my admin-state.  Assume aggregation is configured on. */
+       stats->dot3adAggPortActorAdminState = LACP_STATE_AGG;
+       if (lacp->active) {
+           stats->dot3adAggPortActorAdminState |= LACP_STATE_ACT;
+       }
+       if (lacp->fast) {
+           stats->dot3adAggPortActorAdminState |= LACP_STATE_TIME;
+       }
+       /* XXX Not sure how to know the partner admin state. It
+        * might have to be captured and remembered during the
+        * negotiation phase.
+        */
+       stats->dot3adAggPortPartnerAdminState = 0;
+
+       stats->dot3adAggPortActorOperState = actor.state;
+       stats->dot3adAggPortPartnerOperState = slave->partner.state;
+
+       /* Read out the latest counters */
+       stats->dot3adAggPortStatsLACPDUsRx = slave->count_rx_pdus;
+       stats->dot3adAggPortStatsIllegalRx = slave->count_rx_pdus_bad;
+       stats->dot3adAggPortStatsLACPDUsTx = slave->count_tx_pdus;
+    } else {
+        ret = false;
+    }
+    ovs_mutex_unlock(&mutex);
+    return ret;
+
 }