Date: Tue, 16 Jun 2009 10:04:42 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r193941 - head/sys/netinet Message-ID: <20090616091039.Q25629@delplex.bde.org> In-Reply-To: <200906151328.32279.jhb@freebsd.org> References: <200906101827.n5AIRFoR022115@svn.freebsd.org> <200906120849.07127.jhb@freebsd.org> <20090613094302.I22933@delplex.bde.org> <200906151328.32279.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090616091039.Q25629>