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

Steven Dake sdake at redhat.com
Fri Jul 31 08:27:52 PDT 2009


patch looks good for commit

regards
-steve

On Fri, 2009-07-31 at 09:50 +0200, Jan Friesse wrote:
> Steve,
> patch should fulfill that case.
> 
> regards,
>    Honza
> 
> Steven Dake napsal(a):
> > _POSIX_MONOTONIC_CLOCK could be defined to -1 which would pass this
> > ifdef test but indicates the feature is not available on the platform.
> > 
> > In general the concept is good though.
> > 
> > Regards
> > -steve
> > 
> > On Thu, 2009-07-30 at 11:51 +0200, Jan Friesse wrote:
> >> 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
> >> plain text document attachment
> >> (corosync-monotonic_timer-and-masox.patch)
> >> commit b7cb8efc0eb146649f8d09c9d59c540179e83bb3
> >> Author: Jan Friesse <jfriesse at redhat.com>
> >> Date:   Thu Jul 30 11:47:57 2009 +0200
> >>
> >>     Support for systems where clock_gettime is not available
> >>
> >> diff --git a/trunk/exec/tlist.h b/trunk/exec/tlist.h
> >> index 2bad3b6..a87850d 100644
> >> --- a/trunk/exec/tlist.h
> >> +++ b/trunk/exec/tlist.h
> >> @@ -92,6 +92,7 @@ static inline unsigned long long timerlist_nano_from_epoch (void)
> >>  	return (nano_from_epoch);
> >>  }
> >>  
> >> +#ifdef _POSIX_MONOTONIC_CLOCK
> >>  static inline unsigned long long timerlist_nano_current_get (void)
> >>  {
> >>  	unsigned long long nano_monotonic;
> >> @@ -113,6 +114,17 @@ static inline unsigned long long timerlist_nano_monotonic_hz (void) {
> >>  
> >>  	return (nano_monotonic_hz);
> >>  }
> >> +#else
> >> +#warning "Your system doesn't support monotonic timer. gettimeofday will be used"
> >> +static inline unsigned long long timerlist_nano_current_get (void)
> >> +{
> >> +	return (timerlist_nano_from_epoch ());
> >> +}
> >> +
> >> +static inline unsigned long long timerlist_nano_monotonic_hz (void) {
> >> +	return HZ;
> >> +}
> >> +#endif
> >>  
> >>  static inline void timerlist_add (struct timerlist *timerlist, struct timerlist_timer *timer)
> >>  {
> > 
> 



More information about the Openais mailing list