From owner-freebsd-bugs@FreeBSD.ORG Tue Dec 21 11:20:38 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 D697A16A4CE; Tue, 21 Dec 2004 11:20:38 +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 7D99543D1D; Tue, 21 Dec 2004 11:20:38 +0000 (GMT) (envelope-from gemini@geminix.org) Message-ID: <41C806FC.7080500@geminix.org> Date: Tue, 21 Dec 2004 12:20:28 +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> <41C7F1D6.7020108@geminix.org> In-Reply-To: <41C7F1D6.7020108@geminix.org> 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 1Cgi47-000HJV-00; Tue, 21 Dec 2004 12:20:31 +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 11:20:39 -0000 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