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@lists.linux-foundation.org >>>>>> https://lists.linux-foundation.org/mailman/listinfo/openais >>>> ------------------------------------------------------------------------ >>>> >>>> _______________________________________________ >>>> Openais mailing list >>>> Openais@lists.linux-foundation.org >>>> https://lists.linux-foundation.org/mailman/listinfo/openais >>> >>> ------------------------------------------------------------------------ >>> >>> _______________________________________________ >>> Openais mailing list >>> Openais@lists.linux-foundation.org >>> https://lists.linux-foundation.org/mailman/listinfo/openais >
_______________________________________________ Openais mailing list Openais@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/openais