[Openais] [corosync] [patch] - Use clock_gettime for timers
Jan Friesse
jfriesse at redhat.com
Thu Jul 30 02:51:07 PDT 2009
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
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: corosync-monotonic_timer-and-masox.patch
Url: http://lists.linux-foundation.org/pipermail/openais/attachments/20090730/dfc5db76/attachment.txt
More information about the Openais
mailing list