Date: Tue, 12 Jun 2012 13:49:33 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Hans Petter Selasky <hselasky@c2i.net> Cc: "svn-src-head@freebsd.org" <svn-src-head@FreeBSD.org>, "svn-src-all@freebsd.org" <svn-src-all@FreeBSD.org>, "src-committers@freebsd.org" <src-committers@FreeBSD.org>, Pawel Jakub Dawidek <pjd@FreeBSD.org> Subject: Re: svn commit: r236909 - head/sbin/hastd Message-ID: <20120612130912.O1895@besplex.bde.org> In-Reply-To: <201206112359.41892.hselasky@c2i.net> References: <20120611200507.GG1399@garage.freebsd.pl> <201206112221.51793.hselasky@c2i.net> <201206112359.41892.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120612130912.O1895>