From owner-svn-src-all@FreeBSD.ORG Sat Aug 11 10:27:44 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id C37E7106564A; Sat, 11 Aug 2012 10:27:44 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail26.syd.optusnet.com.au (mail26.syd.optusnet.com.au [211.29.133.167]) by mx1.freebsd.org (Postfix) with ESMTP id EF5CA8FC15; Sat, 11 Aug 2012 10:27:43 +0000 (UTC) Received: from c122-106-171-246.carlnfd1.nsw.optusnet.com.au (c122-106-171-246.carlnfd1.nsw.optusnet.com.au [122.106.171.246]) by mail26.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q7BARXuw017534 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 11 Aug 2012 20:27:34 +1000 Date: Sat, 11 Aug 2012 20:27:32 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: David Xu In-Reply-To: <201208110006.q7B06uPR092724@svn.freebsd.org> Message-ID: <20120811181037.A853@besplex.bde.org> References: <201208110006.q7B06uPR092724@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r239187 - head/sys/kern X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Aug 2012 10:27:44 -0000 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