TSO broken with jumbo MTU

[Ben Hutchings]

On Tue, 2011-10-18 at 00:28 +0100, Ben Hutchings wrote:
> On Mon, 2011-10-17 at 18:09 +0200, Andre Oppermann wrote:
> > On 17.10.2011 17:29, Ben Hutchings wrote:
> > > This is the fix/workaround I used:
> > 
> > Thanks for the fix.  I'll review it and put it into FreeBSD maybe in
> > a slightly different form.
> 
> Which one?  One is tested but maybe not right; the other looks right but
> is not tested!

I have now run the same test that originally triggered the assertion
failure, with the 'right' change, and it passes.  The test script
performed various interface reconfiguration (including MTU changes) and
link state changes with TCP and UDP flows active through the interface,
over a period of 12 hours.

For reference, the 'right' change is:

--- a/netinet/tcp_input.c
+++ b/netinet/tcp_input.c
@@ -3087,7 +3087,7 @@
 tcp_mss_update(struct tcpcb *tp, int offer,
     struct hc_metrics_lite *metricptr, int *mtuflags)
 {
-	int mss;
+	int mss, ts_len;
 	u_long maxmtu;
 	struct inpcb *inp = tp->t_inpcb;
 	struct hc_metrics_lite metrics;
@@ -3212,22 +3212,17 @@
 	mss = max(mss, 64);
 
 	/*
-	 * maxopd stores the maximum length of data AND options
-	 * in a segment; maxseg is the amount of data in a normal
-	 * segment.  We need to store this value (maxopd) apart
-	 * from maxseg, because now every segment carries options
-	 * and thus we normally have somewhat less data in segments.
-	 */
-	tp->t_maxopd = mss;
-
-	/*
 	 * origoffer==-1 indicates that no segments were received yet.
 	 * In this case we just guess.
 	 */
 	if ((tp->t_flags & (TF_REQ_TSTMP|TF_NOOPT)) == TF_REQ_TSTMP &&
 	    (origoffer == -1 ||
 	     (tp->t_flags & TF_RCVD_TSTMP) == TF_RCVD_TSTMP))
-		mss -= TCPOLEN_TSTAMP_APPA;
+		ts_len = TCPOLEN_TSTAMP_APPA;
+	else
+		ts_len = 0;
+
+	mss -= ts_len;
 
 #if	(MCLBYTES & (MCLBYTES - 1)) == 0
 	if (mss > MCLBYTES)
@@ -3237,6 +3232,15 @@
 		mss = mss / MCLBYTES * MCLBYTES;
 #endif
 	tp->t_maxseg = mss;
+
+	/*
+	 * maxopd stores the maximum length of data AND options
+	 * in a segment; maxseg is the amount of data in a normal
+	 * segment.  We need to store this value (maxopd) apart
+	 * from maxseg, because now every segment carries options
+	 * and thus we normally have somewhat less data in segments.
+	 */
+	tp->t_maxopd = mss + ts_len;
 }
 
 void
--- END ---

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.