Date: Sat, 11 Aug 2012 20:27:32 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: David Xu <davidxu@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r239187 - head/sys/kern Message-ID: <20120811181037.A853@besplex.bde.org> In-Reply-To: <201208110006.q7B06uPR092724@svn.freebsd.org> References: <201208110006.q7B06uPR092724@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 11 Aug 2012, David Xu wrote:
> Log:
> tvtohz will print out an error message if a negative value is given
> to it, avoid this problem by detecting timeout earlier.
>
> Reported by: pho
It's interesting that anyone noticed that.
In my version, the printf under DIAGNOSTIC (with prettyprinting of
negative times) is simplified to an unconditional panic (with no
prettyprinting). The panic never occurred here (but I don't have
POSIX timers).
However, I think I would go the other way and remove the diagnostic
and return 1 (tick). In nanosleep(), I objected to returning an error
for negative sleep times, and this is similar. A negative time is
just the result of subtracting times with the one that is normally
largest being smallest for some reason. Unless this must not happen
for technical reasons, it is easiest to let it happen and silently
handle the problem at the lowest level. For sleep intervals, negative
ones should be silently converted to 0. However, there is the technical
reason that 0 means infinity in the callout API, so negative sleep
intervals would have to be converted to 1 tick, and this is not as
good.
> Modified: head/sys/kern/kern_umtx.c
> ==============================================================================
> --- head/sys/kern/kern_umtx.c Fri Aug 10 21:11:00 2012 (r239186)
> +++ head/sys/kern/kern_umtx.c Sat Aug 11 00:06:56 2012 (r239187)
> ...
> @@ -601,6 +600,8 @@ abs_timeout_gethz(struct abs_timeout *ti
>
> tts = timo->end;
> timespecsub(&tts, &timo->cur);
> + if (tts.tv_sec < 0 || (tts.tv_sec == 0 && tts.tv_nsec == 0))
> + return (-1);
> return (tstohz(&tts));
> }
>
Style bug: home made comparison of timespecs. This should be written as:
% + if (timespeccmp(&timo->end, &timo->cur, whatever))
% + return (-1);
% tts = timo->end;
% timespecsub(&tts, &timo->cur);
% return (tstohz(&tts));
(except it shouldn't be written as a function at all -- see below).
> @@ -613,22 +614,25 @@ umtxq_sleep(struct umtx_q *uq, const cha
> {
> struct umtxq_chain *uc;
> int error;
> + int pulse;
Style bug: int declarations not on the same line.
>
> uc = umtxq_getchain(&uq->uq_key);
> UMTXQ_LOCKED_ASSERT(uc);
> for (;;) {
> if (!(uq->uq_flags & UQF_UMTXQ))
> return (0);
> - error = msleep(uq, &uc->uc_lock, PCATCH, wmesg,
> - timo == NULL ? 0 : abs_timeout_gethz(timo));
> - if (error != EWOULDBLOCK)
> - break;
> - umtxq_unlock(&uq->uq_key);
> - if (abs_timeout_update(timo)) {
> - error = ETIMEDOUT;
I see that is part of the spec to return immediately with error
ETIMEDOUT if the timeout expired. So we must check for both negative
and zero timeouts. The clean way to do this is to let them reach
msleep() and make it return EWOULDBLOCK immediately for them. Such
timeouts can only be the result of bugs or races, so they shouldn't
be optimized for with up-front checks before all calls to msleep().
Races turn negative timeouts into 1-2 tick ones anyway (this happens
when the up-front check is done a few nsec before the timeout expires;
then the check passes, but the timeout has expired before the sleep
begins).
This follows the null pointer in abs_timeout_update() if timo == NULL.
timo == NULL has caused msleep to be called with a timeout of 0 ticks
(infinity), so EWOULDBLOCK should not normally be returned in this
case. However, I think it is always returned after the INT_MAX ticks
since a timeout of infinity is not really infinite, but just INT_MAX,
and there is no further magic for this value, so EWOULDBLOCK is
returned as usual if the timeout expires.
> + if (timo != NULL) {
> + pulse = abs_timeout_gethz(timo);
> + if (pulse < 0)
> + return (ETIMEDOUT);
> + } else
> + pulse = 0;
> + error = msleep(uq, &uc->uc_lock, PCATCH|PDROP, wmesg, pulse);
Style bug: missing spaces around binary operator '|'.
Style bug: poor variable name 'pulse'. The good name 'timo' is taken,
and it is already confusing to use it for a point instead of for the
timeout in ticks. (The good name 'timeout' rotted much more to
'callout' in the main timeout API.)
Why is the lock dropped for just calling kern_clock_gettime()?
> + if (error != EWOULDBLOCK) {
> umtxq_lock(&uq->uq_key);
> break;
> }
> + abs_timeout_update(timo);
This still follows the null pointer, as above.
abs_timeout_update() is only called once, and now is just
kern_clock_gettime(). Manually inlining it would make its locking
requirements clearer.
abs_timeout_gethz() is only called once. This was convenient for using
it in an expression. But now this is just an obfuscation related to
'pulse'. After manually inlning it, the expression can use tstohz()
instead of it, and not need the 'pulse' variable to hold this value,
and not need to abuse 'pulse' to hold the error status.
> umtxq_lock(&uq->uq_key);
> }
> return (error);
>
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120811181037.A853>