svn commit: r345655 - head/sys/net

[John Baldwin]

Author: jhb
Date: Thu Mar 28 20:25:36 2019
New Revision: 345655
URL: https://svnweb.freebsd.org/changeset/base/345655

Log:
  Remove nested epochs from lagg(4).
  
  lagg_bcast_start appeared to have a bug in that was using the last
  lagg port structure after exiting the epoch that was keeping that
  structure alive.  However, upon further inspection, the epoch was
  already entered by the caller (lagg_transmit), so the epoch enter/exit
  in lagg_bcast_start was actually unnecessary.
  
  This commit generally removes uses of the net epoch via LAGG_RLOCK to
  protect the list of ports when the list of ports was already protected
  by an existing LAGG_RLOCK in a caller, or the LAGG_XLOCK.
  
  It also adds a missing epoch enter/exit in lagg_snd_tag_alloc while
  accessing the lagg port structures.  An ifp is still accessed via an
  unsafe reference after the epoch is exited, but that is true in the
  current code and will be fixed in a future change.
  
  Reviewed by:	gallatin
  MFC after:	1 month
  Sponsored by:	Netflix
  Differential Revision:	https://reviews.freebsd.org/D19718

Modified:
  head/sys/net/if_lagg.c

Modified: head/sys/net/if_lagg.c
==============================================================================
--- head/sys/net/if_lagg.c	Thu Mar 28 20:17:14 2019	(r345654)
+++ head/sys/net/if_lagg.c	Thu Mar 28 20:25:36 2019	(r345655)
@@ -751,7 +751,6 @@ lagg_port_create(struct lagg_softc *sc, struct ifnet *
 	 * is predictable and `ifconfig laggN create ...` command
 	 * will lead to the same result each time.
 	 */
-	LAGG_RLOCK();
 	CK_SLIST_FOREACH(tlp, &sc->sc_ports, lp_entries) {
 		if (tlp->lp_ifp->if_index < ifp->if_index && (
 		    CK_SLIST_NEXT(tlp, lp_entries) == NULL ||
@@ -759,7 +758,6 @@ lagg_port_create(struct lagg_softc *sc, struct ifnet *
 		    ifp->if_index))
 			break;
 	}
-	LAGG_RUNLOCK();
 	if (tlp != NULL)
 		CK_SLIST_INSERT_AFTER(tlp, lp, lp_entries);
 	else
@@ -1537,14 +1535,17 @@ lagg_snd_tag_alloc(struct ifnet *ifp,
 	struct lagg_lb *lb;
 	uint32_t p;
 
+	LAGG_RLOCK();
 	switch (sc->sc_proto) {
 	case LAGG_PROTO_FAILOVER:
 		lp = lagg_link_active(sc, sc->sc_primary);
 		break;
 	case LAGG_PROTO_LOADBALANCE:
 		if ((sc->sc_opts & LAGG_OPT_USE_FLOWID) == 0 ||
-		    params->hdr.flowtype == M_HASHTYPE_NONE)
+		    params->hdr.flowtype == M_HASHTYPE_NONE) {
+			LAGG_RUNLOCK();
 			return (EOPNOTSUPP);
+		}
 		p = params->hdr.flowid >> sc->flowid_shift;
 		p %= sc->sc_count;
 		lb = (struct lagg_lb *)sc->sc_psc;
@@ -1553,16 +1554,22 @@ lagg_snd_tag_alloc(struct ifnet *ifp,
 		break;
 	case LAGG_PROTO_LACP:
 		if ((sc->sc_opts & LAGG_OPT_USE_FLOWID) == 0 ||
-		    params->hdr.flowtype == M_HASHTYPE_NONE)
+		    params->hdr.flowtype == M_HASHTYPE_NONE) {
+			LAGG_RUNLOCK();
 			return (EOPNOTSUPP);
+		}
 		lp = lacp_select_tx_port_by_hash(sc, params->hdr.flowid);
 		break;
 	default:
+		LAGG_RUNLOCK();
 		return (EOPNOTSUPP);
 	}
-	if (lp == NULL)
+	if (lp == NULL) {
+		LAGG_RUNLOCK();
 		return (EOPNOTSUPP);
+	}
 	ifp = lp->lp_ifp;
+	LAGG_RUNLOCK();
 	if (ifp == NULL || ifp->if_snd_tag_alloc == NULL ||
 	    (ifp->if_capenable & IFCAP_TXRTLMT) == 0)
 		return (EOPNOTSUPP);
@@ -1853,12 +1860,18 @@ struct lagg_port *
 lagg_link_active(struct lagg_softc *sc, struct lagg_port *lp)
 {
 	struct lagg_port *lp_next, *rval = NULL;
-	struct epoch_tracker net_et;
 
 	/*
 	 * Search a port which reports an active link state.
 	 */
 
+	/*
+	 * This is called with either LAGG_RLOCK() held or
+	 * LAGG_XLOCK(sc) held.
+	 */
+	if (!in_epoch(net_epoch_preempt))
+		LAGG_XLOCK_ASSERT(sc);
+
 	if (lp == NULL)
 		goto search;
 	if (LAGG_PORTACTIVE(lp)) {
@@ -1871,15 +1884,12 @@ lagg_link_active(struct lagg_softc *sc, struct lagg_po
 		goto found;
 	}
 
- search:
-	epoch_enter_preempt(net_epoch_preempt, &net_et);
+search:
 	CK_SLIST_FOREACH(lp_next, &sc->sc_ports, lp_entries) {
 		if (LAGG_PORTACTIVE(lp_next)) {
-			epoch_exit_preempt(net_epoch_preempt, &net_et);
 			return (lp_next);
 		}
 	}
-	epoch_exit_preempt(net_epoch_preempt, &net_et);
 found:
 	return (rval);
 }
@@ -1961,7 +1971,7 @@ lagg_bcast_start(struct lagg_softc *sc, struct mbuf *m
 	struct lagg_port *lp, *last = NULL;
 	struct mbuf *m0;
 
-	LAGG_RLOCK();
+	LAGG_RLOCK_ASSERT();
 	CK_SLIST_FOREACH(lp, &sc->sc_ports, lp_entries) {
 		if (!LAGG_PORTACTIVE(lp))
 			continue;
@@ -1982,7 +1992,6 @@ lagg_bcast_start(struct lagg_softc *sc, struct mbuf *m
 		}
 		last = lp;
 	}
-	LAGG_RUNLOCK();
 
 	if (last == NULL) {
 		m_freem(m);
@@ -2095,7 +2104,7 @@ lagg_lb_porttable(struct lagg_softc *sc, struct lagg_p
 
 	rv = 0;
 	bzero(&lb->lb_ports, sizeof(lb->lb_ports));
-	LAGG_RLOCK();
+	LAGG_XLOCK_ASSERT(sc);
 	CK_SLIST_FOREACH(lp_next, &sc->sc_ports, lp_entries) {
 		if (lp_next == lp)
 			continue;
@@ -2108,7 +2117,6 @@ lagg_lb_porttable(struct lagg_softc *sc, struct lagg_p
 			    sc->sc_ifname, lp_next->lp_ifp->if_xname, i);
 		lb->lb_ports[i++] = lp_next;
 	}
-	LAGG_RUNLOCK();
 
 	return (rv);
 }