kern/57719: cargo cult XXXmstohz() idiocy

[kre%munnari.OZ.AU@localhost]

>Number:         57719
>Category:       kern
>Synopsis:       cargo cult XXXmstohz() idiocy
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Nov 22 15:05:00 +0000 2023
>Originator:     Robert Elz
>Release:        NetBSD 10.99.10
>Organization:
>Environment:
	Anything.

>Description:
	There are quite a few XXXmstohz() functions spread throughout the kernel
	source tree - these are more or less mstohz() but deal with the possibility
	that the arg might be 0 or negative (some of them allow -1 with special
	meaning).

	Their general form is (close enough to) always

		int
		XXXmstohz(int m)
		{
			int h = m;  
		 
			if (h > 0) {
				h = h * hz / 1000; 
				if (h == 0)
					h = 1000 / hz;
			} 
			return(h);
		} 

	A minor issue is the use of 'h' as a copy of 'm' - which is not
	necessary, that appears to have originated from the declaration
	of h having been
			regtister int h = m;
	in some earlier versions (and still in at least one of these).
	But that's irrelevant really, the compiler should optimise it
	all away, and (on most architectures) just use a register anyway.

	If h (or m) is > 0, we convert that to ticks, more or less
	open coding mstohz() - that's OK, the h = h * hz / 1000;
	is fine, and is exactly mstohz() for 64 bit architectures.
	We miss the overflow protected version of mstohz() that
	sys/param.h provides for 32 bit architectures (where the
	multiplication, could, perhaps, overflow) but in most of
	these that's unlikely.   Perhaps that line might be converted
	to simply use mstohz() - but that's not the issue here.

	After that, if h == 0, then we know hz < 1000, that's the
	only way that h * hz / 1000 can possibly be 0.

	An example would be if h (or m) == 5, and hz == 100, then
	5 * 100 / 1000 -> 500 / 1000 -> 0.

	In that case, the following line supplies a non-zero value
	to replace that 0 (must be non-zero as hz < 1000).  That's good.

	But the value it generates is nonsense.   That's bad.

	Consider the example again - hz == 100, so 1000/hz == 10.
	The caller asked for 5ms, the result is 10 ticks, which at
	100hz is 100ms.   Way too long.

	Even worse, if hz == 50, we again get 0 from the initial
	calc, but now 1000/hz is 20 - so 20 ticks, that's 400ms (@ 50hz).
	Even worse.

>How-To-Repeat:
	UTSL.

>Fix:
	Perhaps change (all instances in these (many) XXXmstohz functions)
	of that

		if (h == 0)
			h = 1000 / hz;

	into

		if (h == 0)
			h = 1;

	In the cases where h == 0 (here, not earlier), the caller requested a
	delay smaller than is possible to provide (given current mechanisms),
	so the sane response is to simply give it the smallest that we can.

	That's 1 tick.

	But because 1 tick tends to mean "until whenever the clock ticks next"
	which might be much less than 1/hz in some cases, in some of these it
	might be better to use 2 rather than 1, but a case by case analysis by
	someone who understands each environment in which these are used will
	be required for that.