From owner-svn-src-all@FreeBSD.ORG Fri Jun 12 12:59:24 2009 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A6637106564A; Fri, 12 Jun 2009 12:59:24 +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 774D78FC16; Fri, 12 Jun 2009 12:59:24 +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 2BA2546B1A; Fri, 12 Jun 2009 08:59:24 -0400 (EDT) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id E7FC28A06E; Fri, 12 Jun 2009 08:59:22 -0400 (EDT) From: John Baldwin To: Bruce Evans Date: Fri, 12 Jun 2009 08:49:06 -0400 User-Agent: KMail/1.9.7 References: <200906101827.n5AIRFoR022115@svn.freebsd.org> <200906111018.20703.jhb@freebsd.org> <20090612125702.M22046@delplex.bde.org> In-Reply-To: <20090612125702.M22046@delplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906120849.07127.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Fri, 12 Jun 2009 08:59:23 -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-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Jun 2009 12:59:25 -0000 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: > >> > >>> Log: > >>> Change a few members of tcpcb that store cached copies of ticks to be ints > >>> instead of unsigned longs. This fixes a few overflow edge cases on 64-bit > >>> platforms. Specifically, if an idle connection receives a packet shortly > >> > >> I think the variables should still be unsigned (ints now). Otherwise there > >> is at best benign undefined behaviour when the variables overflow at > >> INT_MAX, while the code is apparently designed to work with unsigned > >> values (it casts to int to look at differences between the unsigned values). > > > > 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'. > > Yes, I noticed the t_badrxtwin breakage but wasn't sure how best to fix it > > (or at least thought that it should be a separate followup since it was > > already broken with wraparound). I considered making it work more like > > the other variables such as t_rcvtime that merely store a cached value of > > 'ticks' and then use 'ticks - foo' and compare it with the given interval. > > This would entail renaming 't_badrxtwin' to 't_badrxttime' or some such. > > That seems clearer to me than '(int)(ticks - t_badrxtwin) < 0'. > > `ticks < t_limitvar' is more optimal than `ticks - t_startvar < max', but > harder to get right. Compilers even try to transform the latter to the > former (e.g., for loop counters), but they cannot do this if overflow > or wraparound is a possibility (except overflow gives undefined behaviour > so compilers can do anything). After fixing the former to > `(int)(ticks - t_limitvar) < 0' it has the same number of operations as > the latter and would only be more efficient if 0 is easier to load and/or > compare with than `max'. But casting to (int) seems clear to me -- it is > idiomatic for recovering signed differences from circular counters. Hmm, that is an idiom I am not familiar with I guess. -- John Baldwin