[Openais] [corosync] [patch] - Use clock_gettime for timers

[Jan Friesse]

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
>