kern/75122: [PATCH] Incorrect inflight bandwidth calculation
on first packet
Uwe Doering
gemini at geminix.org
Tue Dec 21 01:50:30 PST 2004
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 at geminix.org | http://www.escapebox.net
More information about the freebsd-bugs
mailing list