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 > >>>>>>>>>>> [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
