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