12754 packet flow over a defaulted LACP port

Reviewed by: Dan McDonald <danmcd@joyent.com>
Reviewed by: Andrew Stormont <andyjstormont@gmail.com>
Reviewed by: Igor Kozhukhov <igor@dilos.org>
Approved by: Robert Mustacchi <rm@fingolfin.org>

2 files changed, 28 insertions, 18 deletions

diff --git a/usr/src/uts/common/io/aggr/aggr_grp.c b/usr/src/uts/common/io/aggr/aggr_grp.c
index 80cc3644ea..dc6f03b247 100644
--- a/usr/src/uts/common/io/aggr/aggr_grp.c
+++ b/usr/src/uts/common/io/aggr/aggr_grp.c
@@ -617,17 +617,22 @@ aggr_grp_add_port(aggr_grp_t *grp, datalink_id_t port_linkid, boolean_t force,
 }
 
 /*
- * This is called in response to either our LACP state machine or a MAC
- * notification that the link has gone down via aggr_send_port_disable(). At
- * this point, we may need to update our default ring. To that end, we go
- * through the set of ports (underlying datalinks in an aggregation) that are
- * currently enabled to transmit data. If all our links have been disabled for
- * transmit, then we don't do anything.
+ * This is called when the 'lg_tx_ports' arrangement has changed and
+ * we need to update the corresponding 'mi_default_tx_ring'. This
+ * happens for several reasons.
  *
- * Note, because we only have a single TX group, we don't have to worry about
- * the rings moving between groups and the chance that mac will reassign it
- * unless someone removes a port, at which point, we play it safe and call this
- * again.
+ *     - A pseudo TX mac group was added or removed.
+ *     - An LACP message has changed the port's state.
+ *     - A link event has changed the port's state.
+ *
+ * In any case, we see if there is at least one port enabled (see
+ * 'aggr_send_port_enable()'), and if so we use its first ring as the
+ * mac's default TX ring.
+ *
+ * Note, because we only have a single TX group, we don't have to
+ * worry about the rings moving between groups and the chance that mac
+ * will reassign it unless someone removes a port, at which point, we
+ * play it safe and call this again.
  */
 void
 aggr_grp_update_default(aggr_grp_t *grp)
diff --git a/usr/src/uts/common/io/mac/mac.c b/usr/src/uts/common/io/mac/mac.c
index 707fca24d0..41d3ee5fe1 100644
--- a/usr/src/uts/common/io/mac/mac.c
+++ b/usr/src/uts/common/io/mac/mac.c
@@ -1950,11 +1950,6 @@ mac_hwring_send_priv(mac_client_handle_t mch, mac_ring_handle_t rh, mblk_t *mp)
  * the ring will no longer exist. It's important to give aggr a case where the
  * rings can still exist such that it may be able to continue to send LACP PDUs
  * to potentially restore the link.
- *
- * Finally, we explicitly don't do anything if the ring hasn't been enabled yet.
- * This is to help out aggr which doesn't really know the internal state that
- * MAC does about the rings and can't know that it's not quite ready for use
- * yet.
  */
 void
 mac_hwring_set_default(mac_handle_t mh, mac_ring_handle_t rh)
@@ -1965,9 +1960,19 @@ mac_hwring_set_default(mac_handle_t mh, mac_ring_handle_t rh)
 	ASSERT(MAC_PERIM_HELD(mh));
 	VERIFY(mip->mi_state_flags & MIS_IS_AGGR);
 
-	if (ring->mr_state != MR_INUSE)
-		return;
-
+	/*
+	 * We used to condition this assignment on the ring's
+	 * 'mr_state' being one of 'MR_INUSE'. However, there are
+	 * cases where this is called before the ring has any active
+	 * clients, and therefore is not marked as in use. Since the
+	 * sole purpose of this function is for aggr to make sure
+	 * 'mi_default_tx_ring' matches 'lg_tx_ports[0]', its
+	 * imperative that we update its value regardless of ring
+	 * state. Otherwise, we can end up in a state where
+	 * 'mi_default_tx_ring' points to a pseudo ring of a downed
+	 * port, even when 'lg_tx_ports[0]' points to a port that is
+	 * up.
+	 */
 	mip->mi_default_tx_ring = rh;
 }