[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 1/7] clocksource: Restructure clocksource struct members

[Thomas Gleixner]

[-- Attachment #1: clocksource-better-layout.patch --]
[-- Type: text/plain, Size: 2119 bytes --]

Group the hot path members of struct clocksource together so we have a
better cache line footprint. Make it cacheline aligned.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/clocksource.h |   32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

Index: linux-2.6-tip/include/linux/clocksource.h
===================================================================
--- linux-2.6-tip.orig/include/linux/clocksource.h
+++ linux-2.6-tip/include/linux/clocksource.h
@@ -159,42 +159,38 @@ extern u64 timecounter_cyc2time(struct t
  */
 struct clocksource {
 	/*
-	 * First part of structure is read mostly
+	 * Hotpath data, fits in a single cache line when the
+	 * clocksource itself is cacheline aligned.
 	 */
-	const char *name;
-	struct list_head list;
-	int rating;
 	cycle_t (*read)(struct clocksource *cs);
-	int (*enable)(struct clocksource *cs);
-	void (*disable)(struct clocksource *cs);
+	cycle_t cycle_last;
 	cycle_t mask;
 	u32 mult;
 	u32 shift;
 	u64 max_idle_ns;
-	unsigned long flags;
-	cycle_t (*vread)(void);
-	void (*suspend)(struct clocksource *cs);
-	void (*resume)(struct clocksource *cs);
+
 #ifdef CONFIG_IA64
 	void *fsys_mmio;        /* used by fsyscall asm code */
 #define CLKSRC_FSYS_MMIO_SET(mmio, addr)      ((mmio) = (addr))
 #else
 #define CLKSRC_FSYS_MMIO_SET(mmio, addr)      do { } while (0)
 #endif
-
-	/*
-	 * Second part is written at each timer interrupt
-	 * Keep it in a different cache line to dirty no
-	 * more than one cache line.
-	 */
-	cycle_t cycle_last ____cacheline_aligned_in_smp;
+	const char *name;
+	struct list_head list;
+	int rating;
+	cycle_t (*vread)(void);
+	int (*enable)(struct clocksource *cs);
+	void (*disable)(struct clocksource *cs);
+	unsigned long flags;
+	void (*suspend)(struct clocksource *cs);
+	void (*resume)(struct clocksource *cs);
 
 #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
 	/* Watchdog related data, used by the framework */
 	struct list_head wd_list;
 	cycle_t wd_last;
 #endif
-};
+} ____cacheline_aligned;
 
 /*
  * Clock source flags bits::

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

[Thomas Gleixner]

[-- Attachment #1: clocksource-make-shift-mult-calc-more-clever.patch --]
[-- Type: text/plain, Size: 2575 bytes --]

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()
 	 */
+	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);

[patch 3/7] clockevents: Restructure clock_event_device members

[Thomas Gleixner]

[-- Attachment #1: ce-better-layout.patch --]
[-- Type: text/plain, Size: 3141 bytes --]

Group the hot path members of struct clock_event_device together so we
have a better cache line footprint. Make it cacheline aligned.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/clockchips.h |   45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

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
@@ -56,46 +56,47 @@ enum clock_event_nofitiers {
 
 /**
  * struct clock_event_device - clock event device descriptor
- * @name:		ptr to clock event name
- * @features:		features
+ * @event_handler:	Assigned by the framework to be called by the low
+ *			level handler of the event source
+ * @set_next_event:	set next event function
+ * @next_event:		local storage for the next event in oneshot mode
  * @max_delta_ns:	maximum delta value in ns
  * @min_delta_ns:	minimum delta value in ns
  * @mult:		nanosecond to cycles multiplier
  * @shift:		nanoseconds to cycles divisor (power of two)
+ * @mode:		operating mode assigned by the management code
+ * @features:		features
+ * @retries:		number of forced programming retries
+ * @set_mode:		set mode function
+ * @broadcast:		function to broadcast events
+ * @name:		ptr to clock event name
  * @rating:		variable to rate clock event devices
  * @irq:		IRQ number (only for non CPU local devices)
  * @cpumask:		cpumask to indicate for which CPUs this device works
- * @set_next_event:	set next event function
- * @set_mode:		set mode function
- * @event_handler:	Assigned by the framework to be called by the low
- *			level handler of the event source
- * @broadcast:		function to broadcast events
  * @list:		list head for the management code
- * @mode:		operating mode assigned by the management code
- * @next_event:		local storage for the next event in oneshot mode
- * @retries:		number of forced programming retries
  */
 struct clock_event_device {
-	const char		*name;
-	unsigned int		features;
+	void			(*event_handler)(struct clock_event_device *);
+	int			(*set_next_event)(unsigned long evt,
+						  struct clock_event_device *);
+	ktime_t			next_event;
 	u64			max_delta_ns;
 	u64			min_delta_ns;
 	u32			mult;
 	u32			shift;
+	enum clock_event_mode	mode;
+	unsigned int		features;
+	unsigned long		retries;
+
+	void			(*broadcast)(const struct cpumask *mask);
+	void			(*set_mode)(enum clock_event_mode mode,
+					    struct clock_event_device *);
+	const char		*name;
 	int			rating;
 	int			irq;
 	const struct cpumask	*cpumask;
-	int			(*set_next_event)(unsigned long evt,
-						  struct clock_event_device *);
-	void			(*set_mode)(enum clock_event_mode mode,
-					    struct clock_event_device *);
-	void			(*event_handler)(struct clock_event_device *);
-	void			(*broadcast)(const struct cpumask *mask);
 	struct list_head	list;
-	enum clock_event_mode	mode;
-	ktime_t			next_event;
-	unsigned long		retries;
-};
+} ____cacheline_aligned;
 
 /*
  * Calculate a multiplication factor for scaled math, which is used to convert

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

[Thomas Gleixner]

[-- Attachment #1: ce-add-register-if.patch --]
[-- Type: text/plain, Size: 3646 bytes --]

All clockevent devices have the same open coded initialization
functions. Provide an interface which does all necessary
initialization in the core code.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/clockchips.h |    9 +++++++++
 kernel/time/clockevents.c  |   44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 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
@@ -69,6 +69,8 @@ enum clock_event_nofitiers {
  * @retries:		number of forced programming retries
  * @set_mode:		set mode function
  * @broadcast:		function to broadcast events
+ * @min_delta_ticks:	minimum delta value in ticks stored for reconfiguration
+ * @max_delta_ticks:	maximum delta value in ticks stored for reconfiguration
  * @name:		ptr to clock event name
  * @rating:		variable to rate clock event devices
  * @irq:		IRQ number (only for non CPU local devices)
@@ -91,6 +93,9 @@ struct clock_event_device {
 	void			(*broadcast)(const struct cpumask *mask);
 	void			(*set_mode)(enum clock_event_mode mode,
 					    struct clock_event_device *);
+	unsigned long		min_delta_ticks;
+	unsigned long		max_delta_ticks;
+
 	const char		*name;
 	int			rating;
 	int			irq;
@@ -123,6 +128,10 @@ extern u64 clockevent_delta2ns(unsigned 
 			       struct clock_event_device *evt);
 extern void clockevents_register_device(struct clock_event_device *dev);
 
+extern void clockevents_config_and_register(struct clock_event_device *dev,
+					    u32 freq, unsigned long min_delta,
+					    unsigned long max_delta);
+
 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
@@ -194,6 +194,50 @@ void clockevents_register_device(struct 
 }
 EXPORT_SYMBOL_GPL(clockevents_register_device);
 
+static void clockevents_config(struct clock_event_device *dev,
+			       u32 freq)
+{
+	unsigned long sec;
+
+	if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
+		return;
+
+	/*
+	 * Calculate the maximum number of seconds we can sleep. Limit
+	 * to 10 minutes for hardware which can program more than
+	 * 32bit ticks so we still get reasonable conversion values.
+	 */
+	sec = dev->max_delta_ticks;
+	do_div(sec, freq);
+	if (!sec)
+		sec = 1;
+	else if (sec > 600 && dev->max_delta_ticks > UINT_MAX)
+		sec = 600;
+
+	clockevents_calc_mult_shift(dev, freq, sec);
+	dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
+	dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
+}
+
+/**
+ * clockevents_config_and_register - Configure and register a clock event device
+ * @dev:	device to register
+ * @freq:	The clock frequency
+ * @min_delta:	The minimum clock ticks to program in oneshot mode
+ * @max_delta:	The maximum clock ticks to program in oneshot mode
+ *
+ * min/max_delta can be 0 for devices which do not support oneshot mode.
+ */
+void clockevents_config_and_register(struct clock_event_device *dev,
+				     u32 freq, unsigned long min_delta,
+				     unsigned long max_delta)
+{
+	dev->min_delta_ticks = min_delta;
+	dev->max_delta_ticks = max_delta;
+	clockevents_config(dev, freq);
+	clockevents_register_device(dev);
+}
+
 /*
  * Noop handler when we shut down an event device
  */

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

[Thomas Gleixner]

[-- Attachment #1: x86-pit-convert-ce.patch --]
[-- Type: text/plain, Size: 1089 bytes --]

Let the core do the work.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/i8253.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Index: linux-2.6-tip/arch/x86/kernel/i8253.c
===================================================================
--- linux-2.6-tip.orig/arch/x86/kernel/i8253.c
+++ linux-2.6-tip/arch/x86/kernel/i8253.c
@@ -93,7 +93,6 @@ static struct clock_event_device pit_ce 
 	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
 	.set_mode	= init_pit_timer,
 	.set_next_event = pit_next_event,
-	.shift		= 32,
 	.irq		= 0,
 };
 
@@ -108,11 +107,8 @@ void __init setup_pit_timer(void)
 	 * IO_APIC has been initialized.
 	 */
 	pit_ce.cpumask = cpumask_of(smp_processor_id());
-	pit_ce.mult = div_sc(CLOCK_TICK_RATE, NSEC_PER_SEC, pit_ce.shift);
-	pit_ce.max_delta_ns = clockevent_delta2ns(0x7FFF, &pit_ce);
-	pit_ce.min_delta_ns = clockevent_delta2ns(0xF, &pit_ce);
 
-	clockevents_register_device(&pit_ce);
+	clockevents_config_and_register(&pit_ce, CLOCK_TICK_RATE, 0xF, 0x7FFF);
 	global_clock_event = &pit_ce;
 }
 

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

[Thomas Gleixner]

[-- Attachment #1: re-arm-twd-adjust-localtimer-frequency-withcpufreqnotifiers.patch --]
[-- Type: text/plain, Size: 2038 bytes --]

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)
+{
+	clockevents_config(dev, freq);
+
+	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
+		return 0;
+
+	return clockevents_program_event(dev, dev->next_event, ktime_get());
+}
+
 /*
  * Noop handler when we shut down an event device
  */

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

[Thomas Gleixner]

[-- Attachment #1: x86-hept-convert.patch --]
[-- Type: text/plain, Size: 4936 bytes --]

No need to recalculate the frequency and the conversion factors over
and over. Calculate the frequency once and use the new config/register
interface and let the core code do the math.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/hpet.c |   71 ++++++++++---------------------------------------
 1 file changed, 15 insertions(+), 56 deletions(-)

Index: linux-2.6-tip/arch/x86/kernel/hpet.c
===================================================================
--- linux-2.6-tip.orig/arch/x86/kernel/hpet.c
+++ linux-2.6-tip/arch/x86/kernel/hpet.c
@@ -217,7 +217,7 @@ static void hpet_reserve_platform_timers
 /*
  * Common hpet info
  */
-static unsigned long hpet_period;
+static unsigned long hpet_freq;
 
 static void hpet_legacy_set_mode(enum clock_event_mode mode,
 			  struct clock_event_device *evt);
@@ -232,7 +232,6 @@ static struct clock_event_device hpet_cl
 	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
 	.set_mode	= hpet_legacy_set_mode,
 	.set_next_event = hpet_legacy_next_event,
-	.shift		= 32,
 	.irq		= 0,
 	.rating		= 50,
 };
@@ -290,28 +289,12 @@ static void hpet_legacy_clockevent_regis
 	hpet_enable_legacy_int();
 
 	/*
-	 * The mult factor is defined as (include/linux/clockchips.h)
-	 *  mult/2^shift = cyc/ns (in contrast to ns/cyc in clocksource.h)
-	 * hpet_period is in units of femtoseconds (per cycle), so
-	 *  mult/2^shift = cyc/ns = 10^6/hpet_period
-	 *  mult = (10^6 * 2^shift)/hpet_period
-	 *  mult = (FSEC_PER_NSEC << hpet_clockevent.shift)/hpet_period
-	 */
-	hpet_clockevent.mult = div_sc((unsigned long) FSEC_PER_NSEC,
-				      hpet_period, hpet_clockevent.shift);
-	/* Calculate the min / max delta */
-	hpet_clockevent.max_delta_ns = clockevent_delta2ns(0x7FFFFFFF,
-							   &hpet_clockevent);
-	/* Setup minimum reprogramming delta. */
-	hpet_clockevent.min_delta_ns = clockevent_delta2ns(HPET_MIN_PROG_DELTA,
-							   &hpet_clockevent);
-
-	/*
 	 * Start hpet with the boot cpu mask and make it
 	 * global after the IO_APIC has been initialized.
 	 */
 	hpet_clockevent.cpumask = cpumask_of(smp_processor_id());
-	clockevents_register_device(&hpet_clockevent);
+	clockevents_config_and_register(&hpet_clockevent, hpet_freq,
+					HPET_MIN_PROG_DELTA, 0x7FFFFFFF);
 	global_clock_event = &hpet_clockevent;
 	printk(KERN_DEBUG "hpet clockevent registered\n");
 }
@@ -549,7 +532,6 @@ static int hpet_setup_irq(struct hpet_de
 static void init_one_hpet_msi_clockevent(struct hpet_dev *hdev, int cpu)
 {
 	struct clock_event_device *evt = &hdev->evt;
-	uint64_t hpet_freq;
 
 	WARN_ON(cpu != smp_processor_id());
 	if (!(hdev->flags & HPET_DEV_VALID))
@@ -571,24 +553,9 @@ static void init_one_hpet_msi_clockevent
 
 	evt->set_mode = hpet_msi_set_mode;
 	evt->set_next_event = hpet_msi_next_event;
-	evt->shift = 32;
-
-	/*
-	 * The period is a femto seconds value. We need to calculate the
-	 * scaled math multiplication factor for nanosecond to hpet tick
-	 * conversion.
-	 */
-	hpet_freq = FSEC_PER_SEC;
-	do_div(hpet_freq, hpet_period);
-	evt->mult = div_sc((unsigned long) hpet_freq,
-				      NSEC_PER_SEC, evt->shift);
-	/* Calculate the max delta */
-	evt->max_delta_ns = clockevent_delta2ns(0x7FFFFFFF, evt);
-	/* 5 usec minimum reprogramming delta. */
-	evt->min_delta_ns = 5000;
-
 	evt->cpumask = cpumask_of(hdev->cpu);
-	clockevents_register_device(evt);
+
+	clockevents_config_and_register(evt, hpet_freq, 5000, 0x7FFFFFFF);
 }
 
 #ifdef CONFIG_HPET
@@ -792,7 +759,6 @@ static struct clocksource clocksource_hp
 static int hpet_clocksource_register(void)
 {
 	u64 start, now;
-	u64 hpet_freq;
 	cycle_t t1;
 
 	/* Start the counter */
@@ -819,24 +785,7 @@ static int hpet_clocksource_register(voi
 		return -ENODEV;
 	}
 
-	/*
-	 * The definition of mult is (include/linux/clocksource.h)
-	 * mult/2^shift = ns/cyc and hpet_period is in units of fsec/cyc
-	 * so we first need to convert hpet_period to ns/cyc units:
-	 *  mult/2^shift = ns/cyc = hpet_period/10^6
-	 *  mult = (hpet_period * 2^shift)/10^6
-	 *  mult = (hpet_period << shift)/FSEC_PER_NSEC
-	 */
-
-	/* Need to convert hpet_period (fsec/cyc) to cyc/sec:
-	 *
-	 * cyc/sec = FSEC_PER_SEC/hpet_period(fsec/cyc)
-	 * cyc/sec = (FSEC_PER_NSEC * NSEC_PER_SEC)/hpet_period
-	 */
-	hpet_freq = FSEC_PER_SEC;
-	do_div(hpet_freq, hpet_period);
 	clocksource_register_hz(&clocksource_hpet, (u32)hpet_freq);
-
 	return 0;
 }
 
@@ -845,7 +794,9 @@ static int hpet_clocksource_register(voi
  */
 int __init hpet_enable(void)
 {
+	unsigned long hpet_period;
 	unsigned int id;
+	u64 freq;
 	int i;
 
 	if (!is_hpet_capable())
@@ -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;
+
+	/*
 	 * Read the HPET ID register to retrieve the IRQ routing
 	 * information and the number of channels
 	 */

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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