Date: Sat, 18 Dec 2004 13:09:14 +0100 From: Uwe Doering <gemini@geminix.org> To: Dan Nelson <dnelson@allantgroup.com> Cc: FreeBSD-gnats-submit@FreeBSD.org Subject: Re: kern/75122: [PATCH] Incorrect inflight bandwidth calculation on first packet Message-ID: <41C41DEA.9000504@geminix.org> In-Reply-To: <20041216190608.GA21382@dan.emsphone.com> References: <200412151827.iBFIRqDB019997@dan.emsphone.com> <200412151830.iBFIUVwE052799@freefall.freebsd.org> <20041216190608.GA21382@dan.emsphone.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@geminix.org | http://www.escapebox.net
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?41C41DEA.9000504>