From owner-freebsd-bugs@FreeBSD.ORG Sat Dec 18 12:10:31 2004 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 05A4616A4CE for ; Sat, 18 Dec 2004 12:10:31 +0000 (GMT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id CC61443D3F for ; Sat, 18 Dec 2004 12:10:30 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.1/8.13.1) with ESMTP id iBICAU3o029488 for ; Sat, 18 Dec 2004 12:10:30 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.1/8.13.1/Submit) id iBICAUK0029485; Sat, 18 Dec 2004 12:10:30 GMT (envelope-from gnats) Date: Sat, 18 Dec 2004 12:10:30 GMT Message-Id: <200412181210.iBICAUK0029485@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Uwe Doering Subject: Re: kern/75122: [PATCH] Incorrect inflight bandwidth calculation on first packet X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: Uwe Doering List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 18 Dec 2004 12:10:31 -0000 The following reply was made to PR kern/75122; it has been noted by GNATS. From: Uwe Doering To: Dan Nelson Cc: FreeBSD-gnats-submit@FreeBSD.org, freebsd-bugs@FreeBSD.org Subject: Re: kern/75122: [PATCH] Incorrect inflight bandwidth calculation on first packet Date: Sat, 18 Dec 2004 13:09:14 +0100 Dan Nelson wrote: > Updated patch including Matt's recommended fix: > > [...] > diff -u -p -r1.201.2.4 tcp_subr.c > --- tcp_subr.c 1 Dec 2004 05:37:28 -0000 1.201.2.4 > +++ tcp_subr.c 16 Dec 2004 06:38:48 -0000 > [...] > @@ -1922,29 +1922,42 @@ tcp_xmit_bandwidth_limit(struct tcpcb *t > } > > /* > + * Validate the delta time. If a connection is new or has been idle > + * a long time we have to reset the bandwidth calculator. > + */ > + save_ticks = ticks; > + delta_ticks = save_ticks - tp->t_bw_rtttime; > + if (tp->t_bw_rtttime == 0 || delta_ticks < 0 || delta_ticks > hz * 10) { > + tp->t_bw_rtttime = ticks; > + tp->t_bw_rtseq = ack_seq; > + if (tp->snd_bandwidth == 0) > + tp->snd_bandwidth = tcp_inflight_min; > + return; > + } > + if (delta_ticks == 0) > + return; > + > + /* > + * Sanity check, plus ignore pure window update acks. > + */ > + if ((int)(ack_seq - tp->t_bw_rtseq) <= 0) > + return; I wonder, isn't there a flaw in the logic with regard to the sequence number handling? If the sequence number wraps around 't_bw_rtseq' no longer gets set and therefore the bandwidth calculation stops until 'ack_seq' either catches up with 't_bw_rtseq' again (which would take quite a while), or 'ticks' wraps around as well, or there is inactivity for more than 10 seconds. This is probably not the intended behavior. I think the correct approach would be to separate the handling of the two cases "sequence number wrap-around" and "window update acks". I suggest to define another variable 'delta_bytes' and initialize it like this: int delta_bytes; [...] delta_bytes = ack_seq - tp->t_bw_rtseq; Then compare it with zero where you compare 'delta_ticks' with zero, and check for less than zero (wrap-around) where you check 'delta_ticks' for less than zero. Combine the respective test results with '||'. Of course, you can then use 'delta_bytes' for the bandwidth calculation further down in the code, too. Uwe -- Uwe Doering | EscapeBox - Managed On-Demand UNIX Servers gemini@geminix.org | http://www.escapebox.net