kern/75122: [PATCH] Incorrect inflight bandwidth calculation on first packet

[Uwe Doering]

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