Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 15 Jun 2009 13:28:31 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Bruce Evans <brde@optusnet.com.au>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r193941 - head/sys/netinet
Message-ID:  <200906151328.32279.jhb@freebsd.org>
In-Reply-To: <20090613094302.I22933@delplex.bde.org>
References:  <200906101827.n5AIRFoR022115@svn.freebsd.org> <200906120849.07127.jhb@freebsd.org> <20090613094302.I22933@delplex.bde.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 12 June 2009 8:56:56 pm Bruce Evans wrote:
> On Fri, 12 Jun 2009, John Baldwin wrote:
> 
> > On Thursday 11 June 2009 11:12:58 pm Bruce Evans wrote:
> >> On Thu, 11 Jun 2009, John Baldwin wrote:
> >>
> >>> On Wednesday 10 June 2009 11:53:16 pm Bruce Evans wrote:
> >>>> On Wed, 10 Jun 2009, John Baldwin wrote:
> >>>
> >>> I wanted to match 'ticks' and figured changing 'ticks' was a far larger
> >>> headache.
> >>
> >> By changing the signedness you get undefined behaviour for the other types
> >> too, and risk new and/or different sign extension bugs.
> >
> > 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:
- 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.
- 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 =
@@ -2253,7 +2253,7 @@
 		 * original cwnd and ssthresh, and proceed to transmit where
 		 * we left off.
 		 */
-		if (tp->t_rxtshift == 1 && ticks < tp->t_badrxtwin) {
+		if (tp->t_rxtshift == 1 && (int)(ticks - tp->t_badrxtwin) < 0) {
 			TCPSTAT_INC(tcps_sndrexmitbad);
 			tp->snd_cwnd = tp->snd_cwnd_prev;
 			tp->snd_ssthresh = tp->snd_ssthresh_prev;
--- //depot/projects/smpng/sys/netinet/tcp_output.c	2009/06/09 15:15:22
+++ //depot/user/jhb/socket/netinet/tcp_output.c	2009/06/15 17:22:13
@@ -172,7 +172,7 @@
 	 * to send, then transmit; otherwise, investigate further.
 	 */
 	idle = (tp->t_flags & TF_LASTIDLE) || (tp->snd_max == tp->snd_una);
-	if (idle && (ticks - tp->t_rcvtime) >= tp->t_rxtcur) {
+	if (idle && ticks - tp->t_rcvtime >= tp->t_rxtcur) {
 		/*
 		 * We have been idle for "a while" and no acks are
 		 * expected to clock out any data we send --
--- //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
@@ -254,7 +254,7 @@
 		tp = tcp_close(tp);             
 	} else {
 		if (tp->t_state != TCPS_TIME_WAIT &&
-		   (ticks - tp->t_rcvtime) <= tcp_maxidle)
+		   ticks - tp->t_rcvtime <= tcp_maxidle)
 		       callout_reset(&tp->t_timers->tt_2msl, tcp_keepintvl,
 				     tcp_timer_2msl, tp);
 	       else
@@ -318,7 +318,7 @@
 		goto dropit;
 	if ((always_keepalive || inp->inp_socket->so_options & SO_KEEPALIVE) &&
 	    tp->t_state <= TCPS_CLOSING) {
-		if ((ticks - tp->t_rcvtime) >= tcp_keepidle + tcp_maxidle)
+		if (ticks - tp->t_rcvtime >= tcp_keepidle + tcp_maxidle)
 			goto dropit;
 		/*
 		 * Send a packet designed to force a response
@@ -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;
--- //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);
 
 	if (SEQ_GT(new_iss, tw->snd_nxt) && SEQ_GT(new_irs, tw->rcv_nxt))
 		return (1);
--- //depot/projects/smpng/sys/netinet/tcp_usrreq.c	2009/06/12 13:45:50
+++ //depot/user/jhb/socket/netinet/tcp_usrreq.c	2009/06/15 17:22:13
@@ -1823,11 +1823,11 @@
 	    tp->snd_recover);
 
 	db_print_indent(indent);
-	db_printf("t_maxopd: %u   t_rcvtime: %d   t_startime: %d\n",
+	db_printf("t_maxopd: %u   t_rcvtime: %u   t_startime: %u\n",
 	    tp->t_maxopd, tp->t_rcvtime, tp->t_starttime);
 
 	db_print_indent(indent);
-	db_printf("t_rttime: %d   t_rtsq: 0x%08x   t_bw_rtttime: %d\n",
+	db_printf("t_rttime: %u   t_rtsq: 0x%08x   t_bw_rtttime: %u\n",
 	    tp->t_rtttime, tp->t_rtseq, tp->t_bw_rtttime);
 
 	db_print_indent(indent);
@@ -1854,7 +1854,7 @@
 	    tp->snd_scale, tp->rcv_scale, tp->request_r_scale);
 
 	db_print_indent(indent);
-	db_printf("ts_recent: %u   ts_recent_age: %d\n",
+	db_printf("ts_recent: %u   ts_recent_age: %u\n",
 	    tp->ts_recent, tp->ts_recent_age);
 
 	db_print_indent(indent);
@@ -1863,7 +1863,7 @@
 
 	db_print_indent(indent);
 	db_printf("snd_ssthresh_prev: %lu   snd_recover_prev: 0x%08x   "
-	    "t_badrxtwin: %d\n", tp->snd_ssthresh_prev,
+	    "t_badrxtwin: %u\n", tp->snd_ssthresh_prev,
 	    tp->snd_recover_prev, tp->t_badrxtwin);
 
 	db_print_indent(indent);
@@ -1877,7 +1877,7 @@
 	/* Skip sackblks, sackhint. */
 
 	db_print_indent(indent);
-	db_printf("t_rttlow: %d   rfbuf_ts: %u   rfbuf_cnt: %d\n",
+	db_printf("t_rttlow: %u   rfbuf_ts: %u   rfbuf_cnt: %d\n",
 	    tp->t_rttlow, tp->rfbuf_ts, tp->rfbuf_cnt);
 }
 
--- //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 */
 	tcp_seq	t_rtseq;		/* sequence number being timed */
 
-	int	t_bw_rtttime;		/* used for bandwidth calculation */
+	u_int	t_bw_rtttime;		/* used for bandwidth calculation */
 	tcp_seq	t_bw_rtseq;		/* used for bandwidth calculation */
 
 	int	t_rxtcur;		/* current retransmit value (ticks) */
@@ -167,7 +167,7 @@
 	u_char	rcv_scale;		/* window scaling for recv window */
 	u_char	request_r_scale;	/* pending window scaling */
 	u_int32_t  ts_recent;		/* timestamp echo data */
-	int	ts_recent_age;		/* when last updated */
+	u_int	ts_recent_age;		/* when last updated */
 	u_int32_t  ts_offset;		/* our timestamp offset */
 
 	tcp_seq	last_ack_sent;
@@ -175,7 +175,7 @@
 	u_long	snd_cwnd_prev;		/* cwnd prior to retransmit */
 	u_long	snd_ssthresh_prev;	/* ssthresh prior to retransmit */
 	tcp_seq	snd_recover_prev;	/* snd_recover prior to retransmit */
-	int	t_badrxtwin;		/* window for retransmit recovery */
+	u_int	t_badrxtwin;		/* window for retransmit recovery */
 	u_char	snd_limited;		/* segments limited transmitted */
 /* SACK related state */
 	int	snd_numholes;		/* number of holes seen by sender */
@@ -187,7 +187,7 @@
 	tcp_seq sack_newdata;		/* New data xmitted in this recovery
 					   episode starts at this seq number */
 	struct sackhint	sackhint;	/* SACK scoreboard hint */
-	int	t_rttlow;		/* smallest observerved RTT */
+	u_int	t_rttlow;		/* smallest observerved RTT */
 	u_int32_t	rfbuf_ts;	/* recv buffer autoscaling timestamp */
 	int	rfbuf_cnt;		/* recv buffer autoscaling byte count */
 	void	*t_pspare[3];		/* toe usrreqs / toepcb * / congestion algo / 1 general use */
@@ -306,9 +306,9 @@
 	u_short		last_win;	/* cached window value */
 	u_short		tw_so_options;	/* copy of so_options */
 	struct ucred	*tw_cred;	/* user credentials */
-	u_long		t_recent;
+	u_int32_t	t_recent;
 	u_int32_t	ts_offset;	/* our timestamp offset */
-	u_long		t_starttime;
+	u_int		t_starttime;
 	int		tw_time;
 	TAILQ_ENTRY(tcptw) tw_2msl;
 };


-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906151328.32279.jhb>