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

Uwe Doering gemini at geminix.org
Tue Dec 21 02:00:44 PST 2004


The following reply was made to PR kern/75122; it has been noted by GNATS.

From: Uwe Doering <gemini at geminix.org>
To: Dan Nelson <dnelson at allantgroup.com>
Cc: FreeBSD-gnats-submit at FreeBSD.org, freebsd-bugs at FreeBSD.org
Subject: Re: kern/75122: [PATCH] Incorrect inflight bandwidth calculation
 on	first packet
Date: Tue, 21 Dec 2004 10:50:14 +0100

 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