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

[Uwe Doering]

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