tcp:Host cache and rack ending up with incorrect values.

The hostcache up to now as been updated in the discard callback
but without checking if we are all done (the race where there are
more than one calls and the counter has not yet reached zero). This
means that when the race occurs, we end up calling the hc_upate
more than once. Also alternate stacks can keep there srtt/rttvar
in different formats (example rack keeps its values in microseconds).
Since we call the hc_update *before* the stack fini() then the
values will be in the wrong format.

Rack on the other hand, needs to convert items pulled from the
hostcache into its internal format else it may end up with
very much incorrect values from the hostcache. In the process
lets commonize the update mechanism for srtt/rttvar since we
now have more than one place that needs to call it.

Reviewed by: Michael Tuexen
Sponsored by: Netflix Inc
Differential Revision:	https://reviews.freebsd.org/D30172

(cherry picked from commit 9867224bab3f247ac875d89c2472aa4bc855fe3b)

2 files changed, 119 insertions, 101 deletions

diff --git a/sys/netinet/tcp_stacks/rack.c b/sys/netinet/tcp_stacks/rack.c
index 110304d29cc0..7bb77d8158af 100644
--- a/sys/netinet/tcp_stacks/rack.c
+++ b/sys/netinet/tcp_stacks/rack.c
@@ -6564,13 +6564,65 @@ rack_remxt_tmr(struct tcpcb *tp)
 }
 
 static void
+rack_convert_rtts(struct tcpcb *tp)
+{
+	if (tp->t_srtt > 1) {
+		uint32_t val, frac;
+
+		val = tp->t_srtt >> TCP_RTT_SHIFT;
+		frac = tp->t_srtt & 0x1f;
+		tp->t_srtt = TICKS_2_USEC(val);
+		/*
+		 * frac is the fractional part of the srtt (if any)
+		 * but its in ticks and every bit represents
+		 * 1/32nd of a hz.
+		 */
+		if (frac) {
+			if (hz == 1000) {
+				frac = (((uint64_t)frac * (uint64_t)HPTS_USEC_IN_MSEC) / (uint64_t)TCP_RTT_SCALE);
+			} else {
+				frac = (((uint64_t)frac * (uint64_t)HPTS_USEC_IN_SEC) / ((uint64_t)(hz) * (uint64_t)TCP_RTT_SCALE));
+			}
+			tp->t_srtt += frac;
+		}
+	}
+	if (tp->t_rttvar) {
+		uint32_t val, frac;
+
+		val = tp->t_rttvar >> TCP_RTTVAR_SHIFT;
+		frac = tp->t_rttvar & 0x1f;
+		tp->t_rttvar = TICKS_2_USEC(val);
+		/*
+		 * frac is the fractional part of the srtt (if any)
+		 * but its in ticks and every bit represents
+		 * 1/32nd of a hz.
+		 */
+		if (frac) {
+			if (hz == 1000) {
+				frac = (((uint64_t)frac * (uint64_t)HPTS_USEC_IN_MSEC) / (uint64_t)TCP_RTT_SCALE);
+			} else {
+				frac = (((uint64_t)frac * (uint64_t)HPTS_USEC_IN_SEC) / ((uint64_t)(hz) * (uint64_t)TCP_RTT_SCALE));
+			}
+			tp->t_rttvar += frac;
+		}
+	}
+	RACK_TCPT_RANGESET(tp->t_rxtcur, RACK_REXMTVAL(tp),
+			   rack_rto_min, rack_rto_max);
+}
+
+static void
 rack_cc_conn_init(struct tcpcb *tp)
 {
 	struct tcp_rack *rack;
 
 	rack = (struct tcp_rack *)tp->t_fb_ptr;
+
 	cc_conn_init(tp);
 	/*
+	 * Now convert to rack's internal format.
+	 */
+	rack_convert_rtts(tp);
+	/*
 	 * We want a chance to stay in slowstart as
 	 * we create a connection. TCP spec says that
 	 * initially ssthresh is infinite. For our
@@ -11916,9 +11968,9 @@ rack_init_fsb_block(struct tcpcb *tp, struct tcp_rack *rack)
 static int
 rack_init_fsb(struct tcpcb *tp, struct tcp_rack *rack)
 {
-	/* 
-	 * Allocate the larger of spaces V6 if available else just 
-	 * V4 and include udphdr (overbook) 
+	/*
+	 * Allocate the larger of spaces V6 if available else just
+	 * V4 and include udphdr (overbook)
 	 */
 #ifdef INET6
 	rack->r_ctl.fsb.tcp_ip_hdr_len = sizeof(struct ip6_hdr) + sizeof(struct tcphdr) + sizeof(struct udphdr);
@@ -12147,47 +12199,7 @@ rack_init(struct tcpcb *tp)
 	 * bit decimal so we have to carefully convert
 	 * these to get the full precision.
 	 */
-	if (tp->t_srtt > 1) {
-		uint32_t val, frac;
-
-		val = tp->t_srtt >> TCP_RTT_SHIFT;
-		frac = tp->t_srtt & 0x1f;
-		tp->t_srtt = TICKS_2_USEC(val);
-		/*
-		 * frac is the fractional part of the srtt (if any)
-		 * but its in ticks and every bit represents
-		 * 1/32nd of a hz.
-		 */
-		if (frac) {
-			if (hz == 1000) {
-				frac = (((uint64_t)frac * (uint64_t)HPTS_USEC_IN_MSEC) / (uint64_t)TCP_RTT_SCALE);
-			} else {
-				frac = (((uint64_t)frac * (uint64_t)HPTS_USEC_IN_SEC) / ((uint64_t)(hz) * (uint64_t)TCP_RTT_SCALE));
-			}
-			tp->t_srtt += frac;
-		}
-	}
-	if (tp->t_rttvar) {
-		uint32_t val, frac;
-
-		val = tp->t_rttvar >> TCP_RTTVAR_SHIFT;
-		frac = tp->t_rttvar & 0x1f;
-		tp->t_rttvar = TICKS_2_USEC(val);
-		/*
-		 * frac is the fractional part of the srtt (if any)
-		 * but its in ticks and every bit represents
-		 * 1/32nd of a hz.
-		 */
-		if (frac) {
-			if (hz == 1000) {
-				frac = (((uint64_t)frac * (uint64_t)HPTS_USEC_IN_MSEC) / (uint64_t)TCP_RTT_SCALE);
-			} else {
-				frac = (((uint64_t)frac * (uint64_t)HPTS_USEC_IN_SEC) / ((uint64_t)(hz) * (uint64_t)TCP_RTT_SCALE));
-			}
-			tp->t_rttvar += frac;
-		}
-	}
-	tp->t_rxtcur = TICKS_2_USEC(tp->t_rxtcur);
+	rack_convert_rtts(tp);
 	tp->t_rttlow = TICKS_2_USEC(tp->t_rttlow);
 	if (rack_def_profile)
 		rack_set_profile(rack, rack_def_profile);
@@ -12230,7 +12242,7 @@ rack_init(struct tcpcb *tp)
 	rack_stop_all_timers(tp);
 	/* Lets setup the fsb block */
 	rack_start_hpts_timer(rack, tp, tcp_get_usecs(NULL), 0, 0, 0);
-	rack_log_rtt_shrinks(rack,  us_cts,  0,
+	rack_log_rtt_shrinks(rack,  us_cts,  tp->t_rxtcur,
 			     __LINE__, RACK_RTTS_INIT);
 	return (0);
 }
diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c
index 93445c636d41..b9da908d2a15 100644
--- a/sys/netinet/tcp_subr.c
+++ b/sys/netinet/tcp_subr.c
@@ -2305,62 +2305,6 @@ tcp_discardcb(struct tcpcb *tp)
 		tp->t_fb->tfb_tcp_timer_stop_all(tp);
 	}
 
-	/*
-	 * If we got enough samples through the srtt filter,
-	 * save the rtt and rttvar in the routing entry.
-	 * 'Enough' is arbitrarily defined as 4 rtt samples.
-	 * 4 samples is enough for the srtt filter to converge
-	 * to within enough % of the correct value; fewer samples
-	 * and we could save a bogus rtt. The danger is not high
-	 * as tcp quickly recovers from everything.
-	 * XXX: Works very well but needs some more statistics!
-	 */
-	if (tp->t_rttupdated >= 4) {
-		struct hc_metrics_lite metrics;
-		uint32_t ssthresh;
-
-		bzero(&metrics, sizeof(metrics));
-		/*
-		 * Update the ssthresh always when the conditions below
-		 * are satisfied. This gives us better new start value
-		 * for the congestion avoidance for new connections.
-		 * ssthresh is only set if packet loss occurred on a session.
-		 *
-		 * XXXRW: 'so' may be NULL here, and/or socket buffer may be
-		 * being torn down.  Ideally this code would not use 'so'.
-		 */
-		ssthresh = tp->snd_ssthresh;
-		if (ssthresh != 0 && ssthresh < so->so_snd.sb_hiwat / 2) {
-			/*
-			 * convert the limit from user data bytes to
-			 * packets then to packet data bytes.
-			 */
-			ssthresh = (ssthresh + tp->t_maxseg / 2) / tp->t_maxseg;
-			if (ssthresh < 2)
-				ssthresh = 2;
-			ssthresh *= (tp->t_maxseg +
-#ifdef INET6
-			    (isipv6 ? sizeof (struct ip6_hdr) +
-				sizeof (struct tcphdr) :
-#endif
-				sizeof (struct tcpiphdr)
-#ifdef INET6
-			    )
-#endif
-			    );
-		} else
-			ssthresh = 0;
-		metrics.rmx_ssthresh = ssthresh;
-
-		metrics.rmx_rtt = tp->t_srtt;
-		metrics.rmx_rttvar = tp->t_rttvar;
-		metrics.rmx_cwnd = tp->snd_cwnd;
-		metrics.rmx_sendpipe = 0;
-		metrics.rmx_recvpipe = 0;
-
-		tcp_hc_update(&inp->inp_inc, &metrics);
-	}
-
 	/* free the reassembly queue, if any */
 	tcp_reass_flush(tp);
 
@@ -2400,6 +2344,68 @@ tcp_discardcb(struct tcpcb *tp)
 		TCPSTATES_DEC(tp->t_state);
 		if (tp->t_fb->tfb_tcp_fb_fini)
 			(*tp->t_fb->tfb_tcp_fb_fini)(tp, 1);
+
+		/*
+		 * If we got enough samples through the srtt filter,
+		 * save the rtt and rttvar in the routing entry.
+		 * 'Enough' is arbitrarily defined as 4 rtt samples.
+		 * 4 samples is enough for the srtt filter to converge
+		 * to within enough % of the correct value; fewer samples
+		 * and we could save a bogus rtt. The danger is not high
+		 * as tcp quickly recovers from everything.
+		 * XXX: Works very well but needs some more statistics!
+		 *
+		 * XXXRRS: Updating must be after the stack fini() since
+		 * that may be converting some internal representation of
+		 * say srtt etc into the general one used by other stacks.
+		 * Lets also at least protect against the so being NULL
+		 * as RW stated below.
+		 */
+		if ((tp->t_rttupdated >= 4) && (so != NULL)) {
+			struct hc_metrics_lite metrics;
+			uint32_t ssthresh;
+
+			bzero(&metrics, sizeof(metrics));
+			/*
+			 * Update the ssthresh always when the conditions below
+			 * are satisfied. This gives us better new start value
+			 * for the congestion avoidance for new connections.
+			 * ssthresh is only set if packet loss occurred on a session.
+			 *
+			 * XXXRW: 'so' may be NULL here, and/or socket buffer may be
+			 * being torn down.  Ideally this code would not use 'so'.
+			 */
+			ssthresh = tp->snd_ssthresh;
+			if (ssthresh != 0 && ssthresh < so->so_snd.sb_hiwat / 2) {
+				/*
+				 * convert the limit from user data bytes to
+				 * packets then to packet data bytes.
+				 */
+				ssthresh = (ssthresh + tp->t_maxseg / 2) / tp->t_maxseg;
+				if (ssthresh < 2)
+					ssthresh = 2;
+				ssthresh *= (tp->t_maxseg +
+#ifdef INET6
+					     (isipv6 ? sizeof (struct ip6_hdr) +
+					      sizeof (struct tcphdr) :
+#endif
+					      sizeof (struct tcpiphdr)
+#ifdef INET6
+						     )
+#endif
+					);
+			} else
+				ssthresh = 0;
+			metrics.rmx_ssthresh = ssthresh;
+
+			metrics.rmx_rtt = tp->t_srtt;
+			metrics.rmx_rttvar = tp->t_rttvar;
+			metrics.rmx_cwnd = tp->snd_cwnd;
+			metrics.rmx_sendpipe = 0;
+			metrics.rmx_recvpipe = 0;
+
+			tcp_hc_update(&inp->inp_inc, &metrics);
+		}
 		refcount_release(&tp->t_fb->tfb_refcnt);
 		tp->t_inpcb = NULL;
 		uma_zfree(V_tcpcb_zone, tp);