Committed revision 2376 Steven Dake napsal(a): > 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
