NetBSD Problem Report #57719
From kre@munnari.OZ.AU Wed Nov 22 15:03:08 2023
Return-Path: <kre@munnari.OZ.AU>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id CB2341A9238
for <gnats-bugs@gnats.NetBSD.org>; Wed, 22 Nov 2023 15:03:07 +0000 (UTC)
Message-Id: <202311221502.3AMF2sHv000719@jacaranda.noi.kre.to>
Date: Wed, 22 Nov 2023 22:02:54 +0700 (+07)
From: kre@munnari.OZ.AU
To: gnats-bugs@NetBSD.org
Subject: cargo cult XXXmstohz() idiocy
X-Send-Pr-Version: 3.95
>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.
(Contact us)
$NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2023
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.