_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 > >>>>>>>>> [email protected] > >>>>>>>>> https://lists.linux-foundation.org/mailman/listinfo/openais > >>>>>>> ------------------------------------------------------------------------ > >>>>>>> > >>>>>>> _______________________________________________ > >>>>>>> Openais mailing list > >>>>>>> [email protected] > >>>>>>> https://lists.linux-foundation.org/mailman/listinfo/openais > >>>>>> ------------------------------------------------------------------------ > >>>>>> > >>>>>> _______________________________________________ > >>>>>> Openais mailing list > >>>>>> [email protected] > >>>>>> https://lists.linux-foundation.org/mailman/listinfo/openais > >> _______________________________________________ > >> Openais mailing list > >> [email protected] > >> https://lists.linux-foundation.org/mailman/listinfo/openais > > > > plain text document attachment > (corosync-monotonic_timer-and-masox.patch) > commit b7cb8efc0eb146649f8d09c9d59c540179e83bb3 > Author: Jan Friesse <[email protected]> > 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) > { _______________________________________________ Openais mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/openais
