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
