Date: Mon, 21 Nov 2011 14:37:02 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Warner Losh <imp@bsdimp.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Hans Petter Selasky <hselasky@freebsd.org> Subject: Re: svn commit: r227749 - head/sys/kern Message-ID: <20111121134538.R1108@besplex.bde.org> In-Reply-To: <229B993A-8F52-42B3-AE87-7DB4451AA9D0@bsdimp.com> References: <201111200836.pAK8aIEq082864@svn.freebsd.org> <229B993A-8F52-42B3-AE87-7DB4451AA9D0@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 20 Nov 2011, Warner Losh wrote:
> Is this right? Passing 0 to timo causes a panic? That can't be good.
Because it reverses the natural order of conversations.
> On Nov 20, 2011, at 1:36 AM, Hans Petter Selasky wrote:
>> Log:
>> Given that the typical usage of pause() is pause("zzz", hz / N), where N can
>> be greater than hz in some cases, simply ignore a timeout value of zero.
>>
>> Suggested by: Bruce Evans
>> MFC after: 1 week
>>
>> Modified:
>> head/sys/kern/kern_synch.c
>>
>> Modified: head/sys/kern/kern_synch.c
>> ==============================================================================
>> --- head/sys/kern/kern_synch.c Sun Nov 20 08:29:23 2011 (r227748)
>> +++ head/sys/kern/kern_synch.c Sun Nov 20 08:36:18 2011 (r227749)
>> @@ -333,7 +333,7 @@ msleep_spin(void *ident, struct mtx *mtx
>> int
>> pause(const char *wmesg, int timo)
>> {
>> - KASSERT(timo > 0, ("pause: timo must be > 0"));
>> + KASSERT(timo >= 0, ("pause: timo must be >= 0"));
>>
>> /* silently convert invalid timeouts */
>> if (timo < 1)
I tried to get Hans to remove the KASSERT() and its panic in one of my
many reviews of changes to usb_pause() and pause(), but got confused in
my review of this commit. I said that this commit restores the old
KASSERT() which gives panics for a timeout of 0, but it actually
completes changing the KASSERT() so that it doesn't give panics in this
case. I got confused because the comments and the code are inconsistent
in different ways than before (and KASSERT() has its usual negative
logic complications):
- a previous comment says, verbosely, that "the timeout must be greater
than zero"
- the previous KASSERT() requires, consisely, that "timo must be > 0".
This was consistent with the comment. The new KASSERT() is inconsistent
with the comment.
- the "silently convert invalid timeouts" code, which was added on my
request in the previous commit, was unreachable in all cases that it
handles when INVARIANTS is configured. Now it is reachable for timeouts
of 0. It remains unreachable for timeouts of < 0. The verbose previous
comment is not as verbose as this mail, so it doesn't describe all the
cases.
timo isn't a function, so 0 can't be passed to it. Passing a timeout of
0 to various functions has various behaviours:
- passing a timeout of 0 to msleep() is supported and means no timeout
(= a timeout of infinity). (Negative timeouts don't seem to be
properly handled by msleep(). They are neither rejected not silently
fixed up, but are passed on.)
- passing a timeout of 0 (or < 0) to callout_reset() makes no sense,
but is not considered harmful enough the panic on. It was silently
fixed up to be a timeout of 1.
- passing a timeout of 0 (or < 0) to pause() makes no sense, but was
considered to be harmful enough to panic on for a timeout of 0, while
negative timeouts were just passed on.
(pause()'s main comment says that it is like tsleep(), but it is
really like DELAY(). Thus its timeout isn't really a (maximum)
timeout, but is a (minimum) DELAY(). Another minor problem with
the API or documentation is that it isn't clear if the caller is
responsible for handling the implementation detail that pause(msg,
1) only delays for a fractional tick iff it is implemented using
tsleep(); the caller must ask for a "timeout" of of 2 if it wants
a delay of strictly >= 1 tick. Returning early is OK for a
maximum timeout but not for a minimum delay. Non-hardware callers
probably don't care about getting a minimum, but usb callers do.
usb_pause() has always adds 1 tick to handle this and rounding
errors.)
After Hans' first change, negative timeouts were considered harmful
enough to panic on too. I thought that this was silly, since pause()
is unimportant compared with callout_reset() -- who cares if it is
passed an invalid timeout? -- and asked him to change to the silent
fixup used by callout_reset(). This is now done for timeouts of 0,
but not for timeouts of < 0 in the INVARIANTS case (since the KASSERT()
is still there for them). Checking for timeouts of < 0 in pause() is
getting close to checking for parity errors in the CPU in pause().
msleep() is much more important, but never did either.
pause() grew some additional complications that I don't like. Hans
noticed that DELAY(n) overflows for large n on arm. So pause() now
avoids passing large n to DELAY(). Probably no other callers of
DELAY() do this, and no callers of pause() need large n for DELAY().
Why is top posting considered harmful?
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111121134538.R1108>