Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 12 Jun 2012 19:21:05 +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>, Bruce Evans <brde@optusnet.com.au>
Subject:   Re: svn commit: r236909 - head/sbin/hastd
Message-ID:  <20120612183547.C2680@besplex.bde.org>
In-Reply-To: <201206120951.38492.hselasky@c2i.net>
References:  <201206112359.41892.hselasky@c2i.net> <20120612130912.O1895@besplex.bde.org> <201206120951.38492.hselasky@c2i.net>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 12 Jun 2012, Hans Petter Selasky wrote:

> On Tuesday 12 June 2012 05:49:33 Bruce Evans wrote:
>> 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
>
>> 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.
>
> Lets assume you need to reboot the system at some point and that solves the
> problem.

Yes, that solves it for >= 86 years after rebooting, provided the clock id
is CLOCK_MONOTONIC and the time that this clock is relative to is the
boot time.

>>> 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:
>
> When CLOCK_REALTIME finally goes negative, then pthread_cond_timedwait() will
> simply return an error code, due to the negative tv_sec check in there! I see
> other clients like sendmail, using even simpler formats like:

Are you going to wait around for 86 years after rebooting for that? :-).

> tv_nsec = 0;
> tv_sec = time(NULL);
>
> If this piece of code stops working at a given data, regardless of uptime,
> shouldn't that be fixed now?

Only if you urgently need current systems 32-bit signed time_t, which are
rebooted tomorrow with this fix, to not need another reboot for 86 years.

Actually `tv_sec = time(NULL);' will overflow on only 26 years on such
systems, but time() use not usable together with CLOCK_MONOTONIC.  32-bit
signed time_t's are a more general problem.

> ...
> A final question on the matter:
>
> I tried to use CLOCK_MONOTONIC_FAST when setting up the condition variable.
> That did not appear to be support in 9-stable. Is there a list of supported
> clocks anywhere?

Don't know, but the list in clock_gettime(2) for -current seems to have
them all except CLOCK_THREAD_CPUTIME_ID.  That one is also badly named:
- verbosely named
- has an _ID suffix, unlike all the others
Since it is like CLOCK_PROF (the differences are that it is for threads
instead of processes, and interrupt times are broken in a different way
for it), it should be named more like CLOCK_PROF.

Of course the kernel source code has the correct list.

Most of these clock ids are only supported by clock_gettime() and
clock_getres().  Most of them arent't writeable, so clock_settime()
doesn't apply to them.  clock_settime(2) is identical with
clock_gettime(2), and doesn't say anything about which clocks are
read-only -- you have to try to set them to see which, or you can
read the POSIX spec to see that CLOCK_MONOTONIC must be read-only.
clock_settime(2) also fails to document the (bad) errno for failure
with CLOCK_MONOTONIC.  It only says "the following error codes _may_
be set", with EINVAL meaning that the clock id was invalid.  But in
POSIX, EINVAL for just clock_settime() is grossly overloaded; it means
any of:
- clock_id for unknown clock (this applies to all the functions; the
   rest are for clock_settime())
- the tp arg (sic; should be the value pointed to be the tp arg) is
   out of bounds for the specified clock_id
- the tp arg (sic) has an out of bounds nanosecond value
- the clock_id arg is CLOCK_MONOTONIC.

I don't like the explosion of clock ids in FreeBSD, and support for them
outside of clock_gettime() and clock_getres() seems to be nonexistent.
In man pages for POSIX realtime timers, only the standard CLOCK_REALTIME
and CLOCK_MONOTONIC are documented, and the code seems to agree.  FreeBSD
aliases like CLOCK_REALTIME_PRECISE will work because they are just
spelling errors.  FreeBSD extensions like CLOCK_REALTIME_FAST won't
work for timers, but timers are already "FAST" (really "!PRECISE").

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120612183547.C2680>