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>