From owner-svn-src-all@FreeBSD.ORG Tue Jun 16 00:04:47 2009 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3628710656E2; Tue, 16 Jun 2009 00:04:47 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id AF4248FC24; Tue, 16 Jun 2009 00:04:46 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-159-184.carlnfd1.nsw.optusnet.com.au (c122-106-159-184.carlnfd1.nsw.optusnet.com.au [122.106.159.184]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n5G04gHi010662 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 16 Jun 2009 10:04:43 +1000 Date: Tue, 16 Jun 2009 10:04:42 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: John Baldwin In-Reply-To: <200906151328.32279.jhb@freebsd.org> Message-ID: <20090616091039.Q25629@delplex.bde.org> References: <200906101827.n5AIRFoR022115@svn.freebsd.org> <200906120849.07127.jhb@freebsd.org> <20090613094302.I22933@delplex.bde.org> <200906151328.32279.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans Subject: Re: svn commit: r193941 - head/sys/netinet X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Jun 2009 00:04:48 -0000 On Mon, 15 Jun 2009, John Baldwin wrote: > On Friday 12 June 2009 8:56:56 pm Bruce Evans wrote: >> On Fri, 12 Jun 2009, John Baldwin wrote: > ... >>> FWIW, the variables were signed before they were changed to unsigned and are now >>> back as signed again. It just seems really odd to have the types not match >>> 'ticks' especially since many of the values are just cached copies of 'ticks'. >> >> No, they were originally u_long and stayed that way until you changed them >> (except possibly for newer variables). The type of `ticks' is just wrong, >> and fixing that would take more work, but there is no reason to propagate >> this bug to new variables. The original version is: > > Gah, I had misremembered the diffs, they were indeed unsigned. Here is my > attempt at trying to fix things then based on my understanding so far: This is about what I want. > - It changes the variables back to unsigned (but u_int instead of unsigned > long). It also changes a few other members to be u_int instead of int such > as the two rtttime members. > - I changed t_starttime in the timewait structure to u_int. > - I changed t_recent in the timewait structure to u_int32_t as it is only > used in relation to other u_int32_t variables. > - I also attempted to fix overflow errors with t_badrxtwin by using > subtraction and casting the result to an int to compare with zero instead > of doing a direct comparison. > - I cast 'ticks' to u_int in the code to compute a new isn. I'm not sure if > this is needed. This shouldn't be needed. The operands of any binary operation are promoted to a common type, and that type should be u_int since one of the operands should have type u_int even if `ticks' doesn't. It only makes sense to cast things both operands are cast in all binary operations where there _might_ , but that would be ugly and too careful. The types should be required to be not _too_ dissimilar (no mixing of ints with u_longs, and no opaque types so that we cannot tell if the types are dissimilar) so that the default promotions work right. > - Some style fixes to remove extra ()'s from 'ticks - t_rcvtime'. > > --- //depot/projects/smpng/sys/netinet/tcp_input.c 2009/06/12 13:45:50 > +++ //depot/user/jhb/socket/netinet/tcp_input.c 2009/06/15 17:22:13 > @@ -1296,7 +1296,7 @@ > * "bad retransmit" recovery. > */ > if (tp->t_rxtshift == 1 && > - ticks < tp->t_badrxtwin) { > + (int)(ticks - tp->t_badrxtwin) < 0) { > TCPSTAT_INC(tcps_sndrexmitbad); > tp->snd_cwnd = tp->snd_cwnd_prev; > tp->snd_ssthresh = This seems to be best for handling t_badrxtwin. I note that you don't cast `ticks' here or in most places. > --- //depot/projects/smpng/sys/netinet/tcp_timer.c 2009/04/14 19:06:19 > +++ //depot/user/jhb/socket/netinet/tcp_timer.c 2009/06/15 17:22:13 > @@ -418,8 +418,8 @@ > * backoff that we would use if retransmitting. > */ > if (tp->t_rxtshift == TCP_MAXRXTSHIFT && > - ((ticks - tp->t_rcvtime) >= tcp_maxpersistidle || > - (ticks - tp->t_rcvtime) >= TCP_REXMTVAL(tp) * tcp_totbackoff)) { > + (ticks - tp->t_rcvtime >= tcp_maxpersistidle || > + ticks - tp->t_rcvtime >= TCP_REXMTVAL(tp) * tcp_totbackoff)) { > TCPSTAT_INC(tcps_persistdrop); > tp = tcp_drop(tp, ETIMEDOUT); > goto out; Do we want to keep the non-KNF indentation (1 char extra to line things up)? > --- //depot/projects/smpng/sys/netinet/tcp_timewait.c 2009/06/09 15:15:22 > +++ //depot/user/jhb/socket/netinet/tcp_timewait.c 2009/06/15 17:22:13 > @@ -322,8 +322,10 @@ > tcp_seq new_irs = tw->irs; > > INP_INFO_WLOCK_ASSERT(&V_tcbinfo); > - new_iss += (ticks - tw->t_starttime) * (ISN_BYTES_PER_SECOND / hz); > - new_irs += (ticks - tw->t_starttime) * (MS_ISN_BYTES_PER_SECOND / hz); > + new_iss += ((u_int)ticks - tw->t_starttime) * > + (ISN_BYTES_PER_SECOND / hz); > + new_irs += ((u_int)ticks - tw->t_starttime) * > + (MS_ISN_BYTES_PER_SECOND / hz); This t_starttime (should be tw_starttime) is now u_int (was u_long), so you don't need the cast here -- the cast has no effect. When t_starttime was u_long, the safest cast would have been of t_starttime down to u_int. > --- //depot/projects/smpng/sys/netinet/tcp_var.h 2009/06/12 13:45:50 > +++ //depot/user/jhb/socket/netinet/tcp_var.h 2009/06/15 17:22:13 > @@ -139,12 +139,12 @@ > > u_int t_maxopd; /* mss plus options */ > > - int t_rcvtime; /* inactivity time */ > - int t_starttime; /* time connection was established */ > - int t_rtttime; /* round trip time */ > + u_int t_rcvtime; /* inactivity time */ > + u_int t_starttime; /* time connection was established */ > + u_int t_rtttime; /* round trip time */ I would prefer the delta-times (like t_rtttime) to remain as ints. Having everything of the same type is safer, and having everything of signed type allows better overflow checking (gcc -ftrapv (*)). Whether ints are actually safer for r_tttime depend on how it is used. Oops, I misread t_rtttime as being the rtt and a delta-time. It seems to be actually an absolute time for the start of rtt measurement. Its comment is thus now just wrong. (*) I bet kernels compiled with -ftrapv don't boot. They need a little from libgcc to compile. I haven't actually tried compiling them. -ftrapv is only documented to trap for overflow on "+-*" binary operations. I can't find anything like it to trap for overflow on other operations (other binary operations starting with division, ,assignment, conversions due to prototypes). gcc -trapv still generates large slow code with libcalls for "+-*" and doesn't generate the much smaller but possibly slower i386 "into" trap. ISTR that "into" only works for very basic operations too. Bruce