From owner-freebsd-bugs@FreeBSD.ORG Tue Dec 21 09:50:30 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 CD12416A4CE; Tue, 21 Dec 2004 09:50:30 +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 1F50443D41; Tue, 21 Dec 2004 09:50:30 +0000 (GMT) (envelope-from gemini@geminix.org) Message-ID: <41C7F1D6.7020108@geminix.org> Date: Tue, 21 Dec 2004 10:50:14 +0100 From: Uwe Doering Organization: Private UNIX Site User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.5) Gecko/20041220 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> <41C41DEA.9000504@geminix.org> <20041221065531.GA78451@dan.emsphone.com> In-Reply-To: <20041221065531.GA78451@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 1Cggen-000FJV-00; Tue, 21 Dec 2004 10:50:18 +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: Tue, 21 Dec 2004 09:50:30 -0000 Dan Nelson wrote: > In the last episode (Dec 18), Uwe Doering said: >>Dan Nelson wrote: >> >>>Updated patch including Matt's recommended fix: >>> >>>+ /* >>>+ * 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 code works as-is. ack_seq and tp->t_bw_rtseq are both of > type "tcp_seq" which is a u_int32_t. Wrap-around is handled > transparently when your variables are unsigned and your sequence space > covers all possible values. It's the magic of mod(2^32) arithmetic :) > The (int) cast just makes the if simpler. Without the cast it would > read > > if (ack_seq - tp->t_bw_rtseq > 2147483648U || ack_seq == tp->t_bw_rtseq) > > The sanity check is probably not even necessary, as any really invalid > sequence numbers would have caused the packet to be dropped before it > got this far. You are correct. Unlike 'ticks', which is of type 'int', a sequence number wrap-around has no effect on the calculation of the byte count due to using 'u_int32_t'. Tricky. ;-) Therefore, I propose testing for equality only (window update acks). Checking for byte counts of more than 2147483648U and declaring that bogus is a little arbitrary. If we are concerned about invalid sequence numbers, they can result in a lower byte count as well. The way it is now (with the patch installed) it looks too much like a wrap-around check and is therefore misleading. So I suggest to either get rid of it or add a comment that explains the situation. Uwe -- Uwe Doering | EscapeBox - Managed On-Demand UNIX Servers gemini@geminix.org | http://www.escapebox.net