svn commit: r346176 - head/sys/sys
Author: imp
Date: Sat Apr 13 04:46:35 2019
New Revision: 346176
URL: https://svnweb.freebsd.org/changeset/base/346176
Log:
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)
Modified:
head/sys/sys/time.h
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 ((10 * _sbt) >> 32);
+#ifdef KASSERT
+ KASSERT(_sbt >= 0, ("Negative values illegal for sbttons: %jx", _sbt));
+#endif
+ ns = _sbt;
+ if (ns >= SBT_1S)
+ ns = (ns >> 32) * 10;
+ else
+ ns = 0;
+
+ return (ns + (10 * (_sbt & 0xu) >> 32));
}
static __inline sbintime_t
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r346176 - head/sys/sys
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.
Old versions supported negative times. This was broken in 2 stages:
- use a buggy inline conversion function instead of a working macro
- break negative times further when fixing overflow bugs in the inline
function
- attempt to panic for negative times, but actually do nothing since
this is in unreachable code.
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 ((10 * _sbt) >> 32);
sbintime_t is a signed type so as to support negative times. This function
returns a signed type so as to support negative times.
The original sloppy and fast version with a macro worked for all negative
times, modulo the assumption that right shifts propagate the sign bit.
It just did _sbt >> 32.
The above slower and more accurate version might have worked for negative
times similarly, but this is less clear. E.g., if _sbt is -1 second, that
is 0x (-1) in the high 32 bits and 0 in the low 32 bits.
Multiplying this by 10**9 doesn't overflow since both terms are signed,
and gives a result near -1 in the high bits and the shift works before.
I think similarly for all negative values down to the overflow threshold
of about -2 seconds.
Similarly for the other functions -- the simple shifts just worked
(unportably), while all later versions have overflow and/or sign extension
bugs.
+#ifdef KASSERT
+ KASSERT(_sbt >= 0, ("Negative values illegal for sbttons: %jx", _sbt));
+#endif
KASSERT is never defined here except in code with style bugs. In the
kernel, sys/time.h is never included directly except in code with style
bugs. It is normally included as part of standard namespace pollution
in sys/param.h. Namespace pollution defeats ifdefs like this. KASSERT
is only defined in sys/systm.h (except...), so it is never defined when
sys/param.h includes sys/time.h (except in code with even worse style
bugs, such as including sys/systm.h unsorted before sys/systm.h...).
In userland, KASSERT is never defined here except in code with style bugs
(such as being chummy with the implementation and defining KASSERT to use
here) and in code to demonstrate the brokenness of this header (KASSERT is
in the application namespace, so the demonstration can define it as
'syntax error'.
+ ns = _sbt;
This corrupts the type from signed to unsigned.
This has no effect except to ensure breaking negative times. E.g., if
the time is not negative, then the conversion doesn't change its value.
Ottherwise, the conversion prevents propagation of the sign bit. E.g,
if the time is -1 then shifting right the signed value by 32 keeps it
as -1, but shifting right the unsigned value gives UINT32_MAX seconds =
UINT32_MAX * 10**9 nsec = about 1/4 of UINT64_MAX seconds. Since this
and all other possible values in ns are less than 1/4 UINT64_MAX, then
are also less than INT64_MAX, so conversion back to a signed type doesn't
overflow them back to a negative value.
+ if (ns >= SBT_1S)
+ ns = (ns >> 32) * 10;
+ else
+ ns = 0;
+
Style bug (extra blank line). In KNF, statements are separated by a
semilcolon and a single newline. An extra newline is especially bad
when it is used to separate closely related statements like here.
+ return (ns + (10 * (_sbt & 0xu) >> 32));
This seems to be correct, except for its style bugs of spelling the U
suffix in lower case. I don't like using the suffix, but the value
of plain 0x after promotion to the type of _sbt is unclear.
}
static __inline sbintime_t
Conversion in the opposite direction is more difficult and buggier. I
currently use the following cut down version with only the most important
function fixed:
XX Index: time.h
XX ===
XX --- time.h (revision 346046)
XX +++ time.h (working copy)
XX @@ -200,8 +200,20 @@
XX sb = (_ns / 10) * SBT_1S;
XX _ns = _ns % 10;
These divisons are very slow on i386, and not very fast on amd64. They
defeat the manual reductions to shifts and multiplications in the rest
of the code. This case starts unnecessarily early (at 1 second instead
of at the overflow threshold of 4+ seconds).
XX }
XX +#if 0
XX /* 9223372037 = ceil(2^63 / 10) */
XX sb += ((_ns * 9223372037ull) + 0x7fff) >> 31;
Re: svn commit: r346176 - head/sys/sys
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 ((10 * _sbt) >> 32);
* ...
+ ns = _sbt;
+ if (ns >= SBT_1S)
+ ns = (ns >> 32) * 10;
+ else
+ ns = 0;
+
...
+ return (ns + (10 * (_sbt & 0xu) >> 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 (10 * (_sbt >> 32) +
(10 * (_sbt & 0xU)) >> 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
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r346176 - head/sys/sys
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 ((10 * _sbt) >> 32);
* ...
+ ns = _sbt;
+ if (ns >= SBT_1S)
+ ns = (ns >> 32) * 10;
+ else
+ ns = 0;
+
...
+ return (ns + (10 * (_sbt & 0xu) >> 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 (10 * (_sbt >> 32) +
(10 * (_sbt & 0xU)) >> 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
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r346176 - head/sys/sys
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.
Old versions supported negative times. This was broken in 2 stages:
- use a buggy inline conversion function instead of a working macro
- break negative times further when fixing overflow bugs in the inline
function
- attempt to panic for negative times, but actually do nothing since
this is in unreachable code.
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 ((10 * _sbt) >> 32);
sbintime_t is a signed type so as to support negative times. This function
returns a signed type so as to support negative times.
The original sloppy and fast version with a macro worked for all negative
times, modulo the assumption that right shifts propagate the sign bit.
It just did _sbt >> 32.
The above slower and more accurate version might have worked for negative
times similarly, but this is less clear. E.g., if _sbt is -1 second, that
is 0x (-1) in the high 32 bits and 0 in the low 32 bits.
Multiplying this by 10**9 doesn't overflow since both terms are signed,
and gives a result near -1 in the high bits and the shift works before.
I think similarly for all negative values down to the overflow threshold
of about -2 seconds.
Similarly for the other functions -- the simple shifts just worked
(unportably), while all later versions have overflow and/or sign extension
bugs.
+#ifdef KASSERT
+ KASSERT(_sbt >= 0, ("Negative values illegal for sbttons: %jx", _sbt));
+#endif
KASSERT is never defined here except in code with style bugs. In the
kernel, sys/time.h is never included directly except in code with style
bugs. It is normally included as part of standard namespace pollution
in sys/param.h. Namespace pollution defeats ifdefs like this. KASSERT
is only defined in sys/systm.h (except...), so it is never defined when
sys/param.h includes sys/time.h (except in code with even worse style
bugs, such as including sys/systm.h unsorted before sys/systm.h...).
In userland, KASSERT is never defined here except in code with style bugs
(such as being chummy with the implementation and defining KASSERT to use
here) and in code to demonstrate the brokenness of this header (KASSERT is
in the application namespace, so the demonstration can define it as
'syntax error'.
+ ns = _sbt;
This corrupts the type from signed to unsigned.
This has no effect except to ensure breaking negative times. E.g., if
the time is not negative, then the conversion doesn't change its value.
Ottherwise, the conversion prevents propagation of the sign bit. E.g,
if the time is -1 then shifting right the signed value by 32 keeps it
as -1, but shifting right the unsigned value gives UINT32_MAX seconds =
UINT32_MAX * 10**9 nsec = about 1/4 of UINT64_MAX seconds. Since this
and all other possible values in ns are less than 1/4 UINT64_MAX, then
are also less than INT64_MAX, so conversion back to a signed type doesn't
overflow them back to a negative value.
+ if (ns >= SBT_1S)
+ ns = (ns >> 32) * 10;
+ else
+ ns = 0;
+
Style bug (extra blank line). In KNF, statements are separated by a
semilcolon and a single newline. An extra newline is especially bad
when it is used to separate closely related statements like here.
+ return (ns + (10 * (_sbt & 0xu) >> 32));
This seems to be correct, except for its style bugs of spelling the U
suffix in lower case. I don't like using the suffix, but the value
of plain 0x after promotion to the type of _sbt is unclear.
}
static __inline sbintime_t
Conversion in the opposite direction is more difficult and buggier. I
currently use the following cut down version with only the most important
function fixed:
XX Index: time.h
XX ===
XX --- time.h (revision 346046)
XX +++ time.h (working copy)
XX @@ -200,8 +200,20 @@
XX sb = (_ns / 10) * SBT_1S;
XX _ns = _ns % 10;
These divisons are very slow on i386, and not very fast on amd64. They
defeat the manual reductions to shifts and multiplications in the rest
of the code. This case starts unnecessarily early (at 1 second instead
of at the overflow threshold of 4+ seconds).
XX }
XX +#if 0
XX /* 9223372037 = ceil(2^63 / 10) */
XX sb += ((_ns * 9223372037ull) + 0x7fff) >> 31;
svn commit: r346176 - head/sys/sys
Author: imp
Date: Sat Apr 13 04:46:35 2019
New Revision: 346176
URL: https://svnweb.freebsd.org/changeset/base/346176
Log:
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)
Modified:
head/sys/sys/time.h
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 ((10 * _sbt) >> 32);
+#ifdef KASSERT
+ KASSERT(_sbt >= 0, ("Negative values illegal for sbttons: %jx", _sbt));
+#endif
+ ns = _sbt;
+ if (ns >= SBT_1S)
+ ns = (ns >> 32) * 10;
+ else
+ ns = 0;
+
+ return (ns + (10 * (_sbt & 0xu) >> 32));
}
static __inline sbintime_t
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
