[patch 0/7] clocksources/clockevents improvements

[patch 0/7] clocksources/clockevents improvements

[Thomas Gleixner]

The series provides a few improvements to the core code of
clocksources and clockevents.

1) Better layout and cacheline alignment of the hotpath data in both
   data structures

2) Get rid of the hardcoded 5 seconds sleep limit in clocksources

3) Provide a generic configuration and registration interface for
   clockevents which does all the common math tasks so we can remove
   copied code all over the place

4) Provide a function to reconfigure an active clock event
   device. Needed by devices which are affected by frequency scaling.

The last two patches are x86 specific and make use of the new
config_register() functions.

Thanks,

	tglx

[patch 2/7] clocksource: Get rid of the hardcoded 5 seconds sleep time limit

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: clocksource-make-shift-mult-calc-more-clever.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110518/38e016ce/attachment.ksh>

[patch 1/7] clocksource: Restructure clocksource struct members

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: clocksource-better-layout.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110518/a74334d0/attachment.ksh>

[patch 4/7] clockevents: Provide combined configure and register function

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: ce-add-register-if.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110518/c76bd52d/attachment.ksh>

[patch 3/7] clockevents: Restructure clock_event_device members

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: ce-better-layout.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110518/bdcd4ee4/attachment.ksh>

[patch 6/7] x86: Convert PIT to clockevents_config_and_register()

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: x86-pit-convert-ce.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110518/5e3ec5d1/attachment.ksh>

[patch 5/7] clockevents: Provide interface to reconfigure an active clock event device

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: re-arm-twd-adjust-localtimer-frequency-withcpufreqnotifiers.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110518/ae8880bd/attachment.ksh>

[patch 7/7] x86: hpet: Cleanup the clockevents init and register code

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: x86-hept-convert.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110518/8c05ec70/attachment.ksh>

[patch 2/7] clocksource: Get rid of the hardcoded 5 seconds sleep time limit

[John Stultz]

On Wed, 2011-05-18 at 21:33 +0000, Thomas Gleixner wrote:
> plain text document attachment
> (clocksource-make-shift-mult-calc-more-clever.patch)
> Slow clocksources can have a way longer sleep time than 5 seconds and
> even fast ones can easily cope with 600 seconds and still maintain
> proper accuracy.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/time/clocksource.c |   38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> Index: linux-2.6-tip/kernel/time/clocksource.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/time/clocksource.c
> +++ linux-2.6-tip/kernel/time/clocksource.c
> @@ -626,19 +626,6 @@ static void clocksource_enqueue(struct c
>  	list_add(&cs->list, entry);
>  }
> 
> -
> -/*
> - * Maximum time we expect to go between ticks. This includes idle
> - * tickless time. It provides the trade off between selecting a
> - * mult/shift pair that is very precise but can only handle a short
> - * period of time, vs. a mult/shift pair that can handle long periods
> - * of time but isn't as precise.
> - *
> - * This is a subsystem constant, and actual hardware limitations
> - * may override it (ie: clocksources that wrap every 3 seconds).
> - */
> -#define MAX_UPDATE_LENGTH 5 /* Seconds */
> -
>  /**
>   * __clocksource_updatefreq_scale - Used update clocksource with new freq
>   * @t:		clocksource to be registered
> @@ -652,15 +639,28 @@ static void clocksource_enqueue(struct c
>   */
>  void __clocksource_updatefreq_scale(struct clocksource *cs, u32 scale, u32 freq)
>  {
> +	unsigned long sec;
> +
>  	/*
> -	 * Ideally we want to use  some of the limits used in
> -	 * clocksource_max_deferment, to provide a more informed
> -	 * MAX_UPDATE_LENGTH. But for now this just gets the
> -	 * register interface working properly.
> +	 * Calc the maximum number of seconds which we can run before
> +	 * wrapping around. For clocksources which have a mask > 32bit
> +	 * we need to limit the max sleep time to have a good
> +	 * conversion precision. 10 minutes is still a reasonable
> +	 * amount. That results in a shift value of 24 for a
> +	 * clocksource with mask >= 40bit and f >= 4GHz. That maps to
> +	 * ~ 0.06ppm granularity for NTP. We apply the same 12.5%
> +	 * margin as we do in clocksource_max_deferment()
>  	 */

So, its not clear to me that the 12.5% margin really needed, since we
take it out when we calculate clocksource_max_deferment(). Although with
or without the margin I get the same mult/shift/max_idle_ns values for
the hardware I'm testing.

Another nice detail from the change: It doesn't affect clocksources that
normally wrap quickly:

Without your patch:
--------------
JDB: hpet mult: 2684354560 shift: 26 max_idle: 83214991360
JDB: acpi_pm mult: 2343484437 shift: 23 max_idle: 4540500826
JDB: tsc mult: 1342183991 shift: 31 max_idle: 2600481483

With your patch:
---------------
JDB: hpet mult: 2684354560 shift: 26 max_idle: 83214991360
JDB: acpi_pm mult: 2343484437 shift: 23 max_idle: 4540500826
JDB: tsc mult: 10485812 shift: 24 max_idle: 332861616128

Although interestingly 332 secs calculated for the max idle is more then
12% off of 600.

> +	sec = (cs->mask - (cs->mask >> 5));
> +	do_div(sec, freq);
> +	do_div(sec, scale);
> +	if (!sec)
> +		sec = 1;
> +	else if (sec > 600 && cs->mask > UINT_MAX)
> +		sec = 600;
> +
>  	clocks_calc_mult_shift(&cs->mult, &cs->shift, freq,
> -				      NSEC_PER_SEC/scale,
> -				      MAX_UPDATE_LENGTH*scale);
> +			       NSEC_PER_SEC / scale, sec * scale);
>  	cs->max_idle_ns = clocksource_max_deferment(cs);
>  }
>  EXPORT_SYMBOL_GPL(__clocksource_updatefreq_scale);

Overall it looks good. I'm doing some NTP testing with the TSC shift
change to make sure we don't get any odd side effects. I'll let you know
how those go.

thanks
-john

[patch 5/7] clockevents: Provide interface to reconfigure an active clock event device

[Linus Walleij]

2011/5/18 Thomas Gleixner <tglx@linutronix.de>:

> Some ARM SoCs have clock event devices which have their frequency
> modified due to frequency scaling. Provide an interface which allows
> to reconfigure an active device. After reconfiguration reprogram the
> current pending event.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for fixing this much needed function Thomas!
Linus Walleij

[patch 2/7] clocksource: Get rid of the hardcoded 5 seconds sleep time limit

[Thomas Gleixner]

On Wed, 18 May 2011, John Stultz wrote:
> On Wed, 2011-05-18 at 21:33 +0000, Thomas Gleixner wrote:
> > -	 * Ideally we want to use  some of the limits used in
> > -	 * clocksource_max_deferment, to provide a more informed
> > -	 * MAX_UPDATE_LENGTH. But for now this just gets the
> > -	 * register interface working properly.
> > +	 * Calc the maximum number of seconds which we can run before
> > +	 * wrapping around. For clocksources which have a mask > 32bit
> > +	 * we need to limit the max sleep time to have a good
> > +	 * conversion precision. 10 minutes is still a reasonable
> > +	 * amount. That results in a shift value of 24 for a
> > +	 * clocksource with mask >= 40bit and f >= 4GHz. That maps to
> > +	 * ~ 0.06ppm granularity for NTP. We apply the same 12.5%
> > +	 * margin as we do in clocksource_max_deferment()
> >  	 */
> 
> So, its not clear to me that the 12.5% margin really needed, since we
> take it out when we calculate clocksource_max_deferment(). Although with
> or without the margin I get the same mult/shift/max_idle_ns values for
> the hardware I'm testing.
> 
> Another nice detail from the change: It doesn't affect clocksources that
> normally wrap quickly:
> 
> Without your patch:
> --------------
> JDB: hpet mult: 2684354560 shift: 26 max_idle: 83214991360
> JDB: acpi_pm mult: 2343484437 shift: 23 max_idle: 4540500826
> JDB: tsc mult: 1342183991 shift: 31 max_idle: 2600481483
> 
> With your patch:
> ---------------
> JDB: hpet mult: 2684354560 shift: 26 max_idle: 83214991360
> JDB: acpi_pm mult: 2343484437 shift: 23 max_idle: 4540500826
> JDB: tsc mult: 10485812 shift: 24 max_idle: 332861616128
> 
> Although interestingly 332 secs calculated for the max idle is more then
> 12% off of 600.

We probably need to look at the math of clocksource_max_deferment()
again.
 
> Overall it looks good. I'm doing some NTP testing with the TSC shift
> change to make sure we don't get any odd side effects. I'll let you know
> how those go.

Ok.

Thanks,

	tglx

[patch 4/7] clockevents: Provide combined configure and register function

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> +extern void clockevents_config_and_register(struct clock_event_device *dev,
> +					    u32 freq, unsigned long min_delta,
> +					    unsigned long max_delta);

might be worth collecting these fields into a clocksource_params structure:

struct clocksource_params {
	u32		freq;
	unsigned long	min_delta;
	unsigned long	max_delta;
};

That way the initialization API looks even more streamlined:

extern void
clockevents_config_and_register(struct clock_event_device *dev,
				struct clocksource_params params);

and could be extended in the future, without having to update every single 
clocksource driver again.

But a good first step in any case:

Reviewed-by: Ingo Molnar <mingo@elte.hu>

Thanks,

	Ingo

[patch 5/7] clockevents: Provide interface to reconfigure an active clock event device

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> Some ARM SoCs have clock event devices which have their frequency
> modified due to frequency scaling. Provide an interface which allows
> to reconfigure an active device. After reconfiguration reprogram the
> current pending event.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/clockchips.h |    2 ++
>  kernel/time/clockevents.c  |   20 ++++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> Index: linux-2.6-tip/include/linux/clockchips.h
> ===================================================================
> --- linux-2.6-tip.orig/include/linux/clockchips.h
> +++ linux-2.6-tip/include/linux/clockchips.h
> @@ -132,6 +132,8 @@ extern void clockevents_config_and_regis
>  					    u32 freq, unsigned long min_delta,
>  					    unsigned long max_delta);
>  
> +extern int clockevents_reconfigure(struct clock_event_device *ce, u32 freq);
> +
>  extern void clockevents_exchange_device(struct clock_event_device *old,
>  					struct clock_event_device *new);
>  extern void clockevents_set_mode(struct clock_event_device *dev,
> Index: linux-2.6-tip/kernel/time/clockevents.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/time/clockevents.c
> +++ linux-2.6-tip/kernel/time/clockevents.c
> @@ -238,6 +238,26 @@ void clockevents_config_and_register(str
>  	clockevents_register_device(dev);
>  }
>  
> +/**
> + * clockevents_reconfigure - Reconfigure and reprogram a clock event device.
> + * @dev:	device to modify
> + * @freq:	new device frequency
> + *
> + * Reconfigure and reprogram a clock event device in oneshot
> + * mode. Must be called on the cpu for which the device delivers per
> + * cpu timer events with interrupts disabled!  Returns 0 on success,
> + * -ETIME when the event is in the past.
> + */
> +int clockevents_reconfigure(struct clock_event_device *dev, u32 freq)

This too could use a struct clockevents_params perhaps - and would only use 
params.freq for now but might be extended in the future.

But i'm fine with this API as well:

Reviewed-by: Ingo Molnar <mingo@elte.hu>

Thanks,

	Ingo

[patch 7/7] x86: hpet: Cleanup the clockevents init and register code

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> @@ -884,6 +835,14 @@ int __init hpet_enable(void)
>  		goto out_nohpet;
>  
>  	/*
> +	 * The period is a femto seconds value. Convert it to a
> +	 * frequency.
> +	 */
> +	freq = FSEC_PER_SEC;
> +	do_div(freq, hpet_period);
> +	hpet_freq = freq;

Something i just noticed: with a typical hpet frequency of around 14 MHz we get 
a period of 71428571 femtoseconds.

Our HPET_MAX_PERIOD is 100000000 at the moment, so our limits look like this:

    100000
  71428571
 100000000

Note how close the max period (lowest frequency) is to our typical value!

So if there's a 10 MHz hpet somewhere, with just slightly below spec, we'd fail 
due to:

        if (hpet_period < HPET_MIN_PERIOD || hpet_period > HPET_MAX_PERIOD)
                goto out_nohpet;

unless i got my numbers wrong it might be worth upping the max period to 
1000000000, to allow down to 1 MHz hpet frequencies. Or at least up it enough 
to make 10 MHz possible modulo small noise.

Patch looks good:

Reviewed-by: Ingo Molnar <mingo@elte.hu>

Thanks,

	Ingo

[patch 5/7] clockevents: Provide interface to reconfigure an active clock event device

[Thomas Gleixner]

On Thu, 19 May 2011, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> > + */
> > +int clockevents_reconfigure(struct clock_event_device *dev, u32 freq)
> 
> This too could use a struct clockevents_params perhaps - and would only use 
> params.freq for now but might be extended in the future.
> 
> But i'm fine with this API as well:

I agree for the config_register interface, but here we really just
want to update the frequency and not change any other parameter.
 
Thanks,

	tglx

[patch 0/7] clocksources/clockevents improvements

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> The series provides a few improvements to the core code of
> clocksources and clockevents.
> 
> 1) Better layout and cacheline alignment of the hotpath data in both
>    data structures
> 
> 2) Get rid of the hardcoded 5 seconds sleep limit in clocksources
> 
> 3) Provide a generic configuration and registration interface for
>    clockevents which does all the common math tasks so we can remove
>    copied code all over the place
> 
> 4) Provide a function to reconfigure an active clock event
>    device. Needed by devices which are affected by frequency scaling.
> 
> The last two patches are x86 specific and make use of the new
> config_register() functions.

Nice! Other than the comments i made for specific patches it's looking good to 
me:

Reviewed-by: Ingo Molnar <mingo@elte.hu>

Thanks,

	Ingo

[patch 5/7] clockevents: Provide interface to reconfigure an active clock event device

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 19 May 2011, Ingo Molnar wrote:
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> > > + */
> > > +int clockevents_reconfigure(struct clock_event_device *dev, u32 freq)
> > 
> > This too could use a struct clockevents_params perhaps - and would only use 
> > params.freq for now but might be extended in the future.
> > 
> > But i'm fine with this API as well:
> 
> I agree for the config_register interface, but here we really just
> want to update the frequency and not change any other parameter.

ok, fair enough - in that case i'd suggest making that property explicitly 
visible in the API name, via something like:

	int clockevents_set_freq(struct clock_event_device *dev, u32 freq)

That way it's obvious at first sight what this does:

	clockevents_set_freq(dev, 1000000);

Versus:

	clockevents_reconfigure(dev, 1000000);

Thanks,

	Ingo

[patch 4/7] clockevents: Provide combined configure and register function

[Thomas Gleixner]

On Thu, 19 May 2011, Ingo Molnar wrote:

> 
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > +extern void clockevents_config_and_register(struct clock_event_device *dev,
> > +					    u32 freq, unsigned long min_delta,
> > +					    unsigned long max_delta);
> 
> might be worth collecting these fields into a clocksource_params structure:
> 
> struct clocksource_params {
> 	u32		freq;
> 	unsigned long	min_delta;
> 	unsigned long	max_delta;
> };
> 
> That way the initialization API looks even more streamlined:
> 
> extern void
> clockevents_config_and_register(struct clock_event_device *dev,
> 				struct clocksource_params params);
> 
> and could be extended in the future, without having to update every single 
> clocksource driver again.

Though it's unlikely that we have more params int the foreseeable
future and it's more code in the clock events registration sites as
you have to do:

    struct param par;

    par.freq = f;
    par.min_delta = x;
    par.max_delta = y;
    clockevents_config_and_register(&dev, &p);

instead of having a single line.

When the need arises we still can do

__clockevents_config_and_register(struct clock_event_device *dev,
   	                          struct clocksource_params params);

and have the current function as a wrapper around it.

Thanks,

	tglx

[patch 4/7] clockevents: Provide combined configure and register function

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 19 May 2011, Ingo Molnar wrote:
> 
> > 
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > +extern void clockevents_config_and_register(struct clock_event_device *dev,
> > > +					    u32 freq, unsigned long min_delta,
> > > +					    unsigned long max_delta);
> > 
> > might be worth collecting these fields into a clocksource_params structure:
> > 
> > struct clocksource_params {
> > 	u32		freq;
> > 	unsigned long	min_delta;
> > 	unsigned long	max_delta;
> > };
> > 
> > That way the initialization API looks even more streamlined:
> > 
> > extern void
> > clockevents_config_and_register(struct clock_event_device *dev,
> > 				struct clocksource_params params);
> > 
> > and could be extended in the future, without having to update every single 
> > clocksource driver again.
> 
> Though it's unlikely that we have more params int the foreseeable
> future and it's more code in the clock events registration sites as
> you have to do:
> 
>     struct param par;
> 
>     par.freq = f;
>     par.min_delta = x;
>     par.max_delta = y;
>     clockevents_config_and_register(&dev, &p);
> 
> instead of having a single line.

Indeed that looks uglish.

> When the need arises we still can do
> 
> __clockevents_config_and_register(struct clock_event_device *dev,
>    	                          struct clocksource_params params);
> 
> and have the current function as a wrapper around it.

Okay, you are right.

Thanks,

	Ingo

[patch 2/7] clocksource: Get rid of the hardcoded 5 seconds sleep time limit

[john stultz]

On Wed, 2011-05-18 at 17:57 -0700, John Stultz wrote:
> Overall it looks good. I'm doing some NTP testing with the TSC shift
> change to make sure we don't get any odd side effects. I'll let you know
> how those go.

So from overnight testing all looks well.

thanks
-john