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

[Steven Dake]

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