Date: Tue, 21 Dec 2004 10:50:14 +0100 From: Uwe Doering <gemini@geminix.org> To: Dan Nelson <dnelson@allantgroup.com> Cc: FreeBSD-gnats-submit@FreeBSD.org Subject: Re: kern/75122: [PATCH] Incorrect inflight bandwidth calculation on first packet Message-ID: <41C7F1D6.7020108@geminix.org> In-Reply-To: <20041221065531.GA78451@dan.emsphone.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?41C7F1D6.7020108>