Re: clocksource

[Roman Zippel]

Hi,

On Fri, 16 Jun 2006, john stultz wrote:

> > This makes it more complex than necessary. AFAICT this controller 
> > calculates the adjustment solely based on the current error, but we have 
> > more information than this, which make the current error rather 
> > uninteresting.
> 
> Indeed it is the current error, but its also taking the change in error
> into account as well.

Which is not really important anymore, as soon as you already _know_ the 
future error.

> > We know the clock frequency and the NTP frequency so we can easily 
> > precalculate, how the error will look like at the next few ticks. Based on 
> > this we can calculate how we have to adjust the clock frequency to reduce 
> > the error. Overshooting is also not a real problem as long as the absolute 
> > error gets smaller.
> 
> I'm not sure I agree here. Using your patch series, if I re-enable the
> code that drops calls to update_wall_time (simulating lost ticks) the
> clock does not appear very stable.

That's because the tick length is not applied in the same way in 
sim-main.c and time.c, if the drop happens around a call to 
second_overflow(), one gets the old value and the other gets the new 
value. If you prevent the drop around a ppm change, it will work just 
fine, it's a bug in the simulator.

> Robustness and features like
> dynamic/no_idle_hz are going to require that we can handle taking
> something close to only one tick per second, so overshoot is a big
> concern in my mind. Maybe I'm misunderstanding you?

Overshooting is not your real problem in this case, as large offsets and 
lost interrupts are less a problem. You're making it way too complex...

> However, I need to forward port your patchset to the new simulator to
> really do a fair comparison as I know there were some issues w/ the
> simulator that I addressed in order to get the new features working. If
> you have already done this, let me know.

The new simulator is far too complex and I don't want to spend the time 
verifying it does a really correct simulation, I don't think it's really a 
good idea to make the simulator more complex than the subject, otherwise 
you need a simulator to test the simulator...
You are also using rather smallish adjustments, changes of upto 1ms/s are 
more realistic (NTP allows a MAXPHASE offset which is spread over a period 
of time). Your simulator is also fixed to 1GHz and 1000Hz, which are 
rather convenient values.

> Line 4b is a bit foggy. Just assuming cycle_offset is zero, we can
> ignore line 3 and 4a. So we're reducing the error by the change in
> length of the next interval. I see how this would in effect dampen the
> next adjustment, but I'm not sure how that then maps the error value to
> the actual distance from ntp_time.

As I said it looks ahead to the next error, if we adjust the multiplier at 
the current tick, it also changes the error at the next tick.

> Abstractly I understand how looking at the next tick is good for when
> the NTP adjustment value changes, but I'm not sure I see how looking
> ahead makes the clock more stable when the NTP adjustment isn't
> changing.

Huh? If the NTP adjustment isn't changing, clock and NTP are hopefully in 
sync and there is no (significant) error to adjust.

> Is there a way you can map the math above to the terms of PID control

Please forget this one, it doesn't really apply here. We know more about 
the model, which allows for simpler calculations. You have to throw away 
information to fit it into the PID model.

> (or maybe some other established concept that I can dig deeper on?).

I guess it's basically a FLL.

Below is another patch, which better takes the update delays into account.

bye, Roman



take the offset better into account by applying the current adjustment
to the error and anticipate a possible large adjustment at the next
update due to a large offset.
---
 time.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: simulator_C2/time.c
===================================================================
--- simulator_C2.orig/time.c
+++ simulator_C2/time.c
@@ -227,7 +227,7 @@ device_initcall(timekeeping_init_device)
  * If the error is already larger, we look ahead another tick,
  * to compensate for late or lost adjustments.
  */
-static __always_inline int clocksource_bigadjust(int sign, s64 error, s64 interval)
+static __always_inline int clocksource_bigadjust(int sign, s64 error, s64 interval, s64 offset)
 {
 	int adj = 0;
 
@@ -236,8 +236,12 @@ static __always_inline int clocksource_b
 
 	while (1) {
 		error >>= 1;
-		if (sign > 0 ? error <= interval : error >= interval)
+		if (sign > 0 ? error <= interval : error >= interval) {
+			error = (error << 1) - interval + offset;
+			if (sign > 0 ? error > interval : error < interval)
+				adj++;
 			return adj;
+		}
 		adj++;
 	}
 }
@@ -246,7 +250,8 @@ static __always_inline int clocksource_b
 	int adj = sign;							\
 	error >>= 2;							\
 	if (unlikely(sign > 0 ? error > interval : error < interval)) {	\
-		adj = clocksource_bigadjust(sign, error, interval);	\
+		adj = clocksource_bigadjust(sign, error,		\
+					    interval, offset);		\
 		interval <<= adj;					\
 		offset <<= adj;						\
 		adj = sign << adj;					\
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/