From owner-freebsd-bugs@FreeBSD.ORG Sat Dec 18 12:09:27 2004 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 8F27416A4CE; Sat, 18 Dec 2004 12:09:27 +0000 (GMT) Received: from gen129.n001.c02.escapebox.net (gen129.n001.c02.escapebox.net [213.73.91.129]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1639B43D39; Sat, 18 Dec 2004 12:09:27 +0000 (GMT) (envelope-from gemini@geminix.org) Message-ID: <41C41DEA.9000504@geminix.org> Date: Sat, 18 Dec 2004 13:09:14 +0100 From: Uwe Doering Organization: Private UNIX Site User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.3) Gecko/20041212 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Dan Nelson References: <200412151827.iBFIRqDB019997@dan.emsphone.com> <200412151830.iBFIUVwE052799@freefall.freebsd.org> <20041216190608.GA21382@dan.emsphone.com> In-Reply-To: <20041216190608.GA21382@dan.emsphone.com> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Received: from gemini by geminix.org with asmtp (TLSv1:AES256-SHA:256) (Exim 3.36 #1) id 1CfdOf-0003xS-00; Sat, 18 Dec 2004 13:09:17 +0100 cc: freebsd-bugs@FreeBSD.org cc: FreeBSD-gnats-submit@FreeBSD.org 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 List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 18 Dec 2004 12:09:27 -0000 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