Date: Sat, 13 Jun 2009 10:56:56 +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: <20090613094302.I22933@delplex.bde.org> In-Reply-To: <200906120849.07127.jhb@freebsd.org> References: <200906101827.n5AIRFoR022115@svn.freebsd.org> <200906111018.20703.jhb@freebsd.org> <20090612125702.M22046@delplex.bde.org> <200906120849.07127.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
% Index: tcp_var.h
% ===================================================================
% RCS file: /home/ncvs/src/sys/netinet/tcp_var.h,v
% retrieving revision 1.51
% retrieving revision 1.52
% diff -u -2 -r1.51 -r1.52
% --- tcp_var.h 28 Aug 1999 00:49:33 -0000 1.51
% +++ tcp_var.h 30 Aug 1999 21:17:07 -0000 1.52
% ...
% @@ -99,10 +103,10 @@
% u_int t_maxopd; /* mss plus options */
%
% - u_int t_idle; /* inactivity time */
This even had a reasonable type.
% - u_long t_duration; /* connection duration */
Maybe copying this type is responsible for the sizing bug.
% - int t_rtt; /* round trip time */
This was a short in FreeBSD-1 and presumably in Net/2. Since it is unrelated
to `ticks' and doesn't need to exceed INT_MAX (or maybe even SHRT_MAX), a
signed type is best for it.
% + u_long t_rcvtime; /* inactivity time */
% + u_long t_starttime; /* time connection was established */
New variables.
% + int t_rtttime; /* round trip time */
This was t_rtt in the above. This commit just regressed its name (to a
more verbose one with a bogus extra "t" ("ttime" = "time time"). There is
now also a t_bw_rtttime with the same bugs. Other rtt variables are
still spelled correctly, but their types are all over the map: from Mar
30 sources:
| icmp6.h:struct rttimer;
| sctp_structs.h: uint32_t prev_rtt;
| sctp_structs.h: uint8_t do_rtt;
| sctp_uio.h: uint32_t spinfo_srtt;
| sctp_uio.h: uint32_t rtt;
| sctp_uio.h: uint32_t sctps_fastretransinrtt; /* number of multiple FR in a
| tcp.h: u_int32_t tcpi_rtt; /* Smoothed RTT in usecs. */
| tcp.h: u_int32_t tcpi_rttvar; /* RTT variance in usecs. */
| tcp.h: u_int32_t __tcpi_rcv_rtt;
| tcp_hostcache.h: u_long rmx_rtt; /* estimated round trip time */
| tcp_hostcache.h: u_long rmx_rttvar; /* estimated rtt variance */
| tcp_timer.h: * up in the srtt because the timestamp is often calculated at
| tcp_var.h: u_long t_starttime; /* time connection was established */
| tcp_var.h: int t_rtttime; /* round trip time */
| tcp_var.h: int t_bw_rtttime; /* used for bandwidth calculation */
| tcp_var.h: int t_srtt; /* smoothed round-trip time */
| tcp_var.h: int t_rttvar; /* variance in round-trip time */
This was short in FreeeBSD-1.
| tcp_var.h: u_int t_rttmin; /* minimum rtt allowed */
This was u_short in FreeeBSD-1.
| tcp_var.h: u_int t_rttbest; /* best rtt we've seen */
| tcp_var.h: u_long t_rttupdated; /* number of times rtt sampled */
| tcp_var.h: int t_rttlow; /* smallest observerved RTT */
| tcp_var.h: u_long rmx_rtt; /* estimated round trip time */
| tcp_var.h: u_long rmx_rttvar; /* estimated rtt variance */
| tcp_var.h: u_long t_starttime;
| tcp_var.h: * With these scales, srtt has 3 bits to the right of the binary point,
| tcp_var.h: * and thus an "ALPHA" of 0.875. rttvar has 2 bits to the right of the
| tcp_var.h:#define TCP_RTT_SCALE 32 /* multiplier for srtt; 3 bits frac. */
| tcp_var.h:#define TCP_RTT_SHIFT 5 /* shift for srtt; 3 bits frac. */
| tcp_var.h:#define TCP_RTTVAR_SCALE 16 /* multiplier for rttvar; 2 bits */
| tcp_var.h:#define TCP_RTTVAR_SHIFT 4 /* shift for rttvar; 2 bits */
| tcp_var.h: * The initial retransmission should happen at rtt + 4 * rttvar.
| tcp_var.h: * Because of the way we do the smoothing, srtt and rttvar
| tcp_var.h: max((tp)->t_rttmin, (((tp)->t_srtt >> (TCP_RTT_SHIFT - TCP_DELTA_SHIFT)) \
| tcp_var.h: + (tp)->t_rttvar) >> TCP_DELTA_SHIFT)
| tcp_var.h: u_long tcps_segstimed; /* segs where we tried to get rtt */
| tcp_var.h: u_long tcps_rttupdated; /* times we succeeded */
| tcp_var.h: u_long tcps_cachedrtt; /* times cached RTT in route updated */
| tcp_var.h: u_long tcps_cachedrttvar; /* times cached rttvar updated */
| tcp_var.h: u_long tcps_usedrtt; /* times RTT initialized from route */
| tcp_var.h: u_long tcps_usedrttvar; /* times RTTVAR initialized from rt */
| tcp_var.h: { "rttdflt", CTLTYPE_INT }, \
| vinet.h: int _tcp_inflight_rttthresh;
| vinet.h:#define V_tcp_inflight_rttthresh VNET_INET(tcp_inflight_rttthresh)
Back to the old diff:
% tcp_seq t_rtseq; /* sequence number being timed */
%
% - int t_rxtcur; /* current retransmit value */
% + int t_rxtcur; /* current retransmit value (ticks) */
This was short in FreeBSD-1. short would probably have remained enough
with HZ=100. Less probably with HZ=1000.
% u_int t_maxseg; /* maximum segment size */
% int t_srtt; /* smoothed round-trip time */
% @@ -132,4 +136,8 @@
% tcp_cc cc_send; /* send connection count */
% tcp_cc cc_recv; /* receive connection count */
% +/* experimental */
% + u_long snd_cwnd_prev; /* cwnd prior to retransmit */
% + u_long snd_ssthresh_prev; /* ssthresh prior to retransmit */
% + u_long t_badrxtwin; /* window for retransmit recovery */
% };
Probably an error for these to be u_long. However, in old times people
worried more about int being 16 bits and used long too much. Using fixed-
width types would be worse.
%
% ...
I checked more closely for uses of these variables and found:
- most uses of t_rcvtime are of the form:
if ((ticks - tp->t_rcvtime) >= tcp_keepidle + tcp_maxidle)
This is safe with int variables modulo its undefined behaviour being
benign. The expression "ticks - tp->t_rcvtime" is almost always
parenthesized, as would be needed if it were cast, but this is
especially bogus when it is compared with an unparenthesized expression
as above.
- t_starttime is also a struct member in struct tcptw. (struct tcptw has
many related style bugs. Only 5 of its members have a name beginning
with tw_; 5 have no apparent prefix at all, 1 has an apparent ts_
prefix that is not a prefix; t_recent has the same confusing prefix as
t_starttime).
- struct tcpcb has many bugs of the same type :-(. The recently changed
ts_recent_age is one (ts_ only appears to be a prefix), and near it
is the unprefixed request_r_scale...
- Apart from the above style bugs, the ofuscated t_starttime in struct
tcptw gives another instance of the bug that you just fixed. It still
has type u_long (you didn't touch it), and is used in the problematic
way: from tcp_timewait.c:
% tw->t_starttime = tp->t_starttime;
% ...
% int
% tcp_twrecycleable(struct tcptw *tw)
% {
% tcp_seq new_iss = tw->iss;
% tcp_seq new_irs = tw->irs;
%
% INP_INFO_WLOCK_ASSERT(&tcbinfo);
% new_iss += (ticks - tw->t_starttime) * (ISN_BYTES_PER_SECOND / hz);
% new_irs += (ticks - tw->t_starttime) * (MS_ISN_BYTES_PER_SECOND / hz);
This gives the usual problem when tw_starttime was set from `ticks' before
`ticks' overflowed (ticks < INT_MAX), but `ticks' has now overflowed to be
negative.
%
% if (SEQ_GT(new_iss, tw->snd_nxt) && SEQ_GT(new_irs, tw->rcv_nxt))
% return (1);
% else
% return (0);
% }
- there are 2 broken comparisons involving t_badrxtwin altogether: from
tcp_input.c:
% if (tp->t_rxtshift == 1 &&
% ticks < tp->t_badrxtwin) {
% ...
% if (tp->t_rxtshift == 1 && ticks < tp->t_badrxtwin) {
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090613094302.I22933>