Re: [RFC][patch 10/12] move NTP adjusted clock multiplier tostruct timekeeper

[john stultz]

On Fri, 2009-07-31 at 09:52 +0200, Martin Schwidefsky wrote:
> On Thu, 30 Jul 2009 15:04:48 -0700
> john stultz <johnstul@xxxxxxxxxx> wrote:
> 
> > On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > > plain text document attachment (timekeeper-mult.diff)
> > > From: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
> > > 
> > > 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 <mingo@xxxxxxx>
> > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Cc: john stultz <johnstul@xxxxxxxxxx>
> > > Cc: Daniel Walker <dwalker@xxxxxxxxxx>
> > > Signed-off-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
> > > ---
> > >  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



--
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/