svn commit: r193941 - head/sys/netinet

[Bruce Evans]

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