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

[Jan Friesse]

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
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: corosync-clock_gettime-take2.patch
Type: text/x-patch
Size: 16431 bytes
Desc: not available
Url : http://lists.linux-foundation.org/pipermail/openais/attachments/20090722/c18b2e27/attachment-0001.bin