Re: [RFC][PATCH 2/2] Reduced NTP rework (part 2)

[Roman Zippel <zippel@linux-m68k.org>]

Hi,

On Tue, 27 Sep 2005, john stultz wrote:

> However, with you idea, on some arches we have to keep two timekeeping
> subsystems running at once, with code trying to smoothly synchronize
> them. I got a little frustrated trying to generate a clean
> implementation and decided to skip that idea for now. I'm not ruling it
> out, but I wanted to explore some other ideas I have.
> 
> So I started playing with a few different approaches in the short term,
> trying to adapt some of your ideas (such as using fixed interval time
> accumulation to avoid the multiply at tick time) to my existing code.

I don't want to keep you from playing with different ideas, but I would 
strongly suggest you do it first with an userspace simulator. If you 
change too much, be prepared to do some sort of analysis of these changes. 
A simulator would be one way, but plain math would be fine too.
It's really important that you get the math right first and then develop 
the kernel model from that. It would also be ok if you do a first 
prototype using nsec, but it's important to also look at the long term 
stability of the clock and how well it works together with the NTP daemon.

> The idea being:
> 
> update_wall_clock():
> 	ticks = jiffies - wall_jiffies
> 	while (ticks):
> 		ticks--
> 		xtime += tick_nsec + ntp_adjustment
> 
> 
> isn't that different from:
> 
> timekeeping_periodic_hook():
> 	now = timesource_read(ts)
> 	delta_cycle = now - last
> 	while (delta_cycle > interval_cycle):
> 		delta_cycle -= interval_cycle
> 		system_time += interval_nsec

BTW that's not what you do in the first part of the patch:

+static void ntp_advance(unsigned long interval_ns)

I'm quite sure that the interval_ns is wrong, it's important to advance 
the ntp state in constant intervals (i.e. interval_cycle). Your patch 
already includes time adjustments and e.g. the "while (interval_ns >= 
tick_nsec)" loop is not executed anymore, once time_adjust_step becomes 
negative.
In general I would prefer it if we could finalize the basic design first, 
before doing such changes, otherwise I'm afraid we need a cleanup of the 
cleanup.

> The only difference between continuous and tick based systems would then
> be in gettimeofday() (which really could be the same with a simple
> #define)
> 
> continuous_gettimeofday():
> 	now = timesource_read(ts)
> 	delta_cycle = now - last
> 	delta_nsec = cyc2ns(timesource, delta_cycle)
> 	return system_time + delta_nsec
> 
> tick_gettime():
> 	now = timesource_read(jiffes_timesource)
> 	delta_cycle = now - last
> 	delta_nsec = cyc2ns(timesource, delta_cycle)
> 	delta_nsec += arch_get_offset()
> 	return system_time + delta_nsec

The basic idea of gettimeofday is of course always the same: "base + 
get_offset() * mult". I can understand the temptation to unify the 
implementation, but please accept the current reality that we have 
different gettimeofday implementations (for whatever reasons), so unifying 
them would be a premature change. If the situation changes later we can 
still do that unification.

> Logic seen in the m68k time.c

I know that code and I really want to replace it with something better. :) 
Unfortunately I didn't catch the crap introduced by nsec conversion.

bye, Roman