Date: Mon, 14 Jan 2013 16:04:29 -0500 From: John Baldwin <jhb@freebsd.org> To: net@freebsd.org Cc: Lawrence Stewart <lstewart@freebsd.org> Subject: Some questions about the new TCP congestion control code Message-ID: <201301141604.29864.jhb@freebsd.org>
next in thread | raw e-mail | index | archive | help
I was looking at TCP congestion control at work recently and noticed a few
"odd" things in the current code. First, there is this chunk of code in
cc_ack_received() in tcp_input.c:
static void inline
cc_ack_received(struct tcpcb *tp, struct tcphdr *th, uint16_t type)
{
INP_WLOCK_ASSERT(tp->t_inpcb);
tp->ccv->bytes_this_ack = BYTES_THIS_ACK(tp, th);
if (tp->snd_cwnd == min(tp->snd_cwnd, tp->snd_wnd))
tp->ccv->flags |= CCF_CWND_LIMITED;
else
tp->ccv->flags &= ~CCF_CWND_LIMITED;
Due to hysterical raisins, snd_cwnd and snd_wnd are u_long values, not
integers, so the call to min() results in truncation on 64-bit hosts. It
should probably be ulmin() instead. However, this line seems to be a really
obfuscated way to just write:
if (tp->snd_cwnd <= tp->snd_wnd)
If that is correct, I would vote for changing this to use the much simpler
logic.
Secondly, in the particular case I was investigating at work (restart of an
idle connnection), the newreno congestion control code in 8.x and later uses a
different algorithm than in 7. Specifically, in 7 TCP would reuse the same
logic used for an initial cwnd (honoring ss_fltsz). In 8 this no longer
happens (instead, 2 is hardcoded). A guess at a possible fix might look
something like this:
Index: cc_newreno.c
===================================================================
--- cc_newreno.c (revision 243660)
+++ cc_newreno.c (working copy)
@@ -169,8 +169,21 @@ newreno_after_idle(struct cc_var *ccv)
if (V_tcp_do_rfc3390)
rw = min(4 * CCV(ccv, t_maxseg),
max(2 * CCV(ccv, t_maxseg), 4380));
+#if 1
else
rw = CCV(ccv, t_maxseg) * 2;
+#else
+ /* XXX: This is missing a lot of stuff that used to be in 7. */
+#ifdef INET6
+ else if ((isipv6 ? in6_localaddr(&CCV(ccv, t_inpcb->in6p_faddr)) :
+ in_localaddr(CCV(ccv, t_inpcb->inp_faddr))))
+#else
+ else if (in_localaddr(CCV(ccv, t_inpcb->inp_faddr)))
+#endif
+ rw = V_ss_fltsz_local * CCV(ccv, t_maxseg);
+ else
+ rw = V_ss_fltsz * CCV(ccv, t_maxseg);
+#endif
CCV(ccv, snd_cwnd) = min(rw, CCV(ccv, snd_cwnd));
}
(But using the #else clause instead of the current #if 1 code). Was this
change in 8 intentional? If not, I would suggest that a real fix would be to
export a function that includes the logic to compute the initial cwnd and
share that between cc_conn_init() in tcp_input.c and cc_newreno.c. (Yes, I
realize that ss_fltsz and friends are gone in 10, but if this was a bug then
the fix I suggested above of using a common function could be applied to 8 to
restore that functionality if desired. The bigger point is to make sure what
we are doing is correct as I'm not sure.)
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201301141604.29864.jhb>