Date: Tue, 21 Dec 2004 12:20:28 +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: <41C806FC.7080500@geminix.org> In-Reply-To: <41C7F1D6.7020108@geminix.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?41C806FC.7080500>