From owner-freebsd-bugs@FreeBSD.ORG Tue Dec 21 11:30:28 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 1666516A4CF for ; Tue, 21 Dec 2004 11:30:28 +0000 (GMT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id D2B6E43D5D for ; Tue, 21 Dec 2004 11:30:27 +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 iBLBULWl018638 for ; Tue, 21 Dec 2004 11:30:21 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.1/8.13.1/Submit) id iBLBUKWO018637; Tue, 21 Dec 2004 11:30:20 GMT (envelope-from gnats) Date: Tue, 21 Dec 2004 11:30:20 GMT Message-Id: <200412211130.iBLBUKWO018637@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 11:30:28 -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 12:20:28 +0100 Uwe Doering wrote: > 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. On second thought, checking for less than zero (by means of the 'int' cast) might have its merits. This may be a protection against out-of-order ACKs, which could in fact be valid, but calculating a byte count from an ACK for an earlier packet when we've already processed a later ACK would be bogus. Hard to tell what Matt's intentions were. In the end the patch appears to be okay as it is. Nevertheless, my suggestion to comment this sufficiently still stands. Uwe -- Uwe Doering | EscapeBox - Managed On-Demand UNIX Servers gemini@geminix.org | http://www.escapebox.net