From owner-svn-src-all@freebsd.org Sat Apr 13 08:14:10 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6987115714BC; Sat, 13 Apr 2019 08:14:10 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 9B63972A8E; Sat, 13 Apr 2019 08:14:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 91D783D7AF0; Sat, 13 Apr 2019 18:14:00 +1000 (AEST) Date: Sat, 13 Apr 2019 18:13:59 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org cc: Warner Losh , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r346176 - head/sys/sys In-Reply-To: <20190413150641.N10265@besplex.bde.org> Message-ID: <20190413170441.N10576@besplex.bde.org> References: <201904130446.x3D4kabU042626@repo.freebsd.org> <20190413150641.N10265@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=FNpr/6gs c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=9cW_t1CCXrUA:10 a=kj9zAlcOel0A:10 a=EbWMYzAKIVLIQpZ_2q8A:9 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: 9B63972A8E X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.98 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.98)[-0.980,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 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: Sat, 13 Apr 2019 08:14:10 -0000 On Sat, 13 Apr 2019, Bruce Evans wrote: > On Sat, 13 Apr 2019, Warner Losh wrote: > >> Fix sbttons for values > 2s >> >> Add test against negative times. Add code to cope with larger values >> properly. >> >> Discussed with: bde@ (quite some time ago, for an earlier version) > > I am unhappy with previous attempted fixes in this area, and still have large > local patches. > ... >> Modified: head/sys/sys/time.h >> ============================================================================== >> --- head/sys/sys/time.h Sat Apr 13 04:42:17 2019 (r346175) >> +++ head/sys/sys/time.h Sat Apr 13 04:46:35 2019 (r346176) >> @@ -184,8 +184,18 @@ sbttobt(sbintime_t _sbt) >> static __inline int64_t >> sbttons(sbintime_t _sbt) >> { >> + uint64_t ns; >> >> - return ((1000000000 * _sbt) >> 32); >* ... >> + ns = _sbt; >> + if (ns >= SBT_1S) >> + ns = (ns >> 32) * 1000000000; >> + else >> + ns = 0; >> + >... >> + return (ns + (1000000000 * (_sbt & 0xffffffffu) >> 32)); Another style bug: extra parentheses moved from a useful place to a non- useful place. '*' has precedence over '>>', but this combination is unusual so the old code emphasized it using parentheses. '*' has precedence over '+', and this combination is usual so it shouldn't be emphasized using parentheses, but the new code does that and removes the more useful parentheses. >> } sbttous() and sbttoms() are still broken. sbtots() might be pessimized since it calls sbttons with a 32-bit arg that doesn't need the above complications (the compiler would be doing well to see that and replace ns by 0 in the above calculations. Another bug in the above and previous changes: ns is in the application namespace. Adding it defeats the care taken with naming the arg with an underscore. All inline functions in standard and sys headers have this problem. These are under __BSD_VISIBLE, but of course there is no documentation that this reserves the names ns, etc. Better fix based on looking at sbtots(): split _sbt into seconds and nanoseconds unconditially and reassemble using simple expressions. This is almost as efficient on 32-bit arches, since the seconds and nanoseconds fit in 32 bits so reassembly uses 2 32x32 -> 64-bit multiplications instead of 1 64x32 -> 64-bit multiplication. static __inline int64_t sbttons(sbintime_t _sbt) { #ifdef __tooslow if (__predict_false(_sbt == INT64_MIN) return (-1); /* round towards +-Inf to keep non-0 */ #endif #ifdef __maybenottooslow if (__predict_false(_sbt < 0)) return sbttons(-sbt); #endif return (1000000000 * (_sbt >> 32) + (1000000000 * (_sbt & 0xffffffffU)) >> 32); /* XXX rounding? */ } Handling negative values makes it about as messy and slow as the committed version :-(. Rounding is also a problem. It is normal to round times down, but that turns nonzero into zero which is bad for timeouts. The version in my previous reply always adds 1 mainly to avoid this. Conversion to a different representation and back gives double rounding down if rounding is always done, so rarely gives the original value. The file still has phk's comment about rounding down being required, and the changes to the sbintime conversion functions still ignore this. Bruce