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

[Uwe Doering]

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: Sat, 18 Dec 2004 13:09:14 +0100

 Dan Nelson wrote:
 > Updated patch including Matt's recommended fix:
 > 
 > [...]
 > diff -u -p -r1.201.2.4 tcp_subr.c
 > --- tcp_subr.c	1 Dec 2004 05:37:28 -0000	1.201.2.4
 > +++ tcp_subr.c	16 Dec 2004 06:38:48 -0000
 > [...]
 > @@ -1922,29 +1922,42 @@ tcp_xmit_bandwidth_limit(struct tcpcb *t
 >  	}
 >  
 >  	/*
 > +	  * Validate the delta time.  If a connection is new or has been idle
 > +	  * a long time we have to reset the bandwidth calculator.
 > +	  */
 > +	save_ticks = ticks;
 > +	delta_ticks = save_ticks - tp->t_bw_rtttime;
 > +	if (tp->t_bw_rtttime == 0 || delta_ticks < 0 || delta_ticks > hz * 10) {
 > +		tp->t_bw_rtttime = ticks;
 > +		tp->t_bw_rtseq = ack_seq;
 > +		if (tp->snd_bandwidth == 0)
 > +			tp->snd_bandwidth = tcp_inflight_min;
 > +		return;
 > +	}
 > +	if (delta_ticks == 0)
 > +		return;
 > +	
 > +	/*
 > +	 * 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 correct approach would be to separate the handling of the 
 two cases "sequence number wrap-around" and "window update acks".  I 
 suggest to define another variable 'delta_bytes' and initialize it like 
 this:
 
    int delta_bytes;
    [...]
    delta_bytes = ack_seq - tp->t_bw_rtseq;
 
 Then compare it with zero where you compare 'delta_ticks' with zero, and 
 check for less than zero (wrap-around) where you check 'delta_ticks' for 
 less than zero.  Combine the respective test results with '||'.  Of 
 course, you can then use 'delta_bytes' for the bandwidth calculation 
 further down in the code, too.
 
     Uwe
 -- 
 Uwe Doering         |  EscapeBox - Managed On-Demand UNIX Servers
 gemini at geminix.org  |  http://www.escapebox.net