[Openais] [corosync] [patch] - Use clock_gettime for timers
Jan Friesse
jfriesse at redhat.com
Fri Jul 31 00:50:31 PDT 2009
Steve,
patch should fulfill that case.
regards,
Honza
Steven Dake napsal(a):
> _POSIX_MONOTONIC_CLOCK could be defined to -1 which would pass this
> ifdef test but indicates the feature is not available on the platform.
>
> In general the concept is good though.
>
> Regards
> -steve
>
> On Thu, 2009-07-30 at 11:51 +0200, Jan Friesse wrote:
>> Steve,
>> what do you think about following patch. No autotools integration,
>> rather one simple #ifdef and warning on compilation.
>>
>> Regards,
>> Honza
>> Steven Dake wrote:
>>> After more looking, there is a sysconf test for this (_POSIX_TIMERS).
>>> I'm not sure how to do that test inside autotools though.
>>>
>>> Regards
>>> -steve
>>>
>>> On Wed, 2009-07-29 at 18:33 -0700, Steven Dake wrote:
>>>> Honza,
>>>>
>>>> After more testing, we need the special casing.
>>>>
>>>> MacOSX 10.5.7 (latest version) doesn't support clock_gettime.
>>>>
>>>> Regards
>>>> -steve
>>>>
>>>> On Mon, 2009-07-27 at 12:13 +0200, Jan Friesse wrote:
>>>>> Committed revision 2373
>>>>>
>>>>> Steven Dake napsal(a):
>>>>>> Patch looks good for commit
>>>>>>
>>>>>> Regards
>>>>>> -steve
>>>>>>
>>>>>> On Thu, 2009-07-23 at 10:09 +0200, Jan Friesse wrote:
>>>>>>> I hope final version of patch.
>>>>>>> #ifdefs removed and fixed problem with computing HZ (not used, but may
>>>>>>> be in future)
>>>>>>>
>>>>>>> Regards,
>>>>>>> Honza
>>>>>>>
>>>>>>>
>>>>>>>> Steve,
>>>>>>>> We talked about it on IRC
>>>>>>>>
>>>>>>>> Jan Friesse wrote:
>>>>>>>>> Steve,
>>>>>>>>> new patch included.
>>>>>>>>>
>>>>>>>>> Steven Dake wrote:
>>>>>>>>>> In general the patch looks very good. The issues are as follows:
>>>>>>>>>>
>>>>>>>>>> 1)
>>>>>>>>>>
>>>>>>>>>> corosync_timer_add_absolute expects the expire time to be the
>>>>>>>>>> nanoseconds since the epoch, or more specifically,
>>>>>>>>>>
>>>>>>>>>> from the time(7) man page:
>>>>>>>>>> The Epoch
>>>>>>>>>> Unix systems represent time in seconds since the Epoch,
>>>>>>>>>> which is
>>>>>>>>>> defined as 0:00:00 UTC on the morning of 1 January 1970.
>>>>>>>>>>
>>>>>>>>>> In this case, the expire_time may not be sufficient. We want absolute
>>>>>>>>>> timers to match the wall clock time rather then be based upon the offset
>>>>>>>>>> from the current time. If that is not the case with your patch, that
>>>>>>>>>> needs to be fixed. absolute timers are used by services to expire
>>>>>>>>>> timers when a specific wall clock time has passed. So in this case, if
>>>>>>>>>> ntp moves the time forward, we want that timer to expire, if its
>>>>>>>>>> recorded wall clock time is past the current time, the timer should be
>>>>>>>>>> expired.
>>>>>>>>> This is biggest change. I added is_absolute_timer to timerlist_timer. If
>>>>>>>>> this is true -> it's absolute timer and uses old behaviour. If not, it's
>>>>>>>>> relative and uses new behaviour. timerlist_add_absolute sets that
>>>>>>>>> variable to true, and timerlist_add_duration to false.
>>>>>>>>>
>>>>>>>>> This is (I hope) how this thing should behave.
>>>>>>>>>
>>>>>>>>>> 2) souce should be spelled source in configure.ac
>>>>>>>>>>
>>>>>>>>>> also the logic to detect monotonic clocks in configure.ac seems complex.
>>>>>>>>>> I'd like Jim to have a look at the configure.ac to determine if there is
>>>>>>>>>> a better way.
>>>>>>>>> Typo fixed + Jim added to CC.
>>>>>>>>>
>>>>>>>>> Jim,
>>>>>>>>> I don't think logic is so complex, but maybe there is really some better
>>>>>>>>> solution, I'm not autotools expert.
>>>>>>>>>
>>>>>>>>>> 3) get_monotonic_timeofday used in pload.c and votequorum.c and
>>>>>>>>>> totemsrp.c should be in tlist.h and should be named differently. I'd
>>>>>>>>>> choose timerlist_timer_duration_current_get() with same semantics of the
>>>>>>>>>> function.
>>>>>>>>>>
>>>>>>>>>> 4)
>>>>>>>>>> an additional function in tlist would be helpful for calculating the
>>>>>>>>>> difference in times, such as timerlist_timer_duration_calculate (tv1,
>>>>>>>>>> tv2, tv_out)
>>>>>>>>> Fixed in way point 7 suggest ;)
>>>>>>>>>> 5)
>>>>>>>>>> + nano_monotonic_hz = ts.tv_sec + 1000000000ULL / (unsigned long
>>>>>>>>>> long )ts.
>>>>>>>>>> experienced c programmers may be able to read this code if they have a
>>>>>>>>>> good understanding of operator precedence. I'd recommend parens to help
>>>>>>>>>> clarify to other maintainers the desired order of precedence.
>>>>>>>>> Clarified
>>>>>>>>>> 6)
>>>>>>>>>> + timer->expire_time= timerlist_nano_monotonic_time() + nano_duration;
>>>>>>>>>>
>>>>>>>>>> missing space
>>>>>>>>> Missing space added
>>>>>>>>>
>>>>>>>>>> 7)
>>>>>>>>>> regarding comment 3 and 4, this could be simplified throughout the code
>>>>>>>>>> base by making a new function in timerlist called
>>>>>>>>>>
>>>>>>>>>> timerlist_nano_current_get() which retrieves the current nanoseconds
>>>>>>>>>> from either the epoch or the monotonic clock source depending on os
>>>>>>>>>> support.
>>>>>>>>>>
>>>>>>>>>> this gets rid of all of the timersub junk throughout the code.
>>>>>>>>> Done. This makes some bigger changes in computing values, but I hope it
>>>>>>>>> makes it clearer. As bonus, I added TIMERLIST_?S_IN_?SEC macros, because
>>>>>>>>> writing everywhere values like 1000000000ULL, doesn't look good for me.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> 8)
>>>>>>>>>> after reviewing the changes, I'm not convinced the absolute timers work
>>>>>>>>>> as expected by the coroapi.
>>>>>>>>>>
>>>>>>>>>> good work
>>>>>>>>> I think now will.
>>>>>>>>>
>>>>>>>>>> Regards
>>>>>>>>>> -steve
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Honza
>>>>>>>>>
>>>>>>>>>> On Tue, 2009-07-21 at 15:42 +0200, Jan Friesse wrote:
>>>>>>>>>>> Patch should solve problem with ntp and corosync (what it really does)
>>>>>>>>>>> by using clock_gettime.
>>>>>>>>>>>
>>>>>>>>>>> I tried to find EVERY appearance of gettimeofday and where it makes
>>>>>>>>>>> sense (so basically, where this value is used as timer) convert to call
>>>>>>>>>>> clock_gettime.
>>>>>>>>>>>
>>>>>>>>>>> Patch modifies configure.ac to detect if clock_gettime and
>>>>>>>>>>> CLOCK_MONOTONIC exists on system. If not, gettimeofday will be used (so
>>>>>>>>>>> you will get old behavior).
>>>>>>>>>>>
>>>>>>>>>>> Comments are welcomed.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Honza
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> Openais mailing list
>>>>>>>>>>> Openais at lists.linux-foundation.org
>>>>>>>>>>> https://lists.linux-foundation.org/mailman/listinfo/openais
>>>>>>>>> ------------------------------------------------------------------------
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> Openais mailing list
>>>>>>>>> Openais at lists.linux-foundation.org
>>>>>>>>> https://lists.linux-foundation.org/mailman/listinfo/openais
>>>>>>>> ------------------------------------------------------------------------
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Openais mailing list
>>>>>>>> Openais at lists.linux-foundation.org
>>>>>>>> https://lists.linux-foundation.org/mailman/listinfo/openais
>>>> _______________________________________________
>>>> Openais mailing list
>>>> Openais at lists.linux-foundation.org
>>>> https://lists.linux-foundation.org/mailman/listinfo/openais
>> plain text document attachment
>> (corosync-monotonic_timer-and-masox.patch)
>> commit b7cb8efc0eb146649f8d09c9d59c540179e83bb3
>> Author: Jan Friesse <jfriesse at redhat.com>
>> Date: Thu Jul 30 11:47:57 2009 +0200
>>
>> Support for systems where clock_gettime is not available
>>
>> diff --git a/trunk/exec/tlist.h b/trunk/exec/tlist.h
>> index 2bad3b6..a87850d 100644
>> --- a/trunk/exec/tlist.h
>> +++ b/trunk/exec/tlist.h
>> @@ -92,6 +92,7 @@ static inline unsigned long long timerlist_nano_from_epoch (void)
>> return (nano_from_epoch);
>> }
>>
>> +#ifdef _POSIX_MONOTONIC_CLOCK
>> static inline unsigned long long timerlist_nano_current_get (void)
>> {
>> unsigned long long nano_monotonic;
>> @@ -113,6 +114,17 @@ static inline unsigned long long timerlist_nano_monotonic_hz (void) {
>>
>> return (nano_monotonic_hz);
>> }
>> +#else
>> +#warning "Your system doesn't support monotonic timer. gettimeofday will be used"
>> +static inline unsigned long long timerlist_nano_current_get (void)
>> +{
>> + return (timerlist_nano_from_epoch ());
>> +}
>> +
>> +static inline unsigned long long timerlist_nano_monotonic_hz (void) {
>> + return HZ;
>> +}
>> +#endif
>>
>> static inline void timerlist_add (struct timerlist *timerlist, struct timerlist_timer *timer)
>> {
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: corosync-monotonic_timer-and-masox-take2.patch
Type: text/x-patch
Size: 1281 bytes
Desc: not available
Url : http://lists.linux-foundation.org/pipermail/openais/attachments/20090731/46b315f1/attachment-0001.bin
More information about the Openais
mailing list