From owner-svn-src-head@freebsd.org Tue Sep 3 14:06:49 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 2EC00DCFAA; Tue, 3 Sep 2019 14:06:49 +0000 (UTC) (envelope-from yuripv@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [96.47.72.132]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "freefall.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 46N7zr25vKz4PwR; Tue, 3 Sep 2019 14:06:48 +0000 (UTC) (envelope-from yuripv@freebsd.org) Received: by freefall.freebsd.org (Postfix, from userid 1452) id E196D1ABDB; Tue, 3 Sep 2019 14:06:17 +0000 (UTC) X-Original-To: yuripv@localmail.freebsd.org Delivered-To: yuripv@localmail.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [96.47.72.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (Client CN "mx1.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by freefall.freebsd.org (Postfix) with ESMTPS id 1337F32C8; Sat, 13 Apr 2019 08:14:14 +0000 (UTC) (envelope-from owner-src-committers@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [96.47.72.132]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "freefall.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 496BF72A94; Sat, 13 Apr 2019 08:14:13 +0000 (UTC) (envelope-from owner-src-committers@freebsd.org) Received: by freefall.freebsd.org (Postfix, from userid 538) id 0DEC53248; Sat, 13 Apr 2019 08:14:13 +0000 (UTC) Delivered-To: src-committers@localmail.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (Client CN "mx1.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by freefall.freebsd.org (Postfix) with ESMTPS id 587993245; 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) 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 Precedence: bulk X-Loop: FreeBSD.org Sender: owner-src-committers@freebsd.org X-Rspamd-Queue-Id: 496BF72A94 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.97 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.97)[-0.969,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] Status: O X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Date: Tue, 03 Sep 2019 14:06:50 -0000 X-Original-Date: Sat, 13 Apr 2019 18:13:59 +1000 (EST) X-List-Received-Date: Tue, 03 Sep 2019 14:06:50 -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