Date: Tue, 03 Sep 2019 14:06:50 -0000 From: Bruce Evans <brde@optusnet.com.au> Cc: Warner Losh <imp@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r346176 - head/sys/sys Message-ID: <20190413170441.N10576@besplex.bde.org> In-Reply-To: <20190413150641.N10265@besplex.bde.org> References: <201904130446.x3D4kabU042626@repo.freebsd.org> <20190413150641.N10265@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190413170441.N10576>