[Openais] [corosync] [patch] - Use clock_gettime for timers
Jan Friesse
jfriesse at redhat.com
Thu Jul 23 01:09:07 PDT 2009
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: corosync-clock_gettime-take4.patch
Type: text/x-patch
Size: 13478 bytes
Desc: not available
Url : http://lists.linux-foundation.org/pipermail/openais/attachments/20090723/c8121e67/attachment.bin
More information about the Openais
mailing list