From owner-freebsd-net Sat Aug 17 11:36:29 2002 Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 497DF37B400; Sat, 17 Aug 2002 11:36:21 -0700 (PDT) Received: from apollo.backplane.com (apollo.backplane.com [216.240.41.2]) by mx1.FreeBSD.org (Postfix) with ESMTP id D258043E70; Sat, 17 Aug 2002 11:36:20 -0700 (PDT) (envelope-from dillon@apollo.backplane.com) Received: from apollo.backplane.com (localhost [127.0.0.1]) by apollo.backplane.com (8.12.5/8.12.4) with ESMTP id g7HIaKdc067941; Sat, 17 Aug 2002 11:36:20 -0700 (PDT) (envelope-from dillon@apollo.backplane.com) Received: (from dillon@localhost) by apollo.backplane.com (8.12.5/8.12.4/Submit) id g7HIaKZ3067940; Sat, 17 Aug 2002 11:36:20 -0700 (PDT) (envelope-from dillon) Date: Sat, 17 Aug 2002 11:36:20 -0700 (PDT) From: Matthew Dillon Message-Id: <200208171836.g7HIaKZ3067940@apollo.backplane.com> To: Nate Lawson Cc: freebsd-hackers@FreeBSD.ORG, freebsd-net@FreeBSD.ORG Subject: Re: Commit schedule for bandwidth delay product pipeline limiting for TCP References: Sender: owner-freebsd-net@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. :) : :> + 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? Yes, when ticks rolls over. This is on purpose and is designed to handle both the rollover condition and someone setting the time of day on the machine (though perhaps 'ticks' doesn't reverse index in that case, I wasn't sure). e.g. if ticks rolls over we fall through the conditional and proceed to the calculation, which resets t_bw_rtttime. :> + 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. It's a cheap way to do an exponential average. The >> 4 is a divide by 16. So the equivalent is: bw = (average_bw * 15 + bw) / 16 average_bw = bw; That should be more obvious to you... a long term exponential average of the bandwidth. :> + * 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. Too late, I just did the commit. I'll make the addition in my local tree. I expect to make more commits in the future to this segment of code and it will show up in one of those. :> + 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? No, this is just debugging code designed to limit the rate of kernel printfs. A tcp_inflight_debug of 1 limits the rate to 1/sec. 2 will be 2/sec. 3 will be 3/sec. It's just debugging code, it isn't meant to be 100% robust. :> + 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? No, because of the check above. Though I suppose in -current there could be a race. :> tcp_seq t_rtseq; /* sequence number being timed */ :> :> + int t_bw_rtttime; /* used for bandwidth calculation */ : :Does this need to be signed? I think you meant unsigned. No, it doesn't need to be unsigned. Hmm. though it's possible rollover will skew the 'bw' calculation for a few milliseconds. I'll look into that. -Matt Matthew Dillon To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message