2009-07-29 13:43:25

by Martin Schwidefsky

[permalink] [raw]
Subject: [RFC][patch 10/12] move NTP adjusted clock multiplier to struct timekeeper

From: Martin Schwidefsky <[email protected]>

The clocksource structure has two multipliers, the unmodified multiplier
clock->mult_orig and the NTP corrected multiplier clock->mult. The NTP
multiplier is misplaced in the struct clocksource, this is private
information of the timekeeping code. Add the mult field to the struct
timekeeper to contain the NTP corrected value, keep the unmodifed
multiplier in clock->mult and remove clock->mult_orig. For consistency
add the shift value associated with the NTP corrected mult value to
struct timekeeper as well.

Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: john stultz <[email protected]>
Cc: Daniel Walker <[email protected]>
Signed-off-by: Martin Schwidefsky <[email protected]>
---
arch/arm/plat-omap/common.c | 2 +-
include/linux/clocksource.h | 4 +---
kernel/time/timekeeping.c | 43 +++++++++++++++++++++++++------------------
3 files changed, 27 insertions(+), 22 deletions(-)

Index: linux-2.6/include/linux/clocksource.h
===================================================================
--- linux-2.6.orig/include/linux/clocksource.h
+++ linux-2.6/include/linux/clocksource.h
@@ -149,8 +149,7 @@ extern u64 timecounter_cyc2time(struct t
* @disable: optional function to disable the clocksource
* @mask: bitmask for two's complement
* subtraction of non 64 bit counters
- * @mult: cycle to nanosecond multiplier (adjusted by NTP)
- * @mult_orig: cycle to nanosecond multiplier (unadjusted by NTP)
+ * @mult: cycle to nanosecond multiplier
* @shift: cycle to nanosecond divisor (power of two)
* @flags: flags describing special properties
* @vread: vsyscall based read
@@ -168,7 +167,6 @@ struct clocksource {
void (*disable)(struct clocksource *cs);
cycle_t mask;
u32 mult;
- u32 mult_orig;
u32 shift;
unsigned long flags;
cycle_t (*vread)(void);
Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -29,6 +29,8 @@ struct timekeeper {
s64 ntp_error;
int xtime_shift;
int ntp_error_shift;
+ u32 mult; /* NTP adjusted clock multiplier */
+ u32 shift; /* NTP adjusted clock shift */
};

struct timekeeper timekeeper;
@@ -54,8 +56,8 @@ static void timekeeper_setup_internals(s
/* Do the ns -> cycle conversion first, using original mult */
tmp = NTP_INTERVAL_LENGTH;
tmp <<= clock->shift;
- tmp += clock->mult_orig/2;
- do_div(tmp, clock->mult_orig);
+ tmp += clock->mult/2;
+ do_div(tmp, clock->mult);
if (tmp == 0)
tmp = 1;

@@ -65,13 +67,21 @@ static void timekeeper_setup_internals(s
/* Go back from cycles -> shifted ns */
timekeeper.xtime_interval = (u64) interval * clock->mult;
timekeeper.raw_interval =
- ((u64) interval * clock->mult_orig) >> clock->shift;
+ ((u64) interval * clock->mult) >> clock->shift;

timekeeper.xtime_nsec = 0;
timekeeper.xtime_shift = clock->shift;

timekeeper.ntp_error = 0;
timekeeper.ntp_error_shift = NTP_SCALE_SHIFT - clock->shift;
+
+ /*
+ * The timekeeper keeps its own mult & shift values for the
+ * currently active clocksource. These value will be adjusted
+ * via NTP to counteract clock drifting.
+ */
+ timekeeper.mult = clock->mult;
+ timekeeper.shift = clock->shift;
}

/*
@@ -142,14 +152,15 @@ static void timekeeping_forward_now(void
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
clock->cycle_last = cycle_now;

- nsec = clocksource_cyc2ns(cycle_delta, clock->mult, clock->shift);
+ nsec = clocksource_cyc2ns(cycle_delta, timekeeper.mult,
+ timekeeper.shift);

/* If arch requires, add in gettimeoffset() */
nsec += arch_gettimeoffset();

timespec_add_ns(&xtime, nsec);

- nsec = clocksource_cyc2ns(cycle_delta, clock->mult_orig, clock->shift);
+ nsec = clocksource_cyc2ns(cycle_delta, clock->mult, clock->shift);
timespec_add_ns(&raw_time, nsec);
}

@@ -181,8 +192,8 @@ void getnstimeofday(struct timespec *ts)
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;

/* convert to nanoseconds: */
- nsecs = clocksource_cyc2ns(cycle_delta, clock->mult,
- clock->shift);
+ nsecs = clocksource_cyc2ns(cycle_delta, timekeeper.mult,
+ timekeeper.shift);

/* If arch requires, add in gettimeoffset() */
nsecs += arch_gettimeoffset();
@@ -216,8 +227,8 @@ ktime_t ktime_get(void)
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;

/* convert to nanoseconds: */
- nsecs += clocksource_cyc2ns(cycle_delta, clock->mult,
- clock->shift);
+ nsecs += clocksource_cyc2ns(cycle_delta, timekeeper.mult,
+ timekeeper.shift);

} while (read_seqretry(&xtime_lock, seq));
/*
@@ -259,8 +270,8 @@ void ktime_get_ts(struct timespec *ts)
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;

/* convert to nanoseconds: */
- nsecs = clocksource_cyc2ns(cycle_delta, clock->mult,
- clock->shift);
+ nsecs = clocksource_cyc2ns(cycle_delta, timekeeper.mult,
+ timekeeper.shift);

} while (read_seqretry(&xtime_lock, seq));

@@ -344,8 +355,6 @@ static void change_clocksource(void)

if (new->enable && ! new->enable(new))
return;
- /* save mult_orig on enable */
- new->mult_orig = new->mult;

old = timekeeper.clock;
timekeeper_setup_internals(new);
@@ -438,7 +447,7 @@ void getrawmonotonic(struct timespec *ts
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;

/* convert to nanoseconds: */
- nsecs = clocksource_cyc2ns(cycle_delta, clock->mult_orig,
+ nsecs = clocksource_cyc2ns(cycle_delta, clock->mult,
clock->shift);

*ts = raw_time;
@@ -498,8 +507,6 @@ void __init timekeeping_init(void)
clock = clocksource_default_clock();
if (clock->enable)
clock->enable(clock);
- /* save mult_orig on enable */
- clock->mult_orig = clock->mult;

timekeeper_setup_internals(clock);

@@ -674,7 +681,7 @@ static void timekeeping_adjust(s64 offse
} else
return;

- timekeeper.clock->mult += adj;
+ timekeeper.mult += adj;
timekeeper.xtime_interval += interval;
timekeeper.xtime_nsec -= offset;
timekeeper.ntp_error -= (interval - offset) <<
@@ -765,7 +772,7 @@ void update_wall_time(void)
timekeeper.ntp_error += timekeeper.xtime_nsec <<
timekeeper.ntp_error_shift;

- nsecs = clocksource_cyc2ns(offset, clock->mult, clock->shift);
+ nsecs = clocksource_cyc2ns(offset, timekeeper.mult, timekeeper.shift);
update_xtime_cache(nsecs);

/* check to see if there is a new clocksource to use */
Index: linux-2.6/arch/arm/plat-omap/common.c
===================================================================
--- linux-2.6.orig/arch/arm/plat-omap/common.c
+++ linux-2.6/arch/arm/plat-omap/common.c
@@ -256,7 +256,7 @@ unsigned long long sched_clock(void)
unsigned long long ret;

ret = (unsigned long long)clocksource_32k.read(&clocksource_32k);
- ret = (ret * clocksource_32k.mult_orig) >> clocksource_32k.shift;
+ ret = (ret * clocksource_32k.mult) >> clocksource_32k.shift;
return ret;
}


--
blue skies,
Martin.

"Reality continues to ruin my life." - Calvin.


2009-07-30 22:04:52

by john stultz

[permalink] [raw]
Subject: Re: [RFC][patch 10/12] move NTP adjusted clock multiplier to struct timekeeper

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (timekeeper-mult.diff)
> From: Martin Schwidefsky <[email protected]>
>
> The clocksource structure has two multipliers, the unmodified multiplier
> clock->mult_orig and the NTP corrected multiplier clock->mult. The NTP
> multiplier is misplaced in the struct clocksource, this is private
> information of the timekeeping code. Add the mult field to the struct
> timekeeper to contain the NTP corrected value, keep the unmodifed
> multiplier in clock->mult and remove clock->mult_orig. For consistency
> add the shift value associated with the NTP corrected mult value to
> struct timekeeper as well.
>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: john stultz <[email protected]>
> Cc: Daniel Walker <[email protected]>
> Signed-off-by: Martin Schwidefsky <[email protected]>
> ---
> arch/arm/plat-omap/common.c | 2 +-
> include/linux/clocksource.h | 4 +---
> kernel/time/timekeeping.c | 43 +++++++++++++++++++++++++------------------
> 3 files changed, 27 insertions(+), 22 deletions(-)
>
> Index: linux-2.6/include/linux/clocksource.h
> ===================================================================
> --- linux-2.6.orig/include/linux/clocksource.h
> +++ linux-2.6/include/linux/clocksource.h
> @@ -149,8 +149,7 @@ extern u64 timecounter_cyc2time(struct t
> * @disable: optional function to disable the clocksource
> * @mask: bitmask for two's complement
> * subtraction of non 64 bit counters
> - * @mult: cycle to nanosecond multiplier (adjusted by NTP)
> - * @mult_orig: cycle to nanosecond multiplier (unadjusted by NTP)
> + * @mult: cycle to nanosecond multiplier
> * @shift: cycle to nanosecond divisor (power of two)
> * @flags: flags describing special properties
> * @vread: vsyscall based read
> @@ -168,7 +167,6 @@ struct clocksource {
> void (*disable)(struct clocksource *cs);
> cycle_t mask;
> u32 mult;
> - u32 mult_orig;
> u32 shift;
> unsigned long flags;
> cycle_t (*vread)(void);
> Index: linux-2.6/kernel/time/timekeeping.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/timekeeping.c
> +++ linux-2.6/kernel/time/timekeeping.c
> @@ -29,6 +29,8 @@ struct timekeeper {
> s64 ntp_error;
> int xtime_shift;
> int ntp_error_shift;
> + u32 mult; /* NTP adjusted clock multiplier */
> + u32 shift; /* NTP adjusted clock shift */

I'm a little confused here. NTP doesn't adjust the shift value. This
seems redundant with the xtime_shift value introduced in [patch 9/12].

thanks
-john

2009-07-31 07:53:31

by Martin Schwidefsky

[permalink] [raw]
Subject: Re: [RFC][patch 10/12] move NTP adjusted clock multiplier to struct timekeeper

On Thu, 30 Jul 2009 15:04:48 -0700
john stultz <[email protected]> wrote:

> On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > plain text document attachment (timekeeper-mult.diff)
> > From: Martin Schwidefsky <[email protected]>
> >
> > The clocksource structure has two multipliers, the unmodified multiplier
> > clock->mult_orig and the NTP corrected multiplier clock->mult. The NTP
> > multiplier is misplaced in the struct clocksource, this is private
> > information of the timekeeping code. Add the mult field to the struct
> > timekeeper to contain the NTP corrected value, keep the unmodifed
> > multiplier in clock->mult and remove clock->mult_orig. For consistency
> > add the shift value associated with the NTP corrected mult value to
> > struct timekeeper as well.
> >
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: john stultz <[email protected]>
> > Cc: Daniel Walker <[email protected]>
> > Signed-off-by: Martin Schwidefsky <[email protected]>
> > ---
> > arch/arm/plat-omap/common.c | 2 +-
> > include/linux/clocksource.h | 4 +---
> > kernel/time/timekeeping.c | 43 +++++++++++++++++++++++++------------------
> > 3 files changed, 27 insertions(+), 22 deletions(-)
> >
> > Index: linux-2.6/include/linux/clocksource.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/clocksource.h
> > +++ linux-2.6/include/linux/clocksource.h
> > @@ -149,8 +149,7 @@ extern u64 timecounter_cyc2time(struct t
> > * @disable: optional function to disable the clocksource
> > * @mask: bitmask for two's complement
> > * subtraction of non 64 bit counters
> > - * @mult: cycle to nanosecond multiplier (adjusted by NTP)
> > - * @mult_orig: cycle to nanosecond multiplier (unadjusted by NTP)
> > + * @mult: cycle to nanosecond multiplier
> > * @shift: cycle to nanosecond divisor (power of two)
> > * @flags: flags describing special properties
> > * @vread: vsyscall based read
> > @@ -168,7 +167,6 @@ struct clocksource {
> > void (*disable)(struct clocksource *cs);
> > cycle_t mask;
> > u32 mult;
> > - u32 mult_orig;
> > u32 shift;
> > unsigned long flags;
> > cycle_t (*vread)(void);
> > Index: linux-2.6/kernel/time/timekeeping.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/time/timekeeping.c
> > +++ linux-2.6/kernel/time/timekeeping.c
> > @@ -29,6 +29,8 @@ struct timekeeper {
> > s64 ntp_error;
> > int xtime_shift;
> > int ntp_error_shift;
> > + u32 mult; /* NTP adjusted clock multiplier */
> > + u32 shift; /* NTP adjusted clock shift */
>
> I'm a little confused here. NTP doesn't adjust the shift value. This
> seems redundant with the xtime_shift value introduced in [patch 9/12].

True the NTP code does not adjust the shift value. But mult and shift
build a pair, no? I think it is more readable if we use timekeeper.mult
and timekeeper.shift instead of timekeeper.mult and clock->shift.
And the xtime_shift does not need to be the same value as the clock
shift - at least in theory. It is just a shift to increase precision.
Therefore I would like to use the xtime_shift and ntp_error_shift values
exclusively as the shifts for the respective xtime_nsec and ntp_error
values. Makes sense ?

--
blue skies,
Martin.

"Reality continues to ruin my life." - Calvin.

2009-07-31 08:13:05

by john stultz

[permalink] [raw]
Subject: Re: [RFC][patch 10/12] move NTP adjusted clock multiplier to struct timekeeper

On Fri, 2009-07-31 at 09:52 +0200, Martin Schwidefsky wrote:
> On Thu, 30 Jul 2009 15:04:48 -0700
> john stultz <[email protected]> wrote:
>
> > On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > > plain text document attachment (timekeeper-mult.diff)
> > > From: Martin Schwidefsky <[email protected]>
> > >
> > > The clocksource structure has two multipliers, the unmodified multiplier
> > > clock->mult_orig and the NTP corrected multiplier clock->mult. The NTP
> > > multiplier is misplaced in the struct clocksource, this is private
> > > information of the timekeeping code. Add the mult field to the struct
> > > timekeeper to contain the NTP corrected value, keep the unmodifed
> > > multiplier in clock->mult and remove clock->mult_orig. For consistency
> > > add the shift value associated with the NTP corrected mult value to
> > > struct timekeeper as well.
> > >
> > > Cc: Ingo Molnar <[email protected]>
> > > Cc: Thomas Gleixner <[email protected]>
> > > Cc: john stultz <[email protected]>
> > > Cc: Daniel Walker <[email protected]>
> > > Signed-off-by: Martin Schwidefsky <[email protected]>
> > > ---
> > > arch/arm/plat-omap/common.c | 2 +-
> > > include/linux/clocksource.h | 4 +---
> > > kernel/time/timekeeping.c | 43 +++++++++++++++++++++++++------------------
> > > 3 files changed, 27 insertions(+), 22 deletions(-)
> > >
> > > Index: linux-2.6/include/linux/clocksource.h
> > > ===================================================================
> > > --- linux-2.6.orig/include/linux/clocksource.h
> > > +++ linux-2.6/include/linux/clocksource.h
> > > @@ -149,8 +149,7 @@ extern u64 timecounter_cyc2time(struct t
> > > * @disable: optional function to disable the clocksource
> > > * @mask: bitmask for two's complement
> > > * subtraction of non 64 bit counters
> > > - * @mult: cycle to nanosecond multiplier (adjusted by NTP)
> > > - * @mult_orig: cycle to nanosecond multiplier (unadjusted by NTP)
> > > + * @mult: cycle to nanosecond multiplier
> > > * @shift: cycle to nanosecond divisor (power of two)
> > > * @flags: flags describing special properties
> > > * @vread: vsyscall based read
> > > @@ -168,7 +167,6 @@ struct clocksource {
> > > void (*disable)(struct clocksource *cs);
> > > cycle_t mask;
> > > u32 mult;
> > > - u32 mult_orig;
> > > u32 shift;
> > > unsigned long flags;
> > > cycle_t (*vread)(void);
> > > Index: linux-2.6/kernel/time/timekeeping.c
> > > ===================================================================
> > > --- linux-2.6.orig/kernel/time/timekeeping.c
> > > +++ linux-2.6/kernel/time/timekeeping.c
> > > @@ -29,6 +29,8 @@ struct timekeeper {
> > > s64 ntp_error;
> > > int xtime_shift;
> > > int ntp_error_shift;
> > > + u32 mult; /* NTP adjusted clock multiplier */
> > > + u32 shift; /* NTP adjusted clock shift */
> >
> > I'm a little confused here. NTP doesn't adjust the shift value. This
> > seems redundant with the xtime_shift value introduced in [patch 9/12].
>
> True the NTP code does not adjust the shift value. But mult and shift
> build a pair, no? I think it is more readable if we use timekeeper.mult
> and timekeeper.shift instead of timekeeper.mult and clock->shift.
> And the xtime_shift does not need to be the same value as the clock
> shift - at least in theory. It is just a shift to increase precision.
> Therefore I would like to use the xtime_shift and ntp_error_shift values
> exclusively as the shifts for the respective xtime_nsec and ntp_error
> values. Makes sense ?

But can't we drop the xtime_shift if we have the clocksource shift kept
internally? Unless I'm missing something more subtle in your patches,
they should be the same at all times.

thanks
-john


2009-07-31 08:28:19

by Martin Schwidefsky

[permalink] [raw]
Subject: Re: [RFC][patch 10/12] move NTP adjusted clock multiplier to struct timekeeper

On Fri, 31 Jul 2009 01:12:59 -0700
john stultz <[email protected]> wrote:

> On Fri, 2009-07-31 at 09:52 +0200, Martin Schwidefsky wrote:
> > On Thu, 30 Jul 2009 15:04:48 -0700
> > john stultz <[email protected]> wrote:
> >
> > > On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > > > plain text document attachment (timekeeper-mult.diff)
> > > > From: Martin Schwidefsky <[email protected]>
> > > >
> > > > The clocksource structure has two multipliers, the unmodified multiplier
> > > > clock->mult_orig and the NTP corrected multiplier clock->mult. The NTP
> > > > multiplier is misplaced in the struct clocksource, this is private
> > > > information of the timekeeping code. Add the mult field to the struct
> > > > timekeeper to contain the NTP corrected value, keep the unmodifed
> > > > multiplier in clock->mult and remove clock->mult_orig. For consistency
> > > > add the shift value associated with the NTP corrected mult value to
> > > > struct timekeeper as well.
> > > >
> > > > Cc: Ingo Molnar <[email protected]>
> > > > Cc: Thomas Gleixner <[email protected]>
> > > > Cc: john stultz <[email protected]>
> > > > Cc: Daniel Walker <[email protected]>
> > > > Signed-off-by: Martin Schwidefsky <[email protected]>
> > > > ---
> > > > arch/arm/plat-omap/common.c | 2 +-
> > > > include/linux/clocksource.h | 4 +---
> > > > kernel/time/timekeeping.c | 43 +++++++++++++++++++++++++------------------
> > > > 3 files changed, 27 insertions(+), 22 deletions(-)
> > > >
> > > > Index: linux-2.6/include/linux/clocksource.h
> > > > ===================================================================
> > > > --- linux-2.6.orig/include/linux/clocksource.h
> > > > +++ linux-2.6/include/linux/clocksource.h
> > > > @@ -149,8 +149,7 @@ extern u64 timecounter_cyc2time(struct t
> > > > * @disable: optional function to disable the clocksource
> > > > * @mask: bitmask for two's complement
> > > > * subtraction of non 64 bit counters
> > > > - * @mult: cycle to nanosecond multiplier (adjusted by NTP)
> > > > - * @mult_orig: cycle to nanosecond multiplier (unadjusted by NTP)
> > > > + * @mult: cycle to nanosecond multiplier
> > > > * @shift: cycle to nanosecond divisor (power of two)
> > > > * @flags: flags describing special properties
> > > > * @vread: vsyscall based read
> > > > @@ -168,7 +167,6 @@ struct clocksource {
> > > > void (*disable)(struct clocksource *cs);
> > > > cycle_t mask;
> > > > u32 mult;
> > > > - u32 mult_orig;
> > > > u32 shift;
> > > > unsigned long flags;
> > > > cycle_t (*vread)(void);
> > > > Index: linux-2.6/kernel/time/timekeeping.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/kernel/time/timekeeping.c
> > > > +++ linux-2.6/kernel/time/timekeeping.c
> > > > @@ -29,6 +29,8 @@ struct timekeeper {
> > > > s64 ntp_error;
> > > > int xtime_shift;
> > > > int ntp_error_shift;
> > > > + u32 mult; /* NTP adjusted clock multiplier */
> > > > + u32 shift; /* NTP adjusted clock shift */
> > >
> > > I'm a little confused here. NTP doesn't adjust the shift value. This
> > > seems redundant with the xtime_shift value introduced in [patch 9/12].
> >
> > True the NTP code does not adjust the shift value. But mult and shift
> > build a pair, no? I think it is more readable if we use timekeeper.mult
> > and timekeeper.shift instead of timekeeper.mult and clock->shift.
> > And the xtime_shift does not need to be the same value as the clock
> > shift - at least in theory. It is just a shift to increase precision.
> > Therefore I would like to use the xtime_shift and ntp_error_shift values
> > exclusively as the shifts for the respective xtime_nsec and ntp_error
> > values. Makes sense ?
>
> But can't we drop the xtime_shift if we have the clocksource shift kept
> internally? Unless I'm missing something more subtle in your patches,
> they should be the same at all times.

They are the same. Perhaps a good comment in the timekeeper struct is
good enough. Then the patch that introduces the shift values would add
timekeeper.shift, timekeeper.xtime_shift is dropped, and the patch that
add the timekeeper.mult would just use the timekeeper.shift as it is.

--
blue skies,
Martin.

"Reality continues to ruin my life." - Calvin.

2009-07-31 09:00:26

by Martin Schwidefsky

[permalink] [raw]
Subject: Re: [RFC][patch 10/12] move NTP adjusted clock multiplier to struct timekeeper

On Fri, 31 Jul 2009 01:12:59 -0700
john stultz <[email protected]> wrote:

> On Fri, 2009-07-31 at 09:52 +0200, Martin Schwidefsky wrote:
> > On Thu, 30 Jul 2009 15:04:48 -0700
> > john stultz <[email protected]> wrote:
> >
> > > On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > > > plain text document attachment (timekeeper-mult.diff)
> > > > From: Martin Schwidefsky <[email protected]>
> > > >
> > > > The clocksource structure has two multipliers, the unmodified multiplier
> > > > clock->mult_orig and the NTP corrected multiplier clock->mult. The NTP
> > > > multiplier is misplaced in the struct clocksource, this is private
> > > > information of the timekeeping code. Add the mult field to the struct
> > > > timekeeper to contain the NTP corrected value, keep the unmodifed
> > > > multiplier in clock->mult and remove clock->mult_orig. For consistency
> > > > add the shift value associated with the NTP corrected mult value to
> > > > struct timekeeper as well.
> > > >
> > > > Cc: Ingo Molnar <[email protected]>
> > > > Cc: Thomas Gleixner <[email protected]>
> > > > Cc: john stultz <[email protected]>
> > > > Cc: Daniel Walker <[email protected]>
> > > > Signed-off-by: Martin Schwidefsky <[email protected]>
> > > > ---
> > > > arch/arm/plat-omap/common.c | 2 +-
> > > > include/linux/clocksource.h | 4 +---
> > > > kernel/time/timekeeping.c | 43 +++++++++++++++++++++++++------------------
> > > > 3 files changed, 27 insertions(+), 22 deletions(-)
> > > >
> > > > Index: linux-2.6/include/linux/clocksource.h
> > > > ===================================================================
> > > > --- linux-2.6.orig/include/linux/clocksource.h
> > > > +++ linux-2.6/include/linux/clocksource.h
> > > > @@ -149,8 +149,7 @@ extern u64 timecounter_cyc2time(struct t
> > > > * @disable: optional function to disable the clocksource
> > > > * @mask: bitmask for two's complement
> > > > * subtraction of non 64 bit counters
> > > > - * @mult: cycle to nanosecond multiplier (adjusted by NTP)
> > > > - * @mult_orig: cycle to nanosecond multiplier (unadjusted by NTP)
> > > > + * @mult: cycle to nanosecond multiplier
> > > > * @shift: cycle to nanosecond divisor (power of two)
> > > > * @flags: flags describing special properties
> > > > * @vread: vsyscall based read
> > > > @@ -168,7 +167,6 @@ struct clocksource {
> > > > void (*disable)(struct clocksource *cs);
> > > > cycle_t mask;
> > > > u32 mult;
> > > > - u32 mult_orig;
> > > > u32 shift;
> > > > unsigned long flags;
> > > > cycle_t (*vread)(void);
> > > > Index: linux-2.6/kernel/time/timekeeping.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/kernel/time/timekeeping.c
> > > > +++ linux-2.6/kernel/time/timekeeping.c
> > > > @@ -29,6 +29,8 @@ struct timekeeper {
> > > > s64 ntp_error;
> > > > int xtime_shift;
> > > > int ntp_error_shift;
> > > > + u32 mult; /* NTP adjusted clock multiplier */
> > > > + u32 shift; /* NTP adjusted clock shift */
> > >
> > > I'm a little confused here. NTP doesn't adjust the shift value. This
> > > seems redundant with the xtime_shift value introduced in [patch 9/12].
> >
> > True the NTP code does not adjust the shift value. But mult and shift
> > build a pair, no? I think it is more readable if we use timekeeper.mult
> > and timekeeper.shift instead of timekeeper.mult and clock->shift.
> > And the xtime_shift does not need to be the same value as the clock
> > shift - at least in theory. It is just a shift to increase precision.
> > Therefore I would like to use the xtime_shift and ntp_error_shift values
> > exclusively as the shifts for the respective xtime_nsec and ntp_error
> > values. Makes sense ?
>
> But can't we drop the xtime_shift if we have the clocksource shift kept
> internally? Unless I'm missing something more subtle in your patches,
> they should be the same at all times.

How about this definition for struct timekeeper?

/* Structure holding internal timekeeping values. */
struct timekeeper {
/* Current clocksource used for timekeeping. */
struct clocksource *clock;
/* The shift value of the current clocksource. */
int shift;

/* Number of clock cycles in one NTP interval. */
cycle_t cycle_interval;
/* Number of clock shifted nano seconds in one NTP interval. */
u64 xtime_interval;
/* Raw nano seconds accumulated per NTP interval. */
u32 raw_interval;

/* Clock shifted nano seconds remainder not stored in xtime.tv_nsec. */
u64 xtime_nsec;
/* Difference between accumulated time and NTP time in ntp
* shifted nano seconds. */
s64 ntp_error;
/* Shift conversion between clock shifted nano seconds and
* ntp shifted nano seconds. */
int ntp_error_shift;
/* NTP adjusted clock multiplier */
u32 mult;
};

--
blue skies,
Martin.

"Reality continues to ruin my life." - Calvin.

2009-07-31 23:33:02

by john stultz

[permalink] [raw]
Subject: Re: [RFC][patch 10/12] move NTP adjusted clock multiplier to struct timekeeper

On Fri, 2009-07-31 at 11:00 +0200, Martin Schwidefsky wrote:
> On Fri, 31 Jul 2009 01:12:59 -0700
> john stultz <[email protected]> wrote:
>
> > On Fri, 2009-07-31 at 09:52 +0200, Martin Schwidefsky wrote:
> > > On Thu, 30 Jul 2009 15:04:48 -0700
> > > john stultz <[email protected]> wrote:
> > >
> > > > On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > > > > plain text document attachment (timekeeper-mult.diff)
> > > > > From: Martin Schwidefsky <[email protected]>
> > > > >
> > > > > The clocksource structure has two multipliers, the unmodified multiplier
> > > > > clock->mult_orig and the NTP corrected multiplier clock->mult. The NTP
> > > > > multiplier is misplaced in the struct clocksource, this is private
> > > > > information of the timekeeping code. Add the mult field to the struct
> > > > > timekeeper to contain the NTP corrected value, keep the unmodifed
> > > > > multiplier in clock->mult and remove clock->mult_orig. For consistency
> > > > > add the shift value associated with the NTP corrected mult value to
> > > > > struct timekeeper as well.
> > > > >
> > > > > Cc: Ingo Molnar <[email protected]>
> > > > > Cc: Thomas Gleixner <[email protected]>
> > > > > Cc: john stultz <[email protected]>
> > > > > Cc: Daniel Walker <[email protected]>
> > > > > Signed-off-by: Martin Schwidefsky <[email protected]>
> > > > > ---
> > > > > arch/arm/plat-omap/common.c | 2 +-
> > > > > include/linux/clocksource.h | 4 +---
> > > > > kernel/time/timekeeping.c | 43 +++++++++++++++++++++++++------------------
> > > > > 3 files changed, 27 insertions(+), 22 deletions(-)
> > > > >
> > > > > Index: linux-2.6/include/linux/clocksource.h
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/include/linux/clocksource.h
> > > > > +++ linux-2.6/include/linux/clocksource.h
> > > > > @@ -149,8 +149,7 @@ extern u64 timecounter_cyc2time(struct t
> > > > > * @disable: optional function to disable the clocksource
> > > > > * @mask: bitmask for two's complement
> > > > > * subtraction of non 64 bit counters
> > > > > - * @mult: cycle to nanosecond multiplier (adjusted by NTP)
> > > > > - * @mult_orig: cycle to nanosecond multiplier (unadjusted by NTP)
> > > > > + * @mult: cycle to nanosecond multiplier
> > > > > * @shift: cycle to nanosecond divisor (power of two)
> > > > > * @flags: flags describing special properties
> > > > > * @vread: vsyscall based read
> > > > > @@ -168,7 +167,6 @@ struct clocksource {
> > > > > void (*disable)(struct clocksource *cs);
> > > > > cycle_t mask;
> > > > > u32 mult;
> > > > > - u32 mult_orig;
> > > > > u32 shift;
> > > > > unsigned long flags;
> > > > > cycle_t (*vread)(void);
> > > > > Index: linux-2.6/kernel/time/timekeeping.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/kernel/time/timekeeping.c
> > > > > +++ linux-2.6/kernel/time/timekeeping.c
> > > > > @@ -29,6 +29,8 @@ struct timekeeper {
> > > > > s64 ntp_error;
> > > > > int xtime_shift;
> > > > > int ntp_error_shift;
> > > > > + u32 mult; /* NTP adjusted clock multiplier */
> > > > > + u32 shift; /* NTP adjusted clock shift */
> > > >
> > > > I'm a little confused here. NTP doesn't adjust the shift value. This
> > > > seems redundant with the xtime_shift value introduced in [patch 9/12].
> > >
> > > True the NTP code does not adjust the shift value. But mult and shift
> > > build a pair, no? I think it is more readable if we use timekeeper.mult
> > > and timekeeper.shift instead of timekeeper.mult and clock->shift.
> > > And the xtime_shift does not need to be the same value as the clock
> > > shift - at least in theory. It is just a shift to increase precision.
> > > Therefore I would like to use the xtime_shift and ntp_error_shift values
> > > exclusively as the shifts for the respective xtime_nsec and ntp_error
> > > values. Makes sense ?
> >
> > But can't we drop the xtime_shift if we have the clocksource shift kept
> > internally? Unless I'm missing something more subtle in your patches,
> > they should be the same at all times.
>
> How about this definition for struct timekeeper?
>
> /* Structure holding internal timekeeping values. */
> struct timekeeper {
> /* Current clocksource used for timekeeping. */
> struct clocksource *clock;
> /* The shift value of the current clocksource. */
> int shift;
>
> /* Number of clock cycles in one NTP interval. */
> cycle_t cycle_interval;
> /* Number of clock shifted nano seconds in one NTP interval. */
> u64 xtime_interval;
> /* Raw nano seconds accumulated per NTP interval. */

I might just use the term "accumulation interval" instead of NTP
interval. This isn't something that's strictly connected to NTP, other
then they both just need to have some chunk of time that we use as an
comparative interval.


> u32 raw_interval;
>
> /* Clock shifted nano seconds remainder not stored in xtime.tv_nsec. */
> u64 xtime_nsec;
> /* Difference between accumulated time and NTP time in ntp
> * shifted nano seconds. */
> s64 ntp_error;
> /* Shift conversion between clock shifted nano seconds and
> * ntp shifted nano seconds. */
> int ntp_error_shift;
> /* NTP adjusted clock multiplier */
> u32 mult;
> };

Other then that it looks good to me!

thanks
-john