From owner-svn-src-all@FreeBSD.ORG Tue Jun 12 03:49:54 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6503C1065673; Tue, 12 Jun 2012 03:49:54 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id DCDF58FC14; Tue, 12 Jun 2012 03:49:53 +0000 (UTC) Received: from c122-106-171-232.carlnfd1.nsw.optusnet.com.au (c122-106-171-232.carlnfd1.nsw.optusnet.com.au [122.106.171.232]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q5C3nYPJ014984 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 12 Jun 2012 13:49:35 +1000 Date: Tue, 12 Jun 2012 13:49:33 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Hans Petter Selasky In-Reply-To: <201206112359.41892.hselasky@c2i.net> Message-ID: <20120612130912.O1895@besplex.bde.org> References: <20120611200507.GG1399@garage.freebsd.pl> <201206112221.51793.hselasky@c2i.net> <201206112359.41892.hselasky@c2i.net> 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" , Pawel Jakub Dawidek Subject: Re: svn commit: r236909 - head/sbin/hastd 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: Tue, 12 Jun 2012 03:49:54 -0000 On Mon, 11 Jun 2012, Hans Petter Selasky wrote: > On Monday 11 June 2012 22:21:51 Hans Petter Selasky wrote: >> On Monday 11 June 2012 22:05:07 Pawel Jakub Dawidek wrote: >>> On Mon, Jun 11, 2012 at 07:21:00PM +0000, Hans Petter Selasky wrote: >>>> Author: hselasky >>>> Date: Mon Jun 11 19:20:59 2012 >>>> New Revision: 236909 >>>> URL: http://svn.freebsd.org/changeset/base/236909 >>>> >>>> Log: >>>> Use the correct clock source when computing timeouts. >>> >>> Could you please explain why? As you can see some lines above in >> >>> cv_init(), we initialize condition variable with CLOCK_MONOTONIC too: >> Sorry, this was a mistake clearly. I will revert ASAP. Pointyhat to me. >> >> My test program didn't take the setattr into account. >> >> However, while at it, what is the default clock used by >> pthread_cond_timedwait(). In libusb we don't set any clock, and can we >> depend on that CLOCK_REALTIME is the default clock used? Else I should >> probably make a patch there. >> >> man pthread_cond_timedwait() is silent! > Some more questions: > > While doing my test, I traced pthread_cond_timedwait() into > "./kern/kern_umtx.c" where the time is subtracted again, so the actual time > value is not that important, but there are some other problems: > > If the time structure argument passed to pthread_cond_timedwait() in 9-stable > is negative, for example the seconds field, the above mentioned function will > just return! See ./libkse/thread/thr_cond.c Inconsistent clock ids might do that. > int > _pthread_cond_timedwait(pthread_cond_t * cond, pthread_mutex_t * mutex, > const struct timespec * abstime) > { > struct pthread *curthread = _get_curthread(); > int rval = 0; > int done = 0; > int mutex_locked = 1; > int seqno; > > THR_ASSERT(curthread->locklevel == 0, > "cv_timedwait: locklevel is not zero!"); > > if (abstime == NULL || abstime->tv_sec < 0 || abstime->tv_nsec < 0 || > abstime->tv_nsec >= 1000000000) > return (EINVAL); I don't really understand this code, but just looked at it a bit. There is a clock id in it somewhere, though not part of the pthread_cond_timedwait() API. Apparently you need to set its clock id separarely. According to hastd code, the other separate function is pthread_condattr_setclock(), which hastd::cv_int() uses to set CLOCK_MONOTONIC. I said in another reply that the clock id must be CLOCK_MONONTONIC because that is the only one supported, but this code apparently supports multiple clock ids, and kernel "realtime" timer code tries to support CLOCK_MONOTONIC and CLOCK_REALTIME and TIMER_ABSTIME, although clock_nanosleep() is missing. The problems with CLOCK_REALTIME come later, because the lowest levels of timeout code only support buggy monotonic time. > CLOCK_MONOTONIC: 18077 (seconds) > CLOCK_REALTIME: 1339451481 (seconds) > > CLOCK_REALTIME will at some point become negative. Will libusb's timeout The point is in 2038, on systems with 32-bit signed time_t's. None should exist then. Even if time_t is 32 bits, it can be unsigned, and never become negative, and work until 2106. Changing time_t from signed to unsigned would break mainly times before the Epoch, which are invalid for current times anyway, and expose broken code which assumes that time_t is signed. > functionality stop working then, because pthread_cond_timedwait() has a check > for negative time? This check is just to prevent invalid times. It does prevents hacks like the kernel treating negative times as large unsigned ones so that the result is much the same as changing time_t to unsigned, without actually changing time_t. > Or is hastd wrong, that it can compute a timeout offset which is outside the > valid range, because it uses a simple: > > tv_sec += timeout? > tv_sec %= 1000000000; /* Is this perhaps missing in hastd and other drivers > ???? */ > > What is the modulus which should be used for tv_sec? `tv_sec %= ANY' makes no sense. With CLOCK_MONOTONIC, signed 32-bit time_t's work for 86 years after the unspecified point in the past that CLOCK_MONOTONIC is relative to. This should be enough for anyone, provided the unspecified point is the boot time. hastd also uses mostly-relative, mostly-monotonic, often 32-bit signed in timeouts internally. E.g., in the function that seems to be causing problems: % static __inline bool % cv_timedwait(pthread_cond_t *cv, pthread_mutex_t *lock, int timeout) % { % struct timespec ts; % int error; % % if (timeout == 0) { % cv_wait(cv, lock); % return (false); % } % % error = clock_gettime(CLOCK_MONOTONIC, &ts); Style bug (corrupt tab in Oct 22 2011 version). % PJDLOG_ASSERT(error == 0); % ts.tv_sec += timeout; This converts to absolute monotonic time. Even a 16-bit signed int is probably enough for `timeout'. % error = pthread_cond_timedwait(cv, lock, &ts); % PJDLOG_ASSERT(error == 0 || error == ETIMEDOUT); % return (error == ETIMEDOUT); % } I don't see any bugs here. Bruce