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

[Dan Nelson]

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

From: Dan Nelson <dnelson at allantgroup.com>
To: Uwe Doering <gemini at geminix.org>
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 00:55:31 -0600

 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.
 
 
 -- 
 	Dan Nelson
 	dnelson at allantgroup.com