NetBSD Problem Report #57718

From www@netbsd.org  Wed Nov 22 03:13:11 2023
Return-Path: <www@netbsd.org>
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 F093F1A9238
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 22 Nov 2023 03:13:10 +0000 (UTC)
Message-Id: <20231122031310.05D151A9239@mollari.NetBSD.org>
Date: Wed, 22 Nov 2023 03:13:09 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: mstohz rounds down, not up, which leads to surprising bugs when it rounds to zero
X-Send-Pr-Version: www-1.0

>Number:         57718
>Category:       kern
>Synopsis:       mstohz rounds down, not up, which leads to surprising bugs when it rounds to zero
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Nov 22 03:15:00 +0000 2023
>Last-Modified:  Thu Nov 23 05:20:01 +0000 2023
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The NetBugsD Hertz
>Environment:
>Description:
If hz=50, then mstohz(10) returns 0.

This means wait forever with cv_timedwait, or with kpause (which trips an assertion instead because waiting forever with kpause is nonsense).

This also means that timeouts are too short, rather than too long.
>How-To-Repeat:
boot a kernel with hz=50, watch things crash and burn
>Fix:
Yes, please!

1. Audit all mstohz callers to determine whether they need rounding up or rounding down.  My guess: the vast majority need rounding up.

2. Either:
   (a) Change mstohz to round up, and fix any callers that actually needs rounding down.
   (b) Make a new name for the operation that rounds up and convert everything that needs rounding up to use it.

>Audit-Trail:
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@netbsd.org
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
     Message-ID:  <20231122031501.0F87D1A923C@mollari.NetBSD.org>

   | 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

From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
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 08:02:43 -0000 (UTC)

 kre@munnari.OZ.AU (Robert Elz) writes:

 >   | If hz=50, then mstohz(10) returns 0.
 >   | This also means that timeouts are too short, rather than too long.

 > 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).


 While there are places that work around the issue, there are others
 that do not. Auditing the code and cleaning up needs to happen with
 or without a change to mstohz().

 After that we should remove the assertion that waiting for 0 ticks is
 illegal. This case should be treated like the event had already occured.

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@netbsd.org
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 16:36:21 +0700

     Date:        Wed, 22 Nov 2023 08:05:01 +0000 (UTC)
     From:        mlelstv@serpens.de (Michael van Elst)
     Message-ID:  <20231122080501.548ED1A9239@mollari.NetBSD.org>

   |  While there are places that work around the issue, there are others
   |  that do not. Auditing the code and cleaning up needs to happen with
   |  or without a change to mstohz().

 Oh, absolutely, I agree.   The earlier message was just pointing out
 that there must have been some people who were already aware of this
 as a problem.   (I have found a couple more workarounds I didn't
 notice earlier).

 Certainly nothing which calls mstohz() appears to want a 0 result though,
 so:

   |  After that we should remove the assertion that waiting for 0 ticks is
   |  illegal. This case should be treated like the event had already occured.

 would largely be for new code (or that which I have not yet seen).
 Everything I have looked at so far wants there to be some delay, not nothing.
 But I am only getting started - maybe 10% through the list so far...

 From what I have seen so far, the uses (including indirect ones via other
 functions and macros) and so far things look to be all over the place.
 In the following, the assumption is that 0 is never wanted, regardless of
 what would otherwise happen...

 For some, rounding down looks to be best (for things like polling, it
 is generally better to poll slightly faster than expected, than slower,
 to avoid missing events) - for others rounding up would be better (for
 timeouts, and "wait for hardware" delays, waiting slightly longer
 rather than slightly shorter is better), and for quite a lot, it simply
 doesn't matter - that includes the ones that simply use a large (multi-second)
 timeout, where +/- 1 tick is irrelevant, but others which much smaller
 values, which are entirely arbitrary (like the schedueller intervals,
 where all that matters is that there are (non-zero) values, the actual
 sizes might affect performance, but they're tunable, so something can be
 selected which works, whichever way the calculation rounds).

 I am making a record of what I am seeing, which I will append here when it
 is done, so people can look at the ones they're familiar with, and determine
 better than I can, whether my assumptions are correct or not.

 kre

From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
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 09:55:36 -0000 (UTC)

 kre@munnari.OZ.AU (Robert Elz) writes:

 > From what I have seen so far, the uses (including indirect ones via other
 > functions and macros) and so far things look to be all over the place.
 > In the following, the assumption is that 0 is never wanted, regardless of
 > what would otherwise happen...

 If you round up, you will get 0 when you pass 0.


 > For some, rounding down looks to be best (for things like polling, it
 > is generally better to poll slightly faster than expected, than slower,

 Consistently waiting for "at least" the time (aka rounding up) is
 consistent with the world, where a delay will be extended when e.g.
 your machine is too slow or is occupied with something else. Code
 should always expect that a delay can take longer than expected.

 Of course, your input value (not the result of mstohz) could already
 be truncated. So being "slightly" faster than expected (less than the
 resolution of your time value) is an issue too, but often acceptable.

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@netbsd.org
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 17:39:52 +0700

     Date:        Wed, 22 Nov 2023 10:00:03 +0000 (UTC)
     From:        mlelstv@serpens.de (Michael van Elst)
     Message-ID:  <20231122100003.423D01A9239@mollari.NetBSD.org>

   |  If you round up, you will get 0 when you pass 0.

 Sure, and I should have been clearer - there are some cases where the
 parameter can be zero, but in all I have seen so far (just a couple)
 the code handles that specially, and tends to not even call mstohz()
 (or issue any kind of kpause/callout/...) in that case.

 The "expects not to get zero" was in the case the parameter was not 0,
 even if the code is clearly attempting to get the minimum delay it
 believes is possible (mstohz(10) is quite common).

 I see (but have not thoroughly examined yet) some mstohz(1) calls - where
 a 0 result is clearly not wanted (a KASSERT would fire if that happened)
 and so which can only possibly work if hz>=1000 (these are in arch/sparc64
 so that assumption is probably true by default, but hz is a kernel config
 setting, and even on sparc64 someone could set hz to < 1000).

   |  Consistently waiting for "at least" the time (aka rounding up) is
   |  consistent with the world,

 Yes, for timeouts, and user code, but for device polling one cannot
 usually just wait "anything longer than  ..." or events might be lost.
 Typically polling would prefer to simply busy loop, but that is "unkind"
 to the rest of the system, so tends to pause, but is almost always going
 to prefer a smaller wait to a longer one.

 kre

 ps: the 32 bit version of hztoms() in sys/param.h looks to be potentially
 broken, if HZ is set higher than 131072 (yes, I know, extremely unlikely
 on any 32 bit processor) it will fail, and will be returning inaccurate
 results for large hz values for HZ settings down to about 10000 (still
 unlikely on such a processor).    There probably should be some mention
 in the doc somewhere (config(8)?) that the HZ value needs to be sane on
 32 bit processors, along with some indication what sane means.

From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
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 11:28:17 -0000 (UTC)

 kre@munnari.OZ.AU (Robert Elz) writes:

 >   |  Consistently waiting for "at least" the time (aka rounding up) is
 >   |  consistent with the world,
 > 
 > Yes, for timeouts, and user code, but for device polling one cannot
 > usually just wait "anything longer than  ..." or events might be lost.

 In this environment, you cannot enforce a maximum wait time and
 "lost events" need to be handled gracefully. If that's not possible
 you need to use real-time systems, dedicated processors or other
 hardware that meets such a requirement.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57718 CVS commit: src/sys/kern
Date: Wed, 22 Nov 2023 13:18:49 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Nov 22 13:18:49 UTC 2023

 Modified Files:
 	src/sys/kern: kern_synch.c

 Log Message:
 kpause(9): KASSERT -> KASSERTMSG

 PR kern/57718 (might help to diagnose manifestations of the problem)


 To generate a diff of this commit:
 cvs rdiff -u -r1.365 -r1.366 src/sys/kern/kern_synch.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57718 CVS commit: src/sys/kern
Date: Wed, 22 Nov 2023 13:19:50 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Nov 22 13:19:50 UTC 2023

 Modified Files:
 	src/sys/kern: vfs_vnode.c

 Log Message:
 vfs(9): Make sure to kpause at least one tick, not zero.

 kpause(9) forbids zero.

 Local workaround for wider problem in PR kern/57718, to address
 immediate symptom of crash on any system with hz=50, e.g. alpha in
 qemu:

 panic: kernel diagnostic assertion "timo != 0 || intr" failed: file "/usr/src/sys/kern/kern_synch.c", line 249

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.150 -r1.151 src/sys/kern/vfs_vnode.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@netbsd.org
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 20:58:37 +0700

     Date:        Wed, 22 Nov 2023 11:30:03 +0000 (UTC)
     From:        mlelstv@serpens.de (Michael van Elst)
     Message-ID:  <20231122113003.BC74D1A9239@mollari.NetBSD.org>

   |  In this environment, you cannot enforce a maximum wait time and
   |  "lost events" need to be handled gracefully.

 Sure, but that generally means lost performance - in general we'd
 prefer to avoid the problem by not missing if possible (and certainly
 it isn't always).  But it is simply insane to force a longer delay just
 "on principle" in cases like this.

 kre

From: "J. Hannken-Illjes" <hannken@mailbox.org>
To: NetBSD GNATS <gnats-bugs@netbsd.org>
Cc: 
Subject: Re: PR/57718 CVS commit: src/sys/kern
Date: Wed, 22 Nov 2023 15:03:49 +0100

 --Apple-Mail=_B33B4409-2B3C-4706-B6AC-14BAFF787EEF
 Content-Transfer-Encoding: 7bit
 Content-Type: text/plain;
 	charset=us-ascii

 No pullups needed -- this kpause() is post-10.

 --
 J. Hannken-Illjes - hannken@mailbox.org

 --Apple-Mail=_B33B4409-2B3C-4706-B6AC-14BAFF787EEF
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
 	filename=signature.asc
 Content-Type: application/pgp-signature;
 	name=signature.asc
 Content-Description: Message signed with OpenPGP

 -----BEGIN PGP SIGNATURE-----

 iQIzBAEBCAAdFiEEyLVMkhxs8fxixv+2IOocBq6p/bMFAmVeCkUACgkQIOocBq6p
 /bOlpw//QFX5WOEDqpSLMSvmB9WOIbKVv4etaQ06oZPOXTJcgMj5lCAtCzB+5X78
 GsiVVaoC1bPPEJbHXwnDv8C2H6AMD7UXPGgXxzM3MaMuLYVtQeTNF7u7rqpGvCjF
 VwOWyJr3XtLBKNyVbIJAYfwWQmdT2mdUcJvaonvRj27JJiXM3oP/PPxmsGrNSSFg
 2tYwT453jV2iOF+fG0CLUlLWZP/e9DAe3gz/ZYmhyO3DxGRCog9CerXh3wCC3iBD
 QgewhS/T1boboLVJHP1dxslrmxF1o7T1jyz+uhI1zO/SCaKRuD01ckKgR9PtQqhh
 UOXaeaZAtdVWJ5tvZGBGOFwt6XezzAWRhMR1Jg8mgo5/ZDPvnURAvhRNlUoyDJUf
 3/f9ztZ/mTTtlm4o6gUw19AkfhnIh8ImmWbJ/XFcawauXezTkPTPwjx3Xphtg8jX
 RLncxqxX2BGqBfHS9KVlV0FmgyPbqfeWKL6L/+kNAL0ie53FejvGG7h7xE36WXp1
 C/iRLZYgLu6IB9YN6xvuCaWAQYsPgEDfdMQjskIRVkEMSOMcJ9htNtCg8hKPlH3N
 vFS/mSAr4cDi1rsJNIott0QZEbJemOu3LJlf551QfqsJLsyO04PZc8IUYd9dRe6A
 eAxpNborK8TUZfSgMNaEBsiHOQV9WRcSoUGmvtCGvJAIzgiRpew=
 =NrDb
 -----END PGP SIGNATURE-----

 --Apple-Mail=_B33B4409-2B3C-4706-B6AC-14BAFF787EEF--

From: Jason Thorpe <thorpej@me.com>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 campbell+netbsd@mumble.net
Subject: Re: kern/57718: mstohz rounds down, not up, which leads to surprising
 bugs when it rounds to zero
Date: Wed, 22 Nov 2023 08:55:33 -0800

 > On Nov 22, 2023, at 12:05 AM, Michael van Elst <mlelstv@serpens.de> =
 wrote:
 >=20
 > While there are places that work around the issue, there are others
 > that do not. Auditing the code and cleaning up needs to happen with
 > or without a change to mstohz().

 If we=E2=80=99re going to go though all of this, we we please transition =
 away from the tick-oriented interfaces and move to a deadline model?  It =
 would remove all of the ambiguity from the call sites.

 -- thorpej

From: matthew green <mrg@eterna23.net>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: re: kern/57718: mstohz rounds down, not up, which leads to surprising bugs when it rounds to zero
Date: Thu, 23 Nov 2023 07:43:34 +1100

 > >Fix:
 > Yes, please!

 yes please :-)

 long ago (90s) we didn't fail with hz=1 even.  :-)  this was
 useful for early bring up on sparc64.

 i've got a couple of places where i convert mstohz() to have
 a min(1, ..) but i never got around to looking closer.


 .mrg.

From: Robert Elz <kre@munnari.OZ.AU>
To: matthew green <mrg@eterna23.net>
Cc: gnats-bugs@netbsd.org, kern-bug-people@netbsd.org
Subject: Re: kern/57718: mstohz rounds down, not up, which leads to surprising bugs when it rounds to zero
Date: Thu, 23 Nov 2023 12:15:28 +0700

     Date:        Thu, 23 Nov 2023 07:43:34 +1100
     From:        matthew green <mrg@eterna23.net>
     Message-ID:  <21466.1700685814@splode.eterna23.net>

   | long ago (90s) we didn't fail with hz=1 even.  :-)  this was
   | useful for early bring up on sparc64.

 some of that renains.

   | i've got a couple of places where i convert mstohz() to have
   | a min(1, ..) but i never got around to looking closer.

 max() hopefully ... haven't spotted any using min() (fortunately!)

 kre

Home	PR Database Search