Re: mstohz and hztoms

[Christos Zoulas]

> On Sep 28, 2019, at 6:53 PM, matthew green  wrote:
> 
>> Comments?
> 
> i like the clean up.  it's clearly a step forward.
> 
> i only don't understand why 32 bit platforms can't handle
> large values here but 64 bit ones can.  is it only so that the
> 32 bit platforms don't use 64 bit maths when it's not needed?

There was a comment about not using 64 bit math on 32
bit platforms, but I don't understand why. It is not like those
are being called too frequently. Of course I might be missing
something...

> it just seems wrong to me to limit 32 bit artificially here,
> and it's not like it's _that_ difficult to overflow 32 bit hz.
> i run with HZ=1000 on some systems, like alpha does by
> default.  that gives you 49 days.  even with standard HZ=100
> it's only 16 months or so.  (hmm, i wonder if these macros
> compile nothing with HZ=1000 kernels.  be nice to confirm or
> add a hack :-)
> 
> can we make the 32 bit version smarter about accepting small
> values with 32 bit maths, but large or non-constant values
> with 64 bit maths?
> 

Yes, now that they are inline functions (we can safely do more
complex things.

> perhaps an explicit mstohz64() could handle the cases if we
> know they will exist, since most probably _know_ they are
> dealing with short intervals.
> 
> there's just something about the artificial limit here that
> is bugging me...

I will take a look at all the uses and come back with a proposal.

christos

re: mstohz and hztoms

[matthew green]

> Comments?

i like the clean up.  it's clearly a step forward.

i only don't understand why 32 bit platforms can't handle
large values here but 64 bit ones can.  is it only so that the
32 bit platforms don't use 64 bit maths when it's not needed?

it just seems wrong to me to limit 32 bit artificially here,
and it's not like it's _that_ difficult to overflow 32 bit hz.
i run with HZ=1000 on some systems, like alpha does by
default.  that gives you 49 days.  even with standard HZ=100
it's only 16 months or so.  (hmm, i wonder if these macros
compile nothing with HZ=1000 kernels.  be nice to confirm or
add a hack :-)

can we make the 32 bit version smarter about accepting small
values with 32 bit maths, but large or non-constant values
with 64 bit maths?

perhaps an explicit mstohz64() could handle the cases if we
know they will exist, since most probably _know_ they are
dealing with short intervals.

there's just something about the artificial limit here that
is bugging me...

thanks.


.mrg.

mstohz and hztoms

[Christos Zoulas]

Hi,

I was looking at the man page for hstohz and hztoms and their function
signatures say "int hztoms(int);" and "int mztohz(int);" where the
implementations use unsigned. We also have 64 bit specific implementations
only for sparc64 and amd64 and only for mstohz() and not for hztoms():

The following patch does the following:
1. documents the prototypes to take and return unsigned (should the 64
   bit ones be documented to return unsigned long or truncate)?
2. fixes the comparison to 0x2 to be unsigned like the other calculations
3. moves the 64 bit mstohz in  so all the 64 bit platforms
   can use it
4. adds an hztoms() 64 bit implementation
5. removes the port-specific mstohz() ones

Comments?

christos

Index: share/man/man9/mstohz.9
===
RCS file: /cvsroot/src/share/man/man9/mstohz.9,v
retrieving revision 1.11
diff -u -p -u -r1.11 mstohz.9
--- share/man/man9/mstohz.9 20 Oct 2011 10:36:42 -  1.11
+++ share/man/man9/mstohz.9 28 Sep 2019 12:47:21 -
@@ -24,7 +24,7 @@
 .\" SUCH DAMAGE.
 .\"
 .\"
-.Dd October 20, 2011
+.Dd September 28, 2019
 .Dt MSTOHZ 9
 .Os
 .Sh NAME
@@ -33,10 +33,10 @@
 .Nd convert between milliseconds and system clock ticks
 .Sh SYNOPSIS
 .In sys/param.h
-.Ft int
-.Fn mstohz "int ms"
-.Ft int
-.Fn hztoms "int hz"
+.Ft unsigned int
+.Fn mstohz "unsigned int ms"
+.Ft unsigned int
+.Fn hztoms "unsigned int hz"
 .Sh DESCRIPTION
 The
 .Fn mstohz
Index: sys/sys/param.h
===
RCS file: /cvsroot/src/sys/sys/param.h,v
retrieving revision 1.614
diff -u -p -u -r1.614 param.h
--- sys/sys/param.h 27 Sep 2019 00:32:03 -  1.614
+++ sys/sys/param.h 28 Sep 2019 12:47:21 -
@@ -488,21 +488,29 @@
 #ifdef _KERNEL
 /*
  * macro to convert from milliseconds to hz without integer overflow
- * Default version using only 32bits arithmetics.
- * 64bit port can define 64bit version in their 
- * 0x2 is safe for hz < 2
+ * The 32 bit version uses only 32bit arithmetic; 0x2 is safe for hz < 
2
+ * the 64 bit version does the computation directly.
  */
 #ifndef mstohz
-#define mstohz(ms) \
-   (__predict_false((ms) >= 0x2) ? \
-   ((ms +0u) / 1000u) * hz : \
-   ((ms +0u) * hz) / 1000u)
+# ifdef _LP64
+#  define mstohz(ms) ((ms + 0UL) * hz / 1000)
+# else
+#  define mstohz(ms) \
+   (__predict_false((ms + 0u) >= 0x2u) ? \
+   ((ms + 0u) / 1000u) * hz : \
+   ((ms + 0u) * hz) / 1000u)
+# endif
 #endif
+
 #ifndef hztoms
-#define hztoms(t) \
-   (__predict_false((t) >= 0x2) ? \
-   ((t +0u) / hz) * 1000u : \
-   ((t +0u) * 1000u) / hz)
+# ifdef _LP64
+#  define hztomz(t) (((t) + 0UL) * 1000u / hz)
+# else
+#  define hztoms(t) \
+   (__predict_false((t + 0u) >= 0x2u) ? \
+   ((t + 0u) / hz) * 1000u : \
+   ((t + 0u) * 1000u) / hz)
+# endif
 #endif
 
 #definehz2bintime(t)   (ms2bintime(hztoms(t)))
Index: sys/arch/amd64/include/param.h
===
RCS file: /cvsroot/src/sys/arch/amd64/include/param.h,v
retrieving revision 1.31
diff -u -p -u -r1.31 param.h
--- sys/arch/amd64/include/param.h  20 Aug 2019 12:33:04 -  1.31
+++ sys/arch/amd64/include/param.h  28 Sep 2019 12:47:21 -
@@ -126,8 +126,6 @@
 #define btop(x)x86_btop(x)
 #define ptob(x)x86_ptob(x)
 
-#define mstohz(ms) ((ms + 0UL) * hz / 1000)
-
 #else  /*  __x86_64__  */
 
 #include 
Index: sys/arch/sparc64/include/param.h
===
RCS file: /cvsroot/src/sys/arch/sparc64/include/param.h,v
retrieving revision 1.60
diff -u -p -u -r1.60 param.h
--- sys/arch/sparc64/include/param.h15 May 2019 16:59:10 -  1.60
+++ sys/arch/sparc64/include/param.h28 Sep 2019 12:47:21 -
@@ -229,11 +229,6 @@ extern voiddelay(unsigned int);
 #defineDELAY(n)delay(n)
 #endif /* __HIDE_DELAY */
 
-#ifdef __arch64__
-/* If we're using a 64-bit kernel use 64-bit math */
-#define mstohz(ms) ((ms + 0UL) * hz / 1000)
-#endif
-
 /* Keep this a const so compiler optimization is done */
 extern const int cputyp;