Re: kern/57718: mstohz rounds down, not up, which leads to surprising bugs when it rounds to zero

[Robert Elz <kre%munnari.OZ.AU@localhost>]

The following reply was made to PR kern/57718; it has been noted by GNATS.

From: Robert Elz <kre%munnari.OZ.AU@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: kern/57718: mstohz rounds down, not up, which leads to surprising bugs when it rounds to zero
Date: Wed, 22 Nov 2023 14:28:03 +0700

     Date:        Wed, 22 Nov 2023 03:15:01 +0000 (UTC)
     From:        campbell+netbsd%mumble.net@localhost
     Message-ID:  <20231122031501.0F87D1A923C%mollari.NetBSD.org@localhost>
 
   | If hz=50, then mstohz(10) returns 0.
   | This also means that timeouts are too short, rather than too long.
 
 This issue has clearly already been discovered, as in arch/amiga/dev/siop.c
 we have:
 
 	mstohz(acb->xs->timeout) + 1
 
 where the +1 would make no sense, if mstohz() rounded up.
 
 Similarly dev/pci/ixgbe/ixgbe_netbsd.c
 	mstohz(us / 1000) + 1
 (which also handles the rounding down of the us/1000).
 
 And in dev/acpi/acpica/OsdSchedule.c
 
 	MAX(mstohz(Milliseconds), 1)
 
 and similarly in dev/pci/cxdtv.c dev/usb/ucom.c dev/usb/xhci.c
 
 except they do (not that it makes any difference):
 
 	MAX(1, mstohz(whatever))
 
 The case in ucom.c is being very protective:
 
 	ticks = MAX(1, MIN(INT_MAX, mstohz(ms)));
 
 So simply switching to round up, without adjusting at least the first of
 those (the +1's) wouldn't be a good idea (the ones using MAX() will work
 either way).
 
 There are plenty of calls where the rounding is unlikely to really
 matter (mstohz(2000) for example - 1 tick either way after 2 seconds
 shouldn't bother anything - even bigger constant args exist).
 
 Apart from those, there are more than 200 other calls to mstohz() in the
 kernel sources (not counting arch/amiga/dev/par.c's parmstohz() routine
 which handles the problem internally - I am not convinced of the correctness
 of its corresponding parhztoms() routine however).   Looking into all of
 the rest of those will take a while.
 
   | 2. Either:
   |    (a) Change mstohz to round up, and fix any callers that actually
   |        needs rounding down.
 
 I don't know if there are any of the latter yet, but there are those cases
 that know rounding down happens, and already handle it - the +1 versions would
 be broken by simply switching to rounding up, but the better fix is to ensure
 the arg is not 0, then if mstohz() rounds up, just remove the +1 (leaving
 just a constant 1 in the case the arg was 0).
 
 kre