From owner-freebsd-hackers Sat Aug 17 10:49:27 2002 Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2ACF337B400 for ; Sat, 17 Aug 2002 10:49:18 -0700 (PDT) Received: from rootlabs.com (root.org [67.118.192.226]) by mx1.FreeBSD.org (Postfix) with SMTP id 8414E43E3B for ; Sat, 17 Aug 2002 10:49:17 -0700 (PDT) (envelope-from nate@rootlabs.com) Received: (qmail 47464 invoked by uid 1000); 17 Aug 2002 17:49:18 -0000 Date: Sat, 17 Aug 2002 10:49:18 -0700 (PDT) From: Nate Lawson To: Matthew Dillon Cc: freebsd-hackers@FreeBSD.ORG, freebsd-net@FreeBSD.ORG Subject: Re: Commit schedule for bandwidth delay product pipeline limiting for TCP In-Reply-To: <200208170233.g7H2XgqG047569@apollo.backplane.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Comments below. Consider them only semi-informed. :) On Fri, 16 Aug 2002, Matthew Dillon wrote: > +void > +tcp_xmit_bandwidth_limit(struct tcpcb *tp, tcp_seq ack_seq) > +{ > + u_long bw; > + u_long bwnd; > + int save_ticks; > + > + /* > + * If inflight_enable is disabled in the middle of a tcp connection, > + * make sure snd_bwnd is effectively disabled. > + */ > + if (tcp_inflight_enable == 0) { > + tp->snd_bwnd = TCP_MAXWIN << TCP_MAX_WINSHIFT; > + tp->snd_bandwidth = 0; > + return; > + } > + > + /* > + * Figure out the bandwidth. Due to the tick granularity this > + * is a very rough number and it MUST be averaged over a fairly > + * long period of time. > + */ > + save_ticks = ticks; > + if ((u_int)(save_ticks - tp->t_bw_rtttime) < 1) > + return; If tp->t_bw_rtttime is greater than save_ticks, the unsigned compare will fail. Can this ever happen (say with the "tick count goes backwards" issue)? Why not do this as a signed compare and check for <= 0? > + bw = (int64_t)(ack_seq - tp->t_bw_rtseq) * hz / > + (save_ticks - tp->t_bw_rtttime); > + tp->t_bw_rtttime = save_ticks; > + tp->t_bw_rtseq = ack_seq; > + if (tp->t_bw_rtttime == 0) > + return; > + bw = ((int64_t)tp->snd_bandwidth * 15 + bw) >> 4; I'm not familiar with the paper -- where does 15 come from? I'm guessing this is a cheap way to perturb the lower bits. > + tp->snd_bandwidth = bw; > + > + /* > + * Calculate the semi-static bandwidth delay product, plus two maximal > + * segments. The additional slop puts us squarely in the sweet > + * spot and also handles the bandwidth run-up case. Without the > + * slop we could be locking ourselves into a lower bandwidth. > + * > + * Situations Handled: > + * (1) prevents over-queueing of packets on LANs, especially > + * high speed LANs, allowing larger TCP buffers to be > + * specified. > + * > + * (2) able to handle increased network loads (bandwidth drops > + * so bwnd drops). > + * > + * (3) Randomly changes the window size in order to force > + * bandwidth balancing between connections. > + */ > +#define USERTT ((tp->t_srtt + tp->t_rttbest) / 2) > + bwnd = (int64_t)bw * USERTT / (hz << TCP_RTT_SHIFT) + 2 * tp->t_maxseg; Would prefer an #undef USERTT after you're done with it. > + if (tcp_inflight_debug > 0) { > + static int ltime; What happens when multiple packets arrive at once. Can they make the calculation below fail? Do you want splimp or a lock on ltime? > + if ((u_int)(ticks - ltime) >= hz / tcp_inflight_debug) { Why the division by a debug enable/disable int? Is it possible for this to ever be 0 at this point? > + ltime = ticks; > + printf("%p bw %ld rttbest %d srtt %d bwnd %ld\n", > + tp, > + bw, > + tp->t_rttbest, > + tp->t_srtt, > + bwnd > + ); > + } > + } > + if ((long)bwnd < tcp_inflight_min) > + bwnd = tcp_inflight_min; > + if (bwnd > tcp_inflight_max) > + bwnd = tcp_inflight_max; > + if ((long)bwnd < tp->t_maxseg * 2) > + bwnd = tp->t_maxseg * 2; > + tp->snd_bwnd = bwnd; > +} > + > Index: netinet/tcp_var.h > =================================================================== > RCS file: /home/ncvs/src/sys/netinet/tcp_var.h,v > retrieving revision 1.82 > diff -u -r1.82 tcp_var.h > --- netinet/tcp_var.h 19 Jul 2002 18:27:39 -0000 1.82 > +++ netinet/tcp_var.h 21 Jul 2002 02:26:36 -0000 > > @@ -137,6 +139,9 @@ > int t_rtttime; /* round trip time */ > tcp_seq t_rtseq; /* sequence number being timed */ > > + int t_bw_rtttime; /* used for bandwidth calculation */ Does this need to be signed? > + tcp_seq t_bw_rtseq; /* used for bandwidth calculation */ > + > int t_rxtcur; /* current retransmit value (ticks) */ > u_int t_maxseg; /* maximum segment size */ > int t_srtt; /* smoothed round-trip time */ > @@ -144,6 +149,7 @@ > > int t_rxtshift; /* log(2) of rexmt exp. backoff */ > u_int t_rttmin; /* minimum rtt allowed */ > + u_int t_rttbest; /* best rtt we've seen */ > u_long t_rttupdated; /* number of times rtt sampled */ > u_long max_sndwnd; /* largest window peer has offered */ -Nate To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message