From owner-svn-src-head@FreeBSD.ORG Mon Jun 15 17:28:50 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D24FF1065674; Mon, 15 Jun 2009 17:28:50 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 96F398FC12; Mon, 15 Jun 2009 17:28:50 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 3190E46B7F; Mon, 15 Jun 2009 13:28:50 -0400 (EDT) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 01A378A070; Mon, 15 Jun 2009 13:28:49 -0400 (EDT) From: John Baldwin To: Bruce Evans Date: Mon, 15 Jun 2009 13:28:31 -0400 User-Agent: KMail/1.9.7 References: <200906101827.n5AIRFoR022115@svn.freebsd.org> <200906120849.07127.jhb@freebsd.org> <20090613094302.I22933@delplex.bde.org> In-Reply-To: <20090613094302.I22933@delplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906151328.32279.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Mon, 15 Jun 2009 13:28:49 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r193941 - head/sys/netinet X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Jun 2009 17:28:51 -0000 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