sepherosa_gmail.com updated the summary for this revision.
sepherosa_gmail.com updated this revision to Diff 13028.
sepherosa_gmail.com added a comment.


  Address gallatin and adrian's concern.

CHANGES SINCE LAST UPDATE
  https://reviews.freebsd.org/D5185?vs=12995&id=13028

REVISION DETAIL
  https://reviews.freebsd.org/D5185

AFFECTED FILES
  sys/dev/hyperv/netvsc/hv_net_vsc.h
  sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c
  sys/netinet/tcp_lro.c
  sys/netinet/tcp_lro.h

EMAIL PREFERENCES
  https://reviews.freebsd.org/settings/panel/emailpreferences/

To: sepherosa_gmail.com, network, adrian, delphij, royger, decui_microsoft.com, 
honzhan_microsoft.com, howard0su_gmail.com, hselasky, np, gallatin, transport
Cc: freebsd-virtualization-list, freebsd-net-list
diff --git a/sys/netinet/tcp_lro.h b/sys/netinet/tcp_lro.h
--- a/sys/netinet/tcp_lro.h
+++ b/sys/netinet/tcp_lro.h
@@ -91,11 +91,16 @@
 	unsigned	lro_cnt;
 	unsigned	lro_mbuf_count;
 	unsigned	lro_mbuf_max;
+	unsigned short	lro_ackcnt_lim;		/* max # of aggregated ACKs */
+	unsigned short	lro_length_lim;		/* max len of aggregated data */
 
 	struct lro_head	lro_active;
 	struct lro_head	lro_free;
 };
 
+#define	TCP_LRO_LENGTH_MAX	65535
+#define	TCP_LRO_ACKCNT_MAX	65535		/* unlimited */
+
 int tcp_lro_init(struct lro_ctrl *);
 int tcp_lro_init_args(struct lro_ctrl *, struct ifnet *, unsigned, unsigned);
 void tcp_lro_free(struct lro_ctrl *);
diff --git a/sys/netinet/tcp_lro.c b/sys/netinet/tcp_lro.c
--- a/sys/netinet/tcp_lro.c
+++ b/sys/netinet/tcp_lro.c
@@ -87,6 +87,8 @@
 	lc->lro_mbuf_count = 0;
 	lc->lro_mbuf_max = lro_mbufs;
 	lc->lro_cnt = lro_entries;
+	lc->lro_ackcnt_lim = TCP_LRO_ACKCNT_MAX;
+	lc->lro_length_lim = TCP_LRO_LENGTH_MAX;
 	lc->ifp = ifp;
 	SLIST_INIT(&lc->lro_free);
 	SLIST_INIT(&lc->lro_active);
@@ -608,7 +610,7 @@
 		}
 
 		/* Flush now if appending will result in overflow. */
-		if (le->p_len > (65535 - tcp_data_len)) {
+		if (le->p_len > (lc->lro_length_lim - tcp_data_len)) {
 			SLIST_REMOVE(&lc->lro_active, le, lro_entry, next);
 			tcp_lro_flush(lc, le);
 			break;
@@ -646,6 +648,15 @@
 
 		if (tcp_data_len == 0) {
 			m_freem(m);
+			/*
+			 * Flush this LRO entry, if this ACK should not
+			 * be further delayed.
+			 */
+			if (le->append_cnt >= lc->lro_ackcnt_lim) {
+				SLIST_REMOVE(&lc->lro_active, le, lro_entry,
+				    next);
+				tcp_lro_flush(lc, le);
+			}
 			return (0);
 		}
 
@@ -666,7 +677,7 @@
 		 * If a possible next full length packet would cause an
 		 * overflow, pro-actively flush now.
 		 */
-		if (le->p_len > (65535 - lc->ifp->if_mtu)) {
+		if (le->p_len > (lc->lro_length_lim - lc->ifp->if_mtu)) {
 			SLIST_REMOVE(&lc->lro_active, le, lro_entry, next);
 			tcp_lro_flush(lc, le);
 		} else
diff --git a/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c b/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c
--- a/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c
+++ b/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c
@@ -176,14 +176,11 @@
 #define HN_CSUM_ASSIST_WIN8	(CSUM_TCP)
 #define HN_CSUM_ASSIST		(CSUM_IP | CSUM_UDP | CSUM_TCP)
 
-/* XXX move to netinet/tcp_lro.h */
-#define HN_LRO_HIWAT_MAX				65535
-#define HN_LRO_HIWAT_DEF				HN_LRO_HIWAT_MAX
+#define HN_LRO_LENLIM_DEF		(25 * ETHERMTU)
 /* YYY 2*MTU is a bit rough, but should be good enough. */
-#define HN_LRO_HIWAT_MTULIM(ifp)			(2 * (ifp)->if_mtu)
-#define HN_LRO_HIWAT_ISVALID(sc, hiwat)			\
-    ((hiwat) >= HN_LRO_HIWAT_MTULIM((sc)->hn_ifp) ||	\
-     (hiwat) <= HN_LRO_HIWAT_MAX)
+#define HN_LRO_LENLIM_MIN(ifp)		(2 * (ifp)->if_mtu)
+
+#define HN_LRO_ACKCNT_DEF		1
 
 /*
  * Be aware that this sleepable mutex will exhibit WITNESS errors when
@@ -253,9 +250,8 @@
 static void hn_start_txeof(struct ifnet *ifp);
 static int hn_ifmedia_upd(struct ifnet *ifp);
 static void hn_ifmedia_sts(struct ifnet *ifp, struct ifmediareq *ifmr);
-#ifdef HN_LRO_HIWAT
-static int hn_lro_hiwat_sysctl(SYSCTL_HANDLER_ARGS);
-#endif
+static int hn_lro_lenlim_sysctl(SYSCTL_HANDLER_ARGS);
+static int hn_lro_ackcnt_sysctl(SYSCTL_HANDLER_ARGS);
 static int hn_trust_hcsum_sysctl(SYSCTL_HANDLER_ARGS);
 static int hn_tx_chimney_size_sysctl(SYSCTL_HANDLER_ARGS);
 static int hn_check_iplen(const struct mbuf *, int);
@@ -265,15 +261,6 @@
 static void hn_txeof_taskfunc(void *xsc, int pending);
 static int hn_encap(struct hn_softc *, struct hn_txdesc *, struct mbuf **);
 
-static __inline void
-hn_set_lro_hiwat(struct hn_softc *sc, int hiwat)
-{
-	sc->hn_lro_hiwat = hiwat;
-#ifdef HN_LRO_HIWAT
-	sc->hn_lro.lro_hiwat = sc->hn_lro_hiwat;
-#endif
-}
-
 static int
 hn_ifmedia_upd(struct ifnet *ifp __unused)
 {
@@ -358,7 +345,6 @@
 	bzero(sc, sizeof(hn_softc_t));
 	sc->hn_unit = unit;
 	sc->hn_dev = dev;
-	sc->hn_lro_hiwat = HN_LRO_HIWAT_DEF;
 	sc->hn_direct_tx_size = hn_direct_tx_size;
 	if (hn_trust_hosttcp)
 		sc->hn_trust_hcsum |= HN_TRUST_HCSUM_TCP;
@@ -442,9 +428,8 @@
 	/* Driver private LRO settings */
 	sc->hn_lro.ifp = ifp;
 #endif
-#ifdef HN_LRO_HIWAT
-	sc->hn_lro.lro_hiwat = sc->hn_lro_hiwat;
-#endif
+	sc->hn_lro.lro_length_lim = HN_LRO_LENLIM_DEF;
+	sc->hn_lro.lro_ackcnt_lim = HN_LRO_ACKCNT_DEF;
 #endif	/* INET || INET6 */
 
 #if __FreeBSD_version >= 1100045
@@ -480,11 +465,12 @@
 	    CTLFLAG_RW, &sc->hn_lro.lro_flushed, 0, "LRO flushed");
 	SYSCTL_ADD_ULONG(ctx, child, OID_AUTO, "lro_tried",
 	    CTLFLAG_RW, &sc->hn_lro_tried, "# of LRO tries");
-#ifdef HN_LRO_HIWAT
-	SYSCTL_ADD_PROC(ctx, child, OID_AUTO, "lro_hiwat",
-	    CTLTYPE_INT | CTLFLAG_RW, sc, 0, hn_lro_hiwat_sysctl,
-	    "I", "LRO high watermark");
-#endif
+	SYSCTL_ADD_PROC(ctx, child, OID_AUTO, "lro_length_lim",
+	    CTLTYPE_INT | CTLFLAG_RW, sc, 0, hn_lro_lenlim_sysctl, "I",
+	    "Max # of data bytes to be aggregated by LRO");
+	SYSCTL_ADD_PROC(ctx, child, OID_AUTO, "lro_ackcnt_lim",
+	    CTLTYPE_INT | CTLFLAG_RW, sc, 0, hn_lro_ackcnt_sysctl, "I",
+	    "Max # of ACKs to be aggregated by LRO");
 	SYSCTL_ADD_PROC(ctx, child, OID_AUTO, "trust_hosttcp",
 	    CTLTYPE_INT | CTLFLAG_RW, sc, HN_TRUST_HCSUM_TCP,
 	    hn_trust_hcsum_sysctl, "I",
@@ -1410,12 +1396,13 @@
 
 		/* Obtain and record requested MTU */
 		ifp->if_mtu = ifr->ifr_mtu;
+
 		/*
-		 * Make sure that LRO high watermark is still valid,
-		 * after MTU change (the 2*MTU limit).
+		 * Make sure that LRO aggregation length limit is still
+		 * valid, after the MTU change.
 		 */
-		if (!HN_LRO_HIWAT_ISVALID(sc, sc->hn_lro_hiwat))
-			hn_set_lro_hiwat(sc, HN_LRO_HIWAT_MTULIM(ifp));
+		if (sc->hn_lro.lro_length_lim < HN_LRO_LENLIM_MIN(ifp))
+			sc->hn_lro.lro_length_lim = HN_LRO_LENLIM_MIN(ifp);
 
 		do {
 			NV_LOCK(sc);
@@ -1722,26 +1709,50 @@
 }
 #endif
 
-#ifdef HN_LRO_HIWAT
 static int
-hn_lro_hiwat_sysctl(SYSCTL_HANDLER_ARGS)
+hn_lro_lenlim_sysctl(SYSCTL_HANDLER_ARGS)
+{
+	struct hn_softc *sc = arg1;
+	int lenlim, error;
+
+	lenlim = sc->hn_lro.lro_length_lim;
+	error = sysctl_handle_int(oidp, &lenlim, 0, req);
+	if (error || req->newptr == NULL)
+		return error;
+
+	if (lenlim < HN_LRO_LENLIM_MIN(sc->hn_ifp) ||
+	    lenlim > TCP_LRO_LENGTH_MAX)
+		return EINVAL;
+
+	sc->hn_lro.lro_length_lim = lenlim;
+	return 0;
+}
+
+static int
+hn_lro_ackcnt_sysctl(SYSCTL_HANDLER_ARGS)
 {
 	struct hn_softc *sc = arg1;
-	int hiwat, error;
+	int ackcnt, error;
 
-	hiwat = sc->hn_lro_hiwat;
-	error = sysctl_handle_int(oidp, &hiwat, 0, req);
+	/*
+	 * lro_ackcnt_lim is append count limit,
+	 * +1 to turn it into aggregation limit.
+	 */
+	ackcnt = sc->hn_lro.lro_ackcnt_lim + 1;
+	error = sysctl_handle_int(oidp, &ackcnt, 0, req);
 	if (error || req->newptr == NULL)
 		return error;
 
-	if (!HN_LRO_HIWAT_ISVALID(sc, hiwat))
+	if (ackcnt < 2 || ackcnt > (TCP_LRO_ACKCNT_MAX + 1))
 		return EINVAL;
 
-	if (sc->hn_lro_hiwat != hiwat)
-		hn_set_lro_hiwat(sc, hiwat);
+	/*
+	 * Convert aggregation limit back to append
+	 * count limit.
+	 */
+	sc->hn_lro.lro_ackcnt_lim = ackcnt - 1;
 	return 0;
 }
-#endif	/* HN_LRO_HIWAT */
 
 static int
 hn_trust_hcsum_sysctl(SYSCTL_HANDLER_ARGS)
diff --git a/sys/dev/hyperv/netvsc/hv_net_vsc.h b/sys/dev/hyperv/netvsc/hv_net_vsc.h
--- a/sys/dev/hyperv/netvsc/hv_net_vsc.h
+++ b/sys/dev/hyperv/netvsc/hv_net_vsc.h
@@ -1030,7 +1030,6 @@
 	struct task	hn_txeof_task;
 
 	struct lro_ctrl	hn_lro;
-	int		hn_lro_hiwat;
 
 	/* Trust csum verification on host side */
 	int		hn_trust_hcsum;	/* HN_TRUST_HCSUM_ */

_______________________________________________
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

Reply via email to