From owner-freebsd-bugs@FreeBSD.ORG Tue Dec 21 10:00:44 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 AB66C16A585 for ; Tue, 21 Dec 2004 10:00:44 +0000 (GMT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 76DF343D53 for ; Tue, 21 Dec 2004 10:00:44 +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 iBLA0iDG006796 for ; Tue, 21 Dec 2004 10:00:44 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.1/8.13.1/Submit) id iBLA0iru006795; Tue, 21 Dec 2004 10:00:44 GMT (envelope-from gnats) Date: Tue, 21 Dec 2004 10:00:44 GMT Message-Id: <200412211000.iBLA0iru006795@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: Tue, 21 Dec 2004 10:00:44 -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: Tue, 21 Dec 2004 10:50:14 +0100 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