Date: Thu, 20 Oct 2011 17:32:29 +0100 From: Ben Hutchings <bhutchings@solarflare.com> To: Andre Oppermann <andre@freebsd.org> Cc: freebsd-net@freebsd.org Subject: Re: TSO broken with jumbo MTU Message-ID: <1319128350.3147.6.camel@bwh-desktop> In-Reply-To: <1318894136.2784.76.camel@bwh-desktop> References: <1317309906.2743.9.camel@bwh-desktop> <1318865394.2784.4.camel@bwh-desktop> <4E9C534D.4090405@freebsd.org> <1318894136.2784.76.camel@bwh-desktop>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1319128350.3147.6.camel>