[RFC][patch 00/12] clocksource / timekeeping rework V2

[RFC][patch 00/12] clocksource / timekeeping rework V2

[Martin Schwidefsky]

Greetings,
version 2 of the clocksource / timekeeping cleanup patches. The series
has grown quite a bit, what started with a simple idea to replace the
tick based clocksource update with stop_machine is now a full fledged
code rework.

The code is working on s390 and on my Athlon system at home which has
a broken tsc clocksource:

[    0.000000] Fast TSC calibration using PIT
[    0.000341] hpet clockevent registered
[    0.000343] HPET: 3 timers in total, 0 timers will be used for per-cpu timer
[    0.204021] hpet0: at MMIO 0xfefff000, IRQs 2, 8, 31
[    0.204027] hpet0: 3 comparators, 32-bit 25.000000 MHz counter
[    0.208007] Switching to clock hpet
[    0.211544] Switched to high resolution mode on CPU 0
[    0.211960] Switched to high resolution mode on CPU 1
[    8.000020] Clocksource tsc unstable (delta = -172310085 ns)

So the clocksource switch via stop_machine and the clocksource watchdog
are working. I keep the fingers crossed that nothing else breaks.

The patch set is based on todays upstream tree plus the patches from
the tip tree, if anyone wants to try them you need to pull from the
master branch of
    git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-tip

There is still more room for improvement. Some sore points are:

1) The cycle_last value still is in the struct clocksource. It should
   be in the struct timekeeper but the check against cycles_last in the
   read function of the TSC clock source makes it hard.
2) read_persistent_clock returns seconds. With a really good initial
   time source this is not very precise. read_persistent_clock should
   return a struct timespec.
3) xtime, raw_time, total_sleep_time, timekeeping_suspended, jiffies,
   the ntp state and probably a few other values may be better located
   in the struct timekeeper as well.

and a few more I forgot.

Many thanks to John who pushed me into the right directions.
-- 
blue skies,
   Martin.

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

[RFC][patch 01/12] introduce timekeeping_leap_insert

[Martin Schwidefsky]

[-- Attachment #1: leap_insert.diff --]
[-- Type: text/plain, Size: 2213 bytes --]

From: john stultz <johnstul@us.ibm.com>

---
 include/linux/time.h      |    1 +
 kernel/time/ntp.c         |    7 ++-----
 kernel/time/timekeeping.c |    7 +++++++
 3 files changed, 10 insertions(+), 5 deletions(-)

Index: linux-2.6/include/linux/time.h
===================================================================
--- linux-2.6.orig/include/linux/time.h
+++ linux-2.6/include/linux/time.h
@@ -147,6 +147,7 @@ extern struct timespec timespec_trunc(st
 extern int timekeeping_valid_for_hres(void);
 extern void update_wall_time(void);
 extern void update_xtime_cache(u64 nsec);
+extern void timekeeping_leap_insert(int leapsecond);
 
 struct tms;
 extern void do_sys_times(struct tms *);
Index: linux-2.6/kernel/time/ntp.c
===================================================================
--- linux-2.6.orig/kernel/time/ntp.c
+++ linux-2.6/kernel/time/ntp.c
@@ -194,8 +194,7 @@ static enum hrtimer_restart ntp_leap_sec
 	case TIME_OK:
 		break;
 	case TIME_INS:
-		xtime.tv_sec--;
-		wall_to_monotonic.tv_sec++;
+		timekeeping_leap_insert(-1);
 		time_state = TIME_OOP;
 		printk(KERN_NOTICE
 			"Clock: inserting leap second 23:59:60 UTC\n");
@@ -203,9 +202,8 @@ static enum hrtimer_restart ntp_leap_sec
 		res = HRTIMER_RESTART;
 		break;
 	case TIME_DEL:
-		xtime.tv_sec++;
+		timekeeping_leap_insert(1);
 		time_tai--;
-		wall_to_monotonic.tv_sec--;
 		time_state = TIME_WAIT;
 		printk(KERN_NOTICE
 			"Clock: deleting leap second 23:59:59 UTC\n");
@@ -219,7 +217,6 @@ static enum hrtimer_restart ntp_leap_sec
 			time_state = TIME_OK;
 		break;
 	}
-	update_vsyscall(&xtime, clock);
 
 	write_sequnlock(&xtime_lock);
 
Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -58,6 +58,13 @@ void update_xtime_cache(u64 nsec)
 
 struct clocksource *clock;
 
+/* must hold xtime_lock */
+void timekeeping_leap_insert(int leapsecond)
+{
+	xtime.tv_sec += leapsecond;
+	wall_to_monotonic.tv_sec -= leapsecond;
+	update_vsyscall(&xtime, clock);
+}
 
 #ifdef CONFIG_GENERIC_TIME
 /**

-- 
blue skies,
   Martin.

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

[RFC][patch 02/12] remove clocksource inline functions

[Martin Schwidefsky]

[-- Attachment #1: clocksource-inline.diff --]
[-- Type: text/plain, Size: 5591 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Remove clocksource_read, clocksource_enable and clocksource_disable
inline functions. No functional change.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 include/linux/clocksource.h |   46 --------------------------------------------
 kernel/time/timekeeping.c   |   32 +++++++++++++++++-------------
 2 files changed, 18 insertions(+), 60 deletions(-)

Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -79,7 +79,7 @@ static void clocksource_forward_now(void
 	cycle_t cycle_now, cycle_delta;
 	s64 nsec;
 
-	cycle_now = clocksource_read(clock);
+	cycle_now = clock->read(clock);
 	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 	clock->cycle_last = cycle_now;
 
@@ -114,7 +114,7 @@ void getnstimeofday(struct timespec *ts)
 		*ts = xtime;
 
 		/* read clocksource: */
-		cycle_now = clocksource_read(clock);
+		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -146,7 +146,7 @@ ktime_t ktime_get(void)
 		nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
 
 		/* read clocksource: */
-		cycle_now = clocksource_read(clock);
+		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -186,7 +186,7 @@ void ktime_get_ts(struct timespec *ts)
 		tomono = wall_to_monotonic;
 
 		/* read clocksource: */
-		cycle_now = clocksource_read(clock);
+		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -274,16 +274,18 @@ static void change_clocksource(void)
 
 	clocksource_forward_now();
 
-	if (clocksource_enable(new))
+	if (new->enable && ! new->enable(new))
 		return;
+	/* save mult_orig on enable */
+	new->mult_orig = new->mult;
 
 	new->raw_time = clock->raw_time;
 	old = clock;
 	clock = new;
-	clocksource_disable(old);
+	if (old->disable)
+		old->disable(old);
 
-	clock->cycle_last = 0;
-	clock->cycle_last = clocksource_read(clock);
+	clock->cycle_last = clock->read(clock);
 	clock->error = 0;
 	clock->xtime_nsec = 0;
 	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
@@ -373,7 +375,7 @@ void getrawmonotonic(struct timespec *ts
 		seq = read_seqbegin(&xtime_lock);
 
 		/* read clocksource: */
-		cycle_now = clocksource_read(clock);
+		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -435,9 +437,12 @@ void __init timekeeping_init(void)
 	ntp_init();
 
 	clock = clocksource_get_next();
-	clocksource_enable(clock);
+	if (clock->enable)
+		clock->enable(clock);
+	/* save mult_orig on enable */
+	clock->mult_orig = clock->mult;
 	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
-	clock->cycle_last = clocksource_read(clock);
+	clock->cycle_last = clock->read(clock);
 
 	xtime.tv_sec = sec;
 	xtime.tv_nsec = 0;
@@ -477,8 +482,7 @@ static int timekeeping_resume(struct sys
 	}
 	update_xtime_cache(0);
 	/* re-base the last cycle value */
-	clock->cycle_last = 0;
-	clock->cycle_last = clocksource_read(clock);
+	clock->cycle_last = clock->read(clock);
 	clock->error = 0;
 	timekeeping_suspended = 0;
 	write_sequnlock_irqrestore(&xtime_lock, flags);
@@ -630,7 +634,7 @@ void update_wall_time(void)
 		return;
 
 #ifdef CONFIG_GENERIC_TIME
-	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
+	offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
 #else
 	offset = clock->cycle_interval;
 #endif
Index: linux-2.6/include/linux/clocksource.h
===================================================================
--- linux-2.6.orig/include/linux/clocksource.h
+++ linux-2.6/include/linux/clocksource.h
@@ -268,52 +268,6 @@ static inline u32 clocksource_hz2mult(u3
 }
 
 /**
- * clocksource_read: - Access the clocksource's current cycle value
- * @cs:		pointer to clocksource being read
- *
- * Uses the clocksource to return the current cycle_t value
- */
-static inline cycle_t clocksource_read(struct clocksource *cs)
-{
-	return cs->read(cs);
-}
-
-/**
- * clocksource_enable: - enable clocksource
- * @cs:		pointer to clocksource
- *
- * Enables the specified clocksource. The clocksource callback
- * function should start up the hardware and setup mult and field
- * members of struct clocksource to reflect hardware capabilities.
- */
-static inline int clocksource_enable(struct clocksource *cs)
-{
-	int ret = 0;
-
-	if (cs->enable)
-		ret = cs->enable(cs);
-
-	/* save mult_orig on enable */
-	cs->mult_orig = cs->mult;
-
-	return ret;
-}
-
-/**
- * clocksource_disable: - disable clocksource
- * @cs:		pointer to clocksource
- *
- * Disables the specified clocksource. The clocksource callback
- * function should power down the now unused hardware block to
- * save power.
- */
-static inline void clocksource_disable(struct clocksource *cs)
-{
-	if (cs->disable)
-		cs->disable(cs);
-}
-
-/**
  * cyc2ns - converts clocksource cycles to nanoseconds
  * @cs:		Pointer to clocksource
  * @cycles:	Cycles

-- 
blue skies,
   Martin.

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

[RFC][patch 03/12] cleanup clocksource selection

[Martin Schwidefsky]

[-- Attachment #1: clocksource-select.diff --]
[-- Type: text/plain, Size: 10364 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Sorting the list of clocksources according to their rating is not
necessary as the clocksource selection is not performance critical.
Simplify clocksource_register and sysfs_override_clocksource,
remove clocksource_enqueue and let clocksource_select do the job.
The check in clocksource_register for double registration of the
same clocksource is removed without replacement.

To find the initial clocksource a new weak function in jiffies.c is
defined that returns the jiffies clocksource. The architecture code
can then override the weak function with a more suitable clocksource,
e.g. the TOD clock on s390.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/kernel/time.c     |    4 +
 include/linux/clocksource.h |    2 
 kernel/time/clocksource.c   |  148 ++++++++++++++------------------------------
 kernel/time/jiffies.c       |    6 +
 kernel/time/timekeeping.c   |    4 -
 5 files changed, 62 insertions(+), 102 deletions(-)

Index: linux-2.6/kernel/time/clocksource.c
===================================================================
--- linux-2.6.orig/kernel/time/clocksource.c
+++ linux-2.6/kernel/time/clocksource.c
@@ -21,7 +21,6 @@
  *
  * TODO WishList:
  *   o Allow clocksource drivers to be unregistered
- *   o get rid of clocksource_jiffies extern
  */
 
 #include <linux/clocksource.h>
@@ -107,9 +106,6 @@ u64 timecounter_cyc2time(struct timecoun
 }
 EXPORT_SYMBOL(timecounter_cyc2time);
 
-/* XXX - Would like a better way for initializing curr_clocksource */
-extern struct clocksource clocksource_jiffies;
-
 /*[Clocksource internal variables]---------
  * curr_clocksource:
  *	currently selected clocksource. Initialized to clocksource_jiffies.
@@ -123,9 +119,8 @@ extern struct clocksource clocksource_ji
  * override_name:
  *	Name of the user-specified clocksource.
  */
-static struct clocksource *curr_clocksource = &clocksource_jiffies;
+static struct clocksource *curr_clocksource;
 static struct clocksource *next_clocksource;
-static struct clocksource *clocksource_override;
 static LIST_HEAD(clocksource_list);
 static DEFINE_SPINLOCK(clocksource_lock);
 static char override_name[32];
@@ -320,6 +315,7 @@ void clocksource_touch_watchdog(void)
 	clocksource_resume_watchdog();
 }
 
+#ifdef CONFIG_GENERIC_TIME
 /**
  * clocksource_get_next - Returns the selected clocksource
  *
@@ -339,77 +335,69 @@ struct clocksource *clocksource_get_next
 }
 
 /**
- * select_clocksource - Selects the best registered clocksource.
- *
- * Private function. Must hold clocksource_lock when called.
- *
- * Select the clocksource with the best rating, or the clocksource,
- * which is selected by userspace override.
+ * clocksource_select - Select the best clocksource available
  */
-static struct clocksource *select_clocksource(void)
+static int clocksource_select(void)
 {
-	struct clocksource *next;
-
-	if (list_empty(&clocksource_list))
-		return NULL;
-
-	if (clocksource_override)
-		next = clocksource_override;
-	else
-		next = list_entry(clocksource_list.next, struct clocksource,
-				  list);
+	struct clocksource *best, *cs;
+	int rc;
 
-	if (next == curr_clocksource)
-		return NULL;
-
-	return next;
-}
-
-/*
- * Enqueue the clocksource sorted by rating
- */
-static int clocksource_enqueue(struct clocksource *c)
-{
-	struct list_head *tmp, *entry = &clocksource_list;
-
-	list_for_each(tmp, &clocksource_list) {
-		struct clocksource *cs;
-
-		cs = list_entry(tmp, struct clocksource, list);
-		if (cs == c)
-			return -EBUSY;
-		/* Keep track of the place, where to insert */
-		if (cs->rating >= c->rating)
-			entry = tmp;
+	rc = 0;
+	best = NULL;
+	list_for_each_entry(cs, &clocksource_list, list) {
+		/* Check for the override clocksource. */
+		if (strcmp(cs->name, override_name) == 0) {
+			/*
+			 * Check to make sure we don't switch to a non-highres
+			 * capable clocksource if the tick code is in oneshot
+			 * mode (highres or nohz)
+			 */
+			if ((cs->flags & CLOCK_SOURCE_VALID_FOR_HRES) ||
+			    !tick_oneshot_mode_active()) {
+				/* Override clocksource can be used. */
+				best = cs;
+				break;
+			}
+			/* Override clocksource cannot be used. */
+			printk(KERN_WARNING "Override clocksource %s is not "
+			       "HRT compatible. Cannot switch while in "
+			       "HRT/NOHZ mode\n", cs->name);
+			override_name[0] = 0;
+		}
+		/* Check rating */
+		if (!best || cs->rating > best->rating)
+			best = cs;
 	}
-	list_add(&c->list, entry);
+	if (curr_clocksource != best)
+		next_clocksource = best;
+	return rc;
+}
 
-	if (strlen(c->name) == strlen(override_name) &&
-	    !strcmp(c->name, override_name))
-		clocksource_override = c;
+#else /* CONFIG_GENERIC_TIME */
 
+static inline int clocksource_select(void)
+{
 	return 0;
 }
 
+#endif
+
 /**
  * clocksource_register - Used to install new clocksources
  * @t:		clocksource to be registered
  *
  * Returns -EBUSY if registration fails, zero otherwise.
  */
-int clocksource_register(struct clocksource *c)
+int clocksource_register(struct clocksource *cs)
 {
 	unsigned long flags;
-	int ret;
 
 	spin_lock_irqsave(&clocksource_lock, flags);
-	ret = clocksource_enqueue(c);
-	if (!ret)
-		next_clocksource = select_clocksource();
+	list_add(&cs->list, &clocksource_list);
+	clocksource_select();
 	spin_unlock_irqrestore(&clocksource_lock, flags);
-	if (!ret)
-		clocksource_check_watchdog(c);
-	return ret;
+	clocksource_check_watchdog(cs);
+	return 0;
 }
 EXPORT_SYMBOL(clocksource_register);
 
@@ -422,10 +410,8 @@ void clocksource_change_rating(struct cl
 	unsigned long flags;
 
 	spin_lock_irqsave(&clocksource_lock, flags);
-	list_del(&cs->list);
 	cs->rating = rating;
-	clocksource_enqueue(cs);
-	next_clocksource = select_clocksource();
+	clocksource_select();
 	spin_unlock_irqrestore(&clocksource_lock, flags);
 }
 
@@ -438,9 +424,7 @@ void clocksource_unregister(struct clock
 
 	spin_lock_irqsave(&clocksource_lock, flags);
 	list_del(&cs->list);
-	if (clocksource_override == cs)
-		clocksource_override = NULL;
-	next_clocksource = select_clocksource();
+	clocksource_select();
 	spin_unlock_irqrestore(&clocksource_lock, flags);
 }
 
@@ -478,10 +462,6 @@ static ssize_t sysfs_override_clocksourc
 					  struct sysdev_attribute *attr,
 					  const char *buf, size_t count)
 {
-	struct clocksource *ovr = NULL;
-	size_t ret = count;
-	int len;
-
 	/* strings from sysfs write are not 0 terminated! */
 	if (count >= sizeof(override_name))
 		return -EINVAL;
@@ -495,41 +475,11 @@ static ssize_t sysfs_override_clocksourc
 	if (count > 0)
 		memcpy(override_name, buf, count);
 	override_name[count] = 0;
-
-	len = strlen(override_name);
-	if (len) {
-		struct clocksource *cs;
-
-		ovr = clocksource_override;
-		/* try to select it: */
-		list_for_each_entry(cs, &clocksource_list, list) {
-			if (strlen(cs->name) == len &&
-			    !strcmp(cs->name, override_name))
-				ovr = cs;
-		}
-	}
-
-	/*
-	 * Check to make sure we don't switch to a non-highres capable
-	 * clocksource if the tick code is in oneshot mode (highres or nohz)
-	 */
-	if (tick_oneshot_mode_active() && ovr &&
-	    !(ovr->flags & CLOCK_SOURCE_VALID_FOR_HRES)) {
-		printk(KERN_WARNING "%s clocksource is not HRT compatible. "
-			"Cannot switch while in HRT/NOHZ mode\n", ovr->name);
-		ovr = NULL;
-		override_name[0] = 0;
-	}
-
-	/* Reselect, when the override name has changed */
-	if (ovr != clocksource_override) {
-		clocksource_override = ovr;
-		next_clocksource = select_clocksource();
-	}
+	clocksource_select();
 
 	spin_unlock_irq(&clocksource_lock);
 
-	return ret;
+	return count;
 }
 
 /**
Index: linux-2.6/include/linux/clocksource.h
===================================================================
--- linux-2.6.orig/include/linux/clocksource.h
+++ linux-2.6/include/linux/clocksource.h
@@ -14,6 +14,7 @@
 #include <linux/list.h>
 #include <linux/cache.h>
 #include <linux/timer.h>
+#include <linux/init.h>
 #include <asm/div64.h>
 #include <asm/io.h>
 
@@ -322,6 +323,7 @@ extern void clocksource_touch_watchdog(v
 extern struct clocksource* clocksource_get_next(void);
 extern void clocksource_change_rating(struct clocksource *cs, int rating);
 extern void clocksource_resume(void);
+extern struct clocksource * __init __weak clocksource_default_clock(void);
 
 #ifdef CONFIG_GENERIC_TIME_VSYSCALL
 extern void update_vsyscall(struct timespec *ts, struct clocksource *c);
Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -269,7 +269,7 @@ static void change_clocksource(void)
 
 	new = clocksource_get_next();
 
-	if (clock == new)
+	if (!new || clock == new)
 		return;
 
 	clocksource_forward_now();
@@ -436,7 +436,7 @@ void __init timekeeping_init(void)
 
 	ntp_init();
 
-	clock = clocksource_get_next();
+	clock = clocksource_default_clock();
 	if (clock->enable)
 		clock->enable(clock);
 	/* save mult_orig on enable */
Index: linux-2.6/kernel/time/jiffies.c
===================================================================
--- linux-2.6.orig/kernel/time/jiffies.c
+++ linux-2.6/kernel/time/jiffies.c
@@ -61,7 +61,6 @@ struct clocksource clocksource_jiffies =
 	.read		= jiffies_read,
 	.mask		= 0xffffffff, /*32bits*/
 	.mult		= NSEC_PER_JIFFY << JIFFIES_SHIFT, /* details above */
-	.mult_orig	= NSEC_PER_JIFFY << JIFFIES_SHIFT,
 	.shift		= JIFFIES_SHIFT,
 };
 
@@ -71,3 +70,8 @@ static int __init init_jiffies_clocksour
 }
 
 core_initcall(init_jiffies_clocksource);
+
+struct clocksource * __init __weak clocksource_default_clock(void)
+{
+	return &clocksource_jiffies;
+}
Index: linux-2.6/arch/s390/kernel/time.c
===================================================================
--- linux-2.6.orig/arch/s390/kernel/time.c
+++ linux-2.6/arch/s390/kernel/time.c
@@ -205,6 +205,10 @@ static struct clocksource clocksource_to
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
+struct clocksource * __init clocksource_default_clock(void)
+{
+	return &clocksource_tod;
+}
 
 void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
 {

-- 
blue skies,
   Martin.

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

[RFC][patch 04/12] clocksource watchdog highres enablement

[Martin Schwidefsky]

[-- Attachment #1: watchdog-highres.diff --]
[-- Type: text/plain, Size: 3178 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

The clocksource watchdog marks a clock as highres capable before it
checked the deviation from the watchdog clocksource even for a single
time. Make sure that the deviation is at least checked once before
doing the switch to highres mode.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 kernel/time/clocksource.c |   47 +++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

Index: linux-2.6/kernel/time/clocksource.c
===================================================================
--- linux-2.6.orig/kernel/time/clocksource.c
+++ linux-2.6/kernel/time/clocksource.c
@@ -153,11 +153,8 @@ static unsigned long watchdog_resumed;
 #define WATCHDOG_INTERVAL (HZ >> 1)
 #define WATCHDOG_THRESHOLD (NSEC_PER_SEC >> 4)
 
-static void clocksource_ratewd(struct clocksource *cs, int64_t delta)
+static void clocksource_unstable(struct clocksource *cs, int64_t delta)
 {
-	if (delta > -WATCHDOG_THRESHOLD && delta < WATCHDOG_THRESHOLD)
-		return;
-
 	printk(KERN_WARNING "Clocksource %s unstable (delta = %Ld ns)\n",
 	       cs->name, delta);
 	cs->flags &= ~(CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_WATCHDOG);
@@ -183,31 +180,31 @@ static void clocksource_watchdog(unsigne
 	list_for_each_entry_safe(cs, tmp, &watchdog_list, wd_list) {
 		csnow = cs->read(cs);
 
-		if (unlikely(resumed)) {
+		/* Clocksource initialized ? */
+		if (!(cs->flags & CLOCK_SOURCE_WATCHDOG)) {
+			cs->flags |= CLOCK_SOURCE_WATCHDOG;
 			cs->wd_last = csnow;
 			continue;
 		}
 
-		/* Initialized ? */
-		if (!(cs->flags & CLOCK_SOURCE_WATCHDOG)) {
-			if ((cs->flags & CLOCK_SOURCE_IS_CONTINUOUS) &&
-			    (watchdog->flags & CLOCK_SOURCE_IS_CONTINUOUS)) {
-				cs->flags |= CLOCK_SOURCE_VALID_FOR_HRES;
-				/*
-				 * We just marked the clocksource as
-				 * highres-capable, notify the rest of the
-				 * system as well so that we transition
-				 * into high-res mode:
-				 */
-				tick_clock_notify();
-			}
-			cs->flags |= CLOCK_SOURCE_WATCHDOG;
-			cs->wd_last = csnow;
-		} else {
-			cs_nsec = cyc2ns(cs, (csnow - cs->wd_last) & cs->mask);
-			cs->wd_last = csnow;
-			/* Check the delta. Might remove from the list ! */
-			clocksource_ratewd(cs, cs_nsec - wd_nsec);
+		/* Check the deviation from the watchdog clocksource. */
+		cs_nsec = cyc2ns(cs, (csnow - cs->wd_last) & cs->mask);
+		cs->wd_last = csnow;
+		if (abs(cs_nsec - wd_nsec) > WATCHDOG_THRESHOLD) {
+			clocksource_unstable(cs, cs_nsec - wd_nsec);
+			continue;
+		}
+
+		if (!(cs->flags & CLOCK_SOURCE_VALID_FOR_HRES) &&
+		    (cs->flags & CLOCK_SOURCE_IS_CONTINUOUS) &&
+		    (watchdog->flags & CLOCK_SOURCE_IS_CONTINUOUS)) {
+			cs->flags |= CLOCK_SOURCE_VALID_FOR_HRES;
+			/*
+			 * We just marked the clocksource as highres-capable,
+			 * notify the rest of the system as well so that we
+			 * transition into high-res mode:
+			 */
+			tick_clock_notify();
 		}
 	}
 

-- 
blue skies,
   Martin.

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

[RFC][patch 05/12] clocksource watchdog resume logic

[Martin Schwidefsky]

[-- Attachment #1: watchdog-resume.diff --]
[-- Type: text/plain, Size: 2478 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

To resume the clocksource watchdog just remove the CLOCK_SOURCE_WATCHDOG
bit from the watched clocksource.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 kernel/time/clocksource.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Index: linux-2.6/kernel/time/clocksource.c
===================================================================
--- linux-2.6.orig/kernel/time/clocksource.c
+++ linux-2.6/kernel/time/clocksource.c
@@ -145,7 +145,6 @@ static struct clocksource *watchdog;
 static struct timer_list watchdog_timer;
 static DEFINE_SPINLOCK(watchdog_lock);
 static cycle_t watchdog_last;
-static unsigned long watchdog_resumed;
 
 /*
  * Interval: 0.5sec Threshold: 0.0625s
@@ -167,12 +166,9 @@ static void clocksource_watchdog(unsigne
 	struct clocksource *cs, *tmp;
 	cycle_t csnow, wdnow;
 	int64_t wd_nsec, cs_nsec;
-	int resumed;
 
 	spin_lock(&watchdog_lock);
 
-	resumed = test_and_clear_bit(0, &watchdog_resumed);
-
 	wdnow = watchdog->read(watchdog);
 	wd_nsec = cyc2ns(watchdog, (wdnow - watchdog_last) & watchdog->mask);
 	watchdog_last = wdnow;
@@ -223,14 +219,26 @@ static void clocksource_watchdog(unsigne
 	}
 	spin_unlock(&watchdog_lock);
 }
+
+static inline void clocksource_reset_watchdog(void)
+{
+	struct clocksource *cs;
+
+	list_for_each_entry(cs, &watchdog_list, wd_list)
+		cs->flags &= ~CLOCK_SOURCE_WATCHDOG;
+}
+
 static void clocksource_resume_watchdog(void)
 {
-	set_bit(0, &watchdog_resumed);
+	unsigned long flags;
+
+	spin_lock_irqsave(&watchdog_lock, flags);
+	clocksource_reset_watchdog();
+	spin_unlock_irqrestore(&watchdog_lock, flags);
 }
 
 static void clocksource_check_watchdog(struct clocksource *cs)
 {
-	struct clocksource *cse;
 	unsigned long flags;
 
 	spin_lock_irqsave(&watchdog_lock, flags);
@@ -256,8 +264,7 @@ static void clocksource_check_watchdog(s
 			watchdog_timer.function = clocksource_watchdog;
 
 			/* Reset watchdog cycles */
-			list_for_each_entry(cse, &watchdog_list, wd_list)
-				cse->flags &= ~CLOCK_SOURCE_WATCHDOG;
+			clocksource_reset_watchdog();
 			/* Start if list is not empty */
 			if (!list_empty(&watchdog_list)) {
 				watchdog_last = watchdog->read(watchdog);

-- 
blue skies,
   Martin.

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

[RFC][patch 06/12] clocksource watchdog refactoring

[Martin Schwidefsky]

[-- Attachment #1: watchdog-refactor.diff --]
[-- Type: text/plain, Size: 6254 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Refactor clocksource watchdog code to make it more readable. Add
clocksource_dequeue_watchdog to remove a clocksource from the
watckdog list when it is unregistered.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 kernel/time/clocksource.c |   96 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 68 insertions(+), 28 deletions(-)

Index: linux-2.6/kernel/time/clocksource.c
===================================================================
--- linux-2.6.orig/kernel/time/clocksource.c
+++ linux-2.6/kernel/time/clocksource.c
@@ -145,6 +145,7 @@ static struct clocksource *watchdog;
 static struct timer_list watchdog_timer;
 static DEFINE_SPINLOCK(watchdog_lock);
 static cycle_t watchdog_last;
+static int watchdog_running;
 
 /*
  * Interval: 0.5sec Threshold: 0.0625s
@@ -168,6 +169,8 @@ static void clocksource_watchdog(unsigne
 	int64_t wd_nsec, cs_nsec;
 
 	spin_lock(&watchdog_lock);
+	if (!watchdog_running)
+		goto out;
 
 	wdnow = watchdog->read(watchdog);
 	wd_nsec = cyc2ns(watchdog, (wdnow - watchdog_last) & watchdog->mask);
@@ -217,9 +220,30 @@ static void clocksource_watchdog(unsigne
 		watchdog_timer.expires += WATCHDOG_INTERVAL;
 		add_timer_on(&watchdog_timer, next_cpu);
 	}
+out:
 	spin_unlock(&watchdog_lock);
 }
 
+static inline void clocksource_start_watchdog(void)
+{
+	if (watchdog_running || !watchdog || list_empty(&watchdog_list))
+		return;
+	init_timer(&watchdog_timer);
+	watchdog_timer.function = clocksource_watchdog;
+	watchdog_last = watchdog->read(watchdog);
+	watchdog_timer.expires = jiffies + WATCHDOG_INTERVAL;
+	add_timer_on(&watchdog_timer, cpumask_first(cpu_online_mask));
+	watchdog_running = 1;
+}
+
+static inline void clocksource_stop_watchdog(void)
+{
+	if (!watchdog_running || (watchdog && !list_empty(&watchdog_list)))
+		return;
+	del_timer(&watchdog_timer);
+	watchdog_running = 0;
+}
+
 static inline void clocksource_reset_watchdog(void)
 {
 	struct clocksource *cs;
@@ -237,55 +261,69 @@ static void clocksource_resume_watchdog(
 	spin_unlock_irqrestore(&watchdog_lock, flags);
 }
 
-static void clocksource_check_watchdog(struct clocksource *cs)
+static void clocksource_enqueue_watchdog(struct clocksource *cs)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&watchdog_lock, flags);
 	if (cs->flags & CLOCK_SOURCE_MUST_VERIFY) {
-		int started = !list_empty(&watchdog_list);
-
+		/* cs is a clocksource to be watched. */
 		list_add(&cs->wd_list, &watchdog_list);
-		if (!started && watchdog) {
-			watchdog_last = watchdog->read(watchdog);
-			watchdog_timer.expires = jiffies + WATCHDOG_INTERVAL;
-			add_timer_on(&watchdog_timer,
-				     cpumask_first(cpu_online_mask));
-		}
+		cs->flags &= ~CLOCK_SOURCE_WATCHDOG;
 	} else {
+		/* cs is a watchdog. */
 		if (cs->flags & CLOCK_SOURCE_IS_CONTINUOUS)
 			cs->flags |= CLOCK_SOURCE_VALID_FOR_HRES;
-
+		/* Pick the best watchdog. */
 		if (!watchdog || cs->rating > watchdog->rating) {
-			if (watchdog)
-				del_timer(&watchdog_timer);
 			watchdog = cs;
-			init_timer(&watchdog_timer);
-			watchdog_timer.function = clocksource_watchdog;
-
 			/* Reset watchdog cycles */
 			clocksource_reset_watchdog();
-			/* Start if list is not empty */
-			if (!list_empty(&watchdog_list)) {
-				watchdog_last = watchdog->read(watchdog);
-				watchdog_timer.expires =
-					jiffies + WATCHDOG_INTERVAL;
-				add_timer_on(&watchdog_timer,
-					     cpumask_first(cpu_online_mask));
-			}
 		}
 	}
+	/* Check if the watchdog timer needs to be started. */
+	clocksource_start_watchdog();
+	spin_unlock_irqrestore(&watchdog_lock, flags);
+}
+
+static void clocksource_dequeue_watchdog(struct clocksource *cs)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&watchdog_lock, flags);
+	if (cs->flags & CLOCK_SOURCE_MUST_VERIFY) {
+		/* cs is a watched clocksource. */
+		list_del_init(&cs->wd_list);
+	} else if (cs == watchdog) {
+		/* Reset watchdog cycles */
+		clocksource_reset_watchdog();
+		/* Current watchdog is removed. Find an alternative. */
+		watchdog = NULL;
+		list_for_each_entry(cs, &clocksource_list, list) {
+			if (cs->flags & CLOCK_SOURCE_MUST_VERIFY)
+				continue;
+			if (!watchdog || cs->rating > watchdog->rating)
+				watchdog = cs;
+		}
+	}
+	cs->flags &= ~CLOCK_SOURCE_WATCHDOG;
+	/* Check if the watchdog timer needs to be stopped. */
+	clocksource_stop_watchdog();
 	spin_unlock_irqrestore(&watchdog_lock, flags);
 }
-#else
-static void clocksource_check_watchdog(struct clocksource *cs)
+
+#else /* CONFIG_CLOCKSOURCE_WATCHDOG */
+
+static void clocksource_enqueue_watchdog(struct clocksource *cs)
 {
 	if (cs->flags & CLOCK_SOURCE_IS_CONTINUOUS)
 		cs->flags |= CLOCK_SOURCE_VALID_FOR_HRES;
 }
 
+static inline void clocksource_dequeue_watchdog(struct clocksource *cs) { }
 static inline void clocksource_resume_watchdog(void) { }
-#endif
+
+#endif /* CONFIG_CLOCKSOURCE_WATCHDOG */
 
 /**
  * clocksource_resume - resume the clocksource(s)
@@ -400,14 +438,13 @@ int clocksource_register(struct clocksou
 	list_add(&cs->list, &clocksource_list);
 	clocksource_select();
 	spin_unlock_irqrestore(&clocksource_lock, flags);
-	clocksource_check_watchdog(cs);
+	clocksource_enqueue_watchdog(cs);
 	return 0;
 }
 EXPORT_SYMBOL(clocksource_register);
 
 /**
  * clocksource_change_rating - Change the rating of a registered clocksource
- *
  */
 void clocksource_change_rating(struct clocksource *cs, int rating)
 {
@@ -418,6 +455,7 @@ void clocksource_change_rating(struct cl
 	clocksource_select();
 	spin_unlock_irqrestore(&clocksource_lock, flags);
 }
+EXPORT_SYMBOL(clocksource_change_rating);
 
 /**
  * clocksource_unregister - remove a registered clocksource
@@ -426,11 +464,13 @@ void clocksource_unregister(struct clock
 {
 	unsigned long flags;
 
+	clocksource_dequeue_watchdog(cs);
 	spin_lock_irqsave(&clocksource_lock, flags);
 	list_del(&cs->list);
 	clocksource_select();
 	spin_unlock_irqrestore(&clocksource_lock, flags);
 }
+EXPORT_SYMBOL(clocksource_unregister);
 
 #ifdef CONFIG_SYSFS
 /**

-- 
blue skies,
   Martin.

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

[RFC][patch 07/12] clocksource watchdog work

[Martin Schwidefsky]

[-- Attachment #1: watchdog-work.diff --]
[-- Type: text/plain, Size: 4769 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Move the downgrade of an unstable clocksource from the timer interrupt
context into the process context of a work queue thread. This is needed
to be able to do the clocksource switch with stop_machine.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 include/linux/clocksource.h |    1 
 kernel/time/clocksource.c   |   56 ++++++++++++++++++++++++++++++--------------
 2 files changed, 40 insertions(+), 17 deletions(-)

Index: linux-2.6/include/linux/clocksource.h
===================================================================
--- linux-2.6.orig/include/linux/clocksource.h
+++ linux-2.6/include/linux/clocksource.h
@@ -213,6 +213,7 @@ extern struct clocksource *clock;	/* cur
 
 #define CLOCK_SOURCE_WATCHDOG			0x10
 #define CLOCK_SOURCE_VALID_FOR_HRES		0x20
+#define CLOCK_SOURCE_UNSTABLE			0x40
 
 /* simplify initialization of mask field */
 #define CLOCKSOURCE_MASK(bits) (cycle_t)((bits) < 64 ? ((1ULL<<(bits))-1) : -1)
Index: linux-2.6/kernel/time/clocksource.c
===================================================================
--- linux-2.6.orig/kernel/time/clocksource.c
+++ linux-2.6/kernel/time/clocksource.c
@@ -143,10 +143,13 @@ fs_initcall(clocksource_done_booting);
 static LIST_HEAD(watchdog_list);
 static struct clocksource *watchdog;
 static struct timer_list watchdog_timer;
+static struct work_struct watchdog_work;
 static DEFINE_SPINLOCK(watchdog_lock);
 static cycle_t watchdog_last;
 static int watchdog_running;
 
+static void clocksource_watchdog_work(struct work_struct *work);
+
 /*
  * Interval: 0.5sec Threshold: 0.0625s
  */
@@ -158,15 +161,16 @@ static void clocksource_unstable(struct 
 	printk(KERN_WARNING "Clocksource %s unstable (delta = %Ld ns)\n",
 	       cs->name, delta);
 	cs->flags &= ~(CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_WATCHDOG);
-	clocksource_change_rating(cs, 0);
-	list_del(&cs->wd_list);
+	cs->flags |= CLOCK_SOURCE_UNSTABLE;
+	schedule_work(&watchdog_work);
 }
 
 static void clocksource_watchdog(unsigned long data)
 {
-	struct clocksource *cs, *tmp;
+	struct clocksource *cs;
 	cycle_t csnow, wdnow;
 	int64_t wd_nsec, cs_nsec;
+	int next_cpu;
 
 	spin_lock(&watchdog_lock);
 	if (!watchdog_running)
@@ -176,7 +180,12 @@ static void clocksource_watchdog(unsigne
 	wd_nsec = cyc2ns(watchdog, (wdnow - watchdog_last) & watchdog->mask);
 	watchdog_last = wdnow;
 
-	list_for_each_entry_safe(cs, tmp, &watchdog_list, wd_list) {
+	list_for_each_entry(cs, &watchdog_list, wd_list) {
+
+		/* Clocksource already marked unstable? */
+		if (cs->flags & CLOCK_SOURCE_UNSTABLE)
+			continue;
+
 		csnow = cs->read(cs);
 
 		/* Clocksource initialized ? */
@@ -207,19 +216,15 @@ static void clocksource_watchdog(unsigne
 		}
 	}
 
-	if (!list_empty(&watchdog_list)) {
-		/*
-		 * Cycle through CPUs to check if the CPUs stay
-		 * synchronized to each other.
-		 */
-		int next_cpu = cpumask_next(raw_smp_processor_id(),
-					    cpu_online_mask);
-
-		if (next_cpu >= nr_cpu_ids)
-			next_cpu = cpumask_first(cpu_online_mask);
-		watchdog_timer.expires += WATCHDOG_INTERVAL;
-		add_timer_on(&watchdog_timer, next_cpu);
-	}
+	/*
+	 * Cycle through CPUs to check if the CPUs stay synchronized
+	 * to each other.
+	 */
+	next_cpu = cpumask_next(raw_smp_processor_id(), cpu_online_mask);
+	if (next_cpu >= nr_cpu_ids)
+		next_cpu = cpumask_first(cpu_online_mask);
+	watchdog_timer.expires += WATCHDOG_INTERVAL;
+	add_timer_on(&watchdog_timer, next_cpu);
 out:
 	spin_unlock(&watchdog_lock);
 }
@@ -228,6 +233,7 @@ static inline void clocksource_start_wat
 {
 	if (watchdog_running || !watchdog || list_empty(&watchdog_list))
 		return;
+	INIT_WORK(&watchdog_work, clocksource_watchdog_work);
 	init_timer(&watchdog_timer);
 	watchdog_timer.function = clocksource_watchdog;
 	watchdog_last = watchdog->read(watchdog);
@@ -312,6 +318,22 @@ static void clocksource_dequeue_watchdog
 	spin_unlock_irqrestore(&watchdog_lock, flags);
 }
 
+static void clocksource_watchdog_work(struct work_struct *work)
+{
+	struct clocksource *cs, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&watchdog_lock, flags);
+	list_for_each_entry_safe(cs, tmp, &watchdog_list, wd_list)
+		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
+			list_del_init(&cs->wd_list);
+			clocksource_change_rating(cs, 0);
+		}
+	/* Check if the watchdog timer needs to be stopped. */
+	clocksource_stop_watchdog();
+	spin_unlock(&watchdog_lock);
+}
+
 #else /* CONFIG_CLOCKSOURCE_WATCHDOG */
 
 static void clocksource_enqueue_watchdog(struct clocksource *cs)

-- 
blue skies,
   Martin.

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

[RFC][patch 08/12] introduce struct timekeeper

[Martin Schwidefsky]

[-- Attachment #1: timekeeper-struct.diff --]
[-- Type: text/plain, Size: 22103 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Add struct timekeeper to keep the internal values timekeeping.c needs
in regard to the currently selected clock source. This moves the
timekeeping intervals, xtime_nsec and the ntp error value from
struct clocksource to struct timekeeper. The raw_time is removed from
the clocksource as well. It gets treated like xtime as a global variable.
Eventually xtime raw_time should be moved to struct timekeeper.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/kernel/time.c     |    1 
 include/linux/clocksource.h |   54 ----------
 kernel/time/clocksource.c   |    6 -
 kernel/time/timekeeping.c   |  223 ++++++++++++++++++++++++++++----------------
 4 files changed, 152 insertions(+), 132 deletions(-)

Index: linux-2.6/arch/s390/kernel/time.c
===================================================================
--- linux-2.6.orig/arch/s390/kernel/time.c
+++ linux-2.6/arch/s390/kernel/time.c
@@ -280,7 +280,6 @@ void __init time_init(void)
 	now = get_clock();
 	tod_to_timeval(now - TOD_UNIX_EPOCH, &xtime);
 	clocksource_tod.cycle_last = now;
-	clocksource_tod.raw_time = xtime;
 	tod_to_timeval(sched_clock_base_cc - TOD_UNIX_EPOCH, &ts);
 	set_normalized_timespec(&wall_to_monotonic, -ts.tv_sec, -ts.tv_nsec);
 	write_sequnlock_irqrestore(&xtime_lock, flags);
Index: linux-2.6/include/linux/clocksource.h
===================================================================
--- linux-2.6.orig/include/linux/clocksource.h
+++ linux-2.6/include/linux/clocksource.h
@@ -155,8 +155,6 @@ extern u64 timecounter_cyc2time(struct t
  * @flags:		flags describing special properties
  * @vread:		vsyscall based read
  * @resume:		resume function for the clocksource, if necessary
- * @cycle_interval:	Used internally by timekeeping core, please ignore.
- * @xtime_interval:	Used internally by timekeeping core, please ignore.
  */
 struct clocksource {
 	/*
@@ -182,19 +180,12 @@ struct clocksource {
 #define CLKSRC_FSYS_MMIO_SET(mmio, addr)      do { } while (0)
 #endif
 
-	/* timekeeping specific data, ignore */
-	cycle_t cycle_interval;
-	u64	xtime_interval;
-	u32	raw_interval;
 	/*
 	 * 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;
-	u64 xtime_nsec;
-	s64 error;
-	struct timespec raw_time;
 
 #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
 	/* Watchdog related data, used by the framework */
@@ -203,8 +194,6 @@ struct clocksource {
 #endif
 };
 
-extern struct clocksource *clock;	/* current clocksource */
-
 /*
  * Clock source flags bits::
  */
@@ -270,50 +259,15 @@ static inline u32 clocksource_hz2mult(u3
 }
 
 /**
- * cyc2ns - converts clocksource cycles to nanoseconds
- * @cs:		Pointer to clocksource
- * @cycles:	Cycles
+ * clocksource_cyc2ns - converts clocksource cycles to nanoseconds
  *
- * Uses the clocksource and ntp ajdustment to convert cycle_ts to nanoseconds.
+ * Converts cycles to nanoseconds, using the given mult and shift.
  *
  * XXX - This could use some mult_lxl_ll() asm optimization
  */
-static inline s64 cyc2ns(struct clocksource *cs, cycle_t cycles)
-{
-	u64 ret = (u64)cycles;
-	ret = (ret * cs->mult) >> cs->shift;
-	return ret;
-}
-
-/**
- * clocksource_calculate_interval - Calculates a clocksource interval struct
- *
- * @c:		Pointer to clocksource.
- * @length_nsec: Desired interval length in nanoseconds.
- *
- * Calculates a fixed cycle/nsec interval for a given clocksource/adjustment
- * pair and interval request.
- *
- * Unless you're the timekeeping code, you should not be using this!
- */
-static inline void clocksource_calculate_interval(struct clocksource *c,
-					  	  unsigned long length_nsec)
+static inline s64 clocksource_cyc2ns(cycle_t cycles, u32 mult, u32 shift)
 {
-	u64 tmp;
-
-	/* Do the ns -> cycle conversion first, using original mult */
-	tmp = length_nsec;
-	tmp <<= c->shift;
-	tmp += c->mult_orig/2;
-	do_div(tmp, c->mult_orig);
-
-	c->cycle_interval = (cycle_t)tmp;
-	if (c->cycle_interval == 0)
-		c->cycle_interval = 1;
-
-	/* Go back from cycles -> shifted ns, this time use ntp adjused mult */
-	c->xtime_interval = (u64)c->cycle_interval * c->mult;
-	c->raw_interval = ((u64)c->cycle_interval * c->mult_orig) >> c->shift;
+	return ((u64) cycles * mult) >> shift;
 }
 
 
Index: linux-2.6/kernel/time/clocksource.c
===================================================================
--- linux-2.6.orig/kernel/time/clocksource.c
+++ linux-2.6/kernel/time/clocksource.c
@@ -177,7 +177,8 @@ static void clocksource_watchdog(unsigne
 		goto out;
 
 	wdnow = watchdog->read(watchdog);
-	wd_nsec = cyc2ns(watchdog, (wdnow - watchdog_last) & watchdog->mask);
+	wd_nsec = clocksource_cyc2ns((wdnow - watchdog_last) & watchdog->mask,
+				     watchdog->mult, watchdog->shift);
 	watchdog_last = wdnow;
 
 	list_for_each_entry(cs, &watchdog_list, wd_list) {
@@ -196,7 +197,8 @@ static void clocksource_watchdog(unsigne
 		}
 
 		/* Check the deviation from the watchdog clocksource. */
-		cs_nsec = cyc2ns(cs, (csnow - cs->wd_last) & cs->mask);
+		cs_nsec = clocksource_cyc2ns((csnow - cs->wd_last) &
+					     cs->mask, cs->mult, cs->shift);
 		cs->wd_last = csnow;
 		if (abs(cs_nsec - wd_nsec) > WATCHDOG_THRESHOLD) {
 			clocksource_unstable(cs, cs_nsec - wd_nsec);
Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -19,6 +19,56 @@
 #include <linux/time.h>
 #include <linux/tick.h>
 
+/* Structure holding internal timekeeping values. */
+struct timekeeper {
+	struct clocksource *clock;
+	cycle_t cycle_interval;
+	u64	xtime_interval;
+	u32	raw_interval;
+	u64	xtime_nsec;
+	s64	ntp_error;
+};
+
+struct timekeeper timekeeper;
+
+/**
+ * timekeeper_setup_internals - Set up internals to use clocksource clock.
+ *
+ * @clock:		Pointer to clocksource.
+ *
+ * Calculates a fixed cycle/nsec interval for a given clocksource/adjustment
+ * pair and interval request.
+ *
+ * Unless you're the timekeeping code, you should not be using this!
+ */
+static void timekeeper_setup_internals(struct clocksource *clock)
+{
+	cycle_t interval;
+	u64 tmp;
+
+	timekeeper.clock = clock;
+	clock->cycle_last = clock->read(clock);
+
+	/* 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);
+	if (tmp == 0)
+		tmp = 1;
+
+	interval = (cycle_t) tmp;
+	timekeeper.cycle_interval = interval;
+
+	/* Go back from cycles -> shifted ns */
+	timekeeper.xtime_interval = (u64) interval * clock->mult;
+	timekeeper.raw_interval =
+		((u64) interval * clock->mult_orig) >> clock->shift;
+
+	timekeeper.xtime_nsec = 0;
+
+	timekeeper.ntp_error = 0;
+}
 
 /*
  * This read-write spinlock protects us from races in SMP while
@@ -46,6 +96,11 @@ struct timespec xtime __attribute__ ((al
 struct timespec wall_to_monotonic __attribute__ ((aligned (16)));
 static unsigned long total_sleep_time;		/* seconds */
 
+/*
+ * The raw monotonic time for the CLOCK_MONOTONIC_RAW posix clock.
+ */
+struct timespec raw_time;
+
 /* flag for if timekeeping is suspended */
 int __read_mostly timekeeping_suspended;
 
@@ -56,42 +111,42 @@ void update_xtime_cache(u64 nsec)
 	timespec_add_ns(&xtime_cache, nsec);
 }
 
-struct clocksource *clock;
-
 /* must hold xtime_lock */
 void timekeeping_leap_insert(int leapsecond)
 {
 	xtime.tv_sec += leapsecond;
 	wall_to_monotonic.tv_sec -= leapsecond;
-	update_vsyscall(&xtime, clock);
+	update_vsyscall(&xtime, timekeeper.clock);
 }
 
 #ifdef CONFIG_GENERIC_TIME
 /**
- * clocksource_forward_now - update clock to the current time
+ * timekeeping_forward_now - update clock to the current time
  *
  * Forward the current clock to update its state since the last call to
  * update_wall_time(). This is useful before significant clock changes,
  * as it avoids having to deal with this time offset explicitly.
  */
-static void clocksource_forward_now(void)
+static void timekeeping_forward_now(void)
 {
 	cycle_t cycle_now, cycle_delta;
+	struct clocksource *clock;
 	s64 nsec;
 
+	clock = timekeeper.clock;
 	cycle_now = clock->read(clock);
 	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 	clock->cycle_last = cycle_now;
 
-	nsec = cyc2ns(clock, cycle_delta);
+	nsec = clocksource_cyc2ns(cycle_delta, clock->mult, clock->shift);
 
 	/* If arch requires, add in gettimeoffset() */
 	nsec += arch_gettimeoffset();
 
 	timespec_add_ns(&xtime, nsec);
 
-	nsec = ((s64)cycle_delta * clock->mult_orig) >> clock->shift;
-	clock->raw_time.tv_nsec += nsec;
+	nsec = clocksource_cyc2ns(cycle_delta, clock->mult_orig, clock->shift);
+	timespec_add_ns(&raw_time, nsec);
 }
 
 /**
@@ -103,6 +158,7 @@ static void clocksource_forward_now(void
 void getnstimeofday(struct timespec *ts)
 {
 	cycle_t cycle_now, cycle_delta;
+	struct clocksource *clock;
 	unsigned long seq;
 	s64 nsecs;
 
@@ -114,13 +170,15 @@ void getnstimeofday(struct timespec *ts)
 		*ts = xtime;
 
 		/* read clocksource: */
+		clock = timekeeper.clock;
 		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 
 		/* convert to nanoseconds: */
-		nsecs = cyc2ns(clock, cycle_delta);
+		nsecs = clocksource_cyc2ns(cycle_delta, clock->mult,
+					   clock->shift);
 
 		/* If arch requires, add in gettimeoffset() */
 		nsecs += arch_gettimeoffset();
@@ -135,6 +193,7 @@ EXPORT_SYMBOL(getnstimeofday);
 ktime_t ktime_get(void)
 {
 	cycle_t cycle_now, cycle_delta;
+	struct clocksource *clock;
 	unsigned int seq;
 	s64 secs, nsecs;
 
@@ -146,13 +205,15 @@ ktime_t ktime_get(void)
 		nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
 
 		/* read clocksource: */
+		clock = timekeeper.clock;
 		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 
 		/* convert to nanoseconds: */
-		nsecs += cyc2ns(clock, cycle_delta);
+		nsecs += clocksource_cyc2ns(cycle_delta, clock->mult,
+					    clock->shift);
 
 	} while (read_seqretry(&xtime_lock, seq));
 	/*
@@ -174,6 +235,7 @@ EXPORT_SYMBOL_GPL(ktime_get);
 void ktime_get_ts(struct timespec *ts)
 {
 	cycle_t cycle_now, cycle_delta;
+	struct clocksource *clock;
 	struct timespec tomono;
 	unsigned int seq;
 	s64 nsecs;
@@ -186,13 +248,15 @@ void ktime_get_ts(struct timespec *ts)
 		tomono = wall_to_monotonic;
 
 		/* read clocksource: */
+		clock = timekeeper.clock;
 		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 
 		/* convert to nanoseconds: */
-		nsecs = cyc2ns(clock, cycle_delta);
+		nsecs = clocksource_cyc2ns(cycle_delta, clock->mult,
+					   clock->shift);
 
 	} while (read_seqretry(&xtime_lock, seq));
 
@@ -233,7 +297,7 @@ int do_settimeofday(struct timespec *tv)
 
 	write_seqlock_irqsave(&xtime_lock, flags);
 
-	clocksource_forward_now();
+	timekeeping_forward_now();
 
 	ts_delta.tv_sec = tv->tv_sec - xtime.tv_sec;
 	ts_delta.tv_nsec = tv->tv_nsec - xtime.tv_nsec;
@@ -243,10 +307,10 @@ int do_settimeofday(struct timespec *tv)
 
 	update_xtime_cache(0);
 
-	clock->error = 0;
+	timekeeper.ntp_error = 0;
 	ntp_clear();
 
-	update_vsyscall(&xtime, clock);
+	update_vsyscall(&xtime, timekeeper.clock);
 
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 
@@ -269,38 +333,25 @@ static void change_clocksource(void)
 
 	new = clocksource_get_next();
 
-	if (!new || clock == new)
+	if (!new || timekeeper.clock == new)
 		return;
 
-	clocksource_forward_now();
+	timekeeping_forward_now();
 
 	if (new->enable && ! new->enable(new))
 		return;
 	/* save mult_orig on enable */
 	new->mult_orig = new->mult;
 
-	new->raw_time = clock->raw_time;
-	old = clock;
-	clock = new;
+	old = timekeeper.clock;
+	timekeeper_setup_internals(new);
 	if (old->disable)
 		old->disable(old);
 
-	clock->cycle_last = clock->read(clock);
-	clock->error = 0;
-	clock->xtime_nsec = 0;
-	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
-
 	tick_clock_notify();
-
-	/*
-	 * We're holding xtime lock and waking up klogd would deadlock
-	 * us on enqueue.  So no printing!
-	printk(KERN_INFO "Time: %s clocksource has been installed.\n",
-	       clock->name);
-	 */
 }
 #else /* GENERIC_TIME */
-static inline void clocksource_forward_now(void) { }
+static inline void timekeeping_forward_now(void) { }
 static inline void change_clocksource(void) { }
 
 /**
@@ -370,20 +421,23 @@ void getrawmonotonic(struct timespec *ts
 	unsigned long seq;
 	s64 nsecs;
 	cycle_t cycle_now, cycle_delta;
+	struct clocksource *clock;
 
 	do {
 		seq = read_seqbegin(&xtime_lock);
 
 		/* read clocksource: */
+		clock = timekeeper.clock;
 		cycle_now = clock->read(clock);
 
 		/* calculate the delta since the last update_wall_time: */
 		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
 
 		/* convert to nanoseconds: */
-		nsecs = ((s64)cycle_delta * clock->mult_orig) >> clock->shift;
+		nsecs = clocksource_cyc2ns(cycle_delta, clock->mult_orig,
+					   clock->shift);
 
-		*ts = clock->raw_time;
+		*ts = raw_time;
 
 	} while (read_seqretry(&xtime_lock, seq));
 
@@ -403,7 +457,7 @@ int timekeeping_valid_for_hres(void)
 	do {
 		seq = read_seqbegin(&xtime_lock);
 
-		ret = clock->flags & CLOCK_SOURCE_VALID_FOR_HRES;
+		ret = timekeeper.clock->flags & CLOCK_SOURCE_VALID_FOR_HRES;
 
 	} while (read_seqretry(&xtime_lock, seq));
 
@@ -429,6 +483,7 @@ unsigned long __attribute__((weak)) read
  */
 void __init timekeeping_init(void)
 {
+	struct clocksource *clock;
 	unsigned long flags;
 	unsigned long sec = read_persistent_clock();
 
@@ -441,11 +496,13 @@ void __init timekeeping_init(void)
 		clock->enable(clock);
 	/* save mult_orig on enable */
 	clock->mult_orig = clock->mult;
-	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
-	clock->cycle_last = clock->read(clock);
+
+	timekeeper_setup_internals(clock);
 
 	xtime.tv_sec = sec;
 	xtime.tv_nsec = 0;
+	raw_time.tv_sec = 0;
+	raw_time.tv_nsec = 0;
 	set_normalized_timespec(&wall_to_monotonic,
 		-xtime.tv_sec, -xtime.tv_nsec);
 	update_xtime_cache(0);
@@ -482,8 +539,8 @@ static int timekeeping_resume(struct sys
 	}
 	update_xtime_cache(0);
 	/* re-base the last cycle value */
-	clock->cycle_last = clock->read(clock);
-	clock->error = 0;
+	timekeeper.clock->cycle_last = timekeeper.clock->read(timekeeper.clock);
+	timekeeper.ntp_error = 0;
 	timekeeping_suspended = 0;
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 
@@ -504,7 +561,7 @@ static int timekeeping_suspend(struct sy
 	timekeeping_suspend_time = read_persistent_clock();
 
 	write_seqlock_irqsave(&xtime_lock, flags);
-	clocksource_forward_now();
+	timekeeping_forward_now();
 	timekeeping_suspended = 1;
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 
@@ -539,7 +596,7 @@ device_initcall(timekeeping_init_device)
  * If the error is already larger, we look ahead even further
  * to compensate for late or lost adjustments.
  */
-static __always_inline int clocksource_bigadjust(s64 error, s64 *interval,
+static __always_inline int timekeeping_bigadjust(s64 error, s64 *interval,
 						 s64 *offset)
 {
 	s64 tick_error, i;
@@ -555,7 +612,7 @@ static __always_inline int clocksource_b
 	 * here.  This is tuned so that an error of about 1 msec is adjusted
 	 * within about 1 sec (or 2^20 nsec in 2^SHIFT_HZ ticks).
 	 */
-	error2 = clock->error >> (NTP_SCALE_SHIFT + 22 - 2 * SHIFT_HZ);
+	error2 = timekeeper.ntp_error >> (NTP_SCALE_SHIFT + 22 - 2 * SHIFT_HZ);
 	error2 = abs(error2);
 	for (look_ahead = 0; error2 > 0; look_ahead++)
 		error2 >>= 2;
@@ -564,8 +621,9 @@ static __always_inline int clocksource_b
 	 * Now calculate the error in (1 << look_ahead) ticks, but first
 	 * remove the single look ahead already included in the error.
 	 */
-	tick_error = tick_length >> (NTP_SCALE_SHIFT - clock->shift + 1);
-	tick_error -= clock->xtime_interval >> 1;
+	tick_error = tick_length >>
+			(NTP_SCALE_SHIFT - timekeeper.clock->shift + 1);
+	tick_error -= timekeeper.xtime_interval >> 1;
 	error = ((error - tick_error) >> look_ahead) + tick_error;
 
 	/* Finally calculate the adjustment shift value.  */
@@ -590,18 +648,19 @@ static __always_inline int clocksource_b
  * this is optimized for the most common adjustments of -1,0,1,
  * for other values we can do a bit more work.
  */
-static void clocksource_adjust(s64 offset)
+static void timekeeping_adjust(s64 offset)
 {
-	s64 error, interval = clock->cycle_interval;
+	s64 error, interval = timekeeper.cycle_interval;
 	int adj;
 
-	error = clock->error >> (NTP_SCALE_SHIFT - clock->shift - 1);
+	error = timekeeper.ntp_error >>
+		(NTP_SCALE_SHIFT - timekeeper.clock->shift - 1);
 	if (error > interval) {
 		error >>= 2;
 		if (likely(error <= interval))
 			adj = 1;
 		else
-			adj = clocksource_bigadjust(error, &interval, &offset);
+			adj = timekeeping_bigadjust(error, &interval, &offset);
 	} else if (error < -interval) {
 		error >>= 2;
 		if (likely(error >= -interval)) {
@@ -609,15 +668,15 @@ static void clocksource_adjust(s64 offse
 			interval = -interval;
 			offset = -offset;
 		} else
-			adj = clocksource_bigadjust(error, &interval, &offset);
+			adj = timekeeping_bigadjust(error, &interval, &offset);
 	} else
 		return;
 
-	clock->mult += adj;
-	clock->xtime_interval += interval;
-	clock->xtime_nsec -= offset;
-	clock->error -= (interval - offset) <<
-			(NTP_SCALE_SHIFT - clock->shift);
+	timekeeper.clock->mult += adj;
+	timekeeper.xtime_interval += interval;
+	timekeeper.xtime_nsec -= offset;
+	timekeeper.ntp_error -= (interval - offset) <<
+			(NTP_SCALE_SHIFT - timekeeper.clock->shift);
 }
 
 /**
@@ -627,53 +686,57 @@ static void clocksource_adjust(s64 offse
  */
 void update_wall_time(void)
 {
+	struct clocksource *clock;
 	cycle_t offset;
+	s64 nsecs;
 
 	/* Make sure we're fully resumed: */
 	if (unlikely(timekeeping_suspended))
 		return;
 
+	clock = timekeeper.clock;
 #ifdef CONFIG_GENERIC_TIME
 	offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
 #else
-	offset = clock->cycle_interval;
+	offset = timekeeper.cycle_interval;
 #endif
-	clock->xtime_nsec = (s64)xtime.tv_nsec << clock->shift;
+	timekeeper.xtime_nsec = (s64)xtime.tv_nsec << clock->shift;
 
 	/* normally this loop will run just once, however in the
 	 * case of lost or late ticks, it will accumulate correctly.
 	 */
-	while (offset >= clock->cycle_interval) {
+	while (offset >= timekeeper.cycle_interval) {
 		/* accumulate one interval */
-		offset -= clock->cycle_interval;
-		clock->cycle_last += clock->cycle_interval;
+		offset -= timekeeper.cycle_interval;
+		clock->cycle_last += timekeeper.cycle_interval;
 
-		clock->xtime_nsec += clock->xtime_interval;
-		if (clock->xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
-			clock->xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
+		timekeeper.xtime_nsec += timekeeper.xtime_interval;
+		if (timekeeper.xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
+			timekeeper.xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
 			xtime.tv_sec++;
 			second_overflow();
 		}
 
-		clock->raw_time.tv_nsec += clock->raw_interval;
-		if (clock->raw_time.tv_nsec >= NSEC_PER_SEC) {
-			clock->raw_time.tv_nsec -= NSEC_PER_SEC;
-			clock->raw_time.tv_sec++;
+		raw_time.tv_nsec += timekeeper.raw_interval;
+		if (raw_time.tv_nsec >= NSEC_PER_SEC) {
+			raw_time.tv_nsec -= NSEC_PER_SEC;
+			raw_time.tv_sec++;
 		}
 
 		/* accumulate error between NTP and clock interval */
-		clock->error += tick_length;
-		clock->error -= clock->xtime_interval << (NTP_SCALE_SHIFT - clock->shift);
+		timekeeper.ntp_error += tick_length;
+		timekeeper.ntp_error -= timekeeper.xtime_interval <<
+					(NTP_SCALE_SHIFT - clock->shift);
 	}
 
 	/* correct the clock when NTP error is too big */
-	clocksource_adjust(offset);
+	timekeeping_adjust(offset);
 
 	/*
 	 * Since in the loop above, we accumulate any amount of time
 	 * in xtime_nsec over a second into xtime.tv_sec, its possible for
 	 * xtime_nsec to be fairly small after the loop. Further, if we're
-	 * slightly speeding the clocksource up in clocksource_adjust(),
+	 * slightly speeding the clocksource up in timekeeping_adjust(),
 	 * its possible the required corrective factor to xtime_nsec could
 	 * cause it to underflow.
 	 *
@@ -685,24 +748,26 @@ void update_wall_time(void)
 	 * We'll correct this error next time through this function, when
 	 * xtime_nsec is not as small.
 	 */
-	if (unlikely((s64)clock->xtime_nsec < 0)) {
-		s64 neg = -(s64)clock->xtime_nsec;
-		clock->xtime_nsec = 0;
-		clock->error += neg << (NTP_SCALE_SHIFT - clock->shift);
+	if (unlikely((s64)timekeeper.xtime_nsec < 0)) {
+		s64 neg = -(s64)timekeeper.xtime_nsec;
+		timekeeper.xtime_nsec = 0;
+		timekeeper.ntp_error += neg << (NTP_SCALE_SHIFT - clock->shift);
 	}
 
 	/* store full nanoseconds into xtime after rounding it up and
 	 * add the remainder to the error difference.
 	 */
-	xtime.tv_nsec = ((s64)clock->xtime_nsec >> clock->shift) + 1;
-	clock->xtime_nsec -= (s64)xtime.tv_nsec << clock->shift;
-	clock->error += clock->xtime_nsec << (NTP_SCALE_SHIFT - clock->shift);
+	xtime.tv_nsec = ((s64)timekeeper.xtime_nsec >> clock->shift) + 1;
+	timekeeper.xtime_nsec -= (s64)xtime.tv_nsec << clock->shift;
+	timekeeper.ntp_error += timekeeper.xtime_nsec <<
+				(NTP_SCALE_SHIFT - clock->shift);
 
-	update_xtime_cache(cyc2ns(clock, offset));
+	nsecs = clocksource_cyc2ns(offset, clock->mult, clock->shift);
+	update_xtime_cache(nsecs);
 
 	/* check to see if there is a new clocksource to use */
 	change_clocksource();
-	update_vsyscall(&xtime, clock);
+	update_vsyscall(&xtime, timekeeper.clock);
 }
 
 /**

-- 
blue skies,
   Martin.

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

[RFC][patch 09/12] add xtime_shift and ntp_error_shift to struct timekeeper

[Martin Schwidefsky]

[-- Attachment #1: timekeeper-shift.diff --]
[-- Type: text/plain, Size: 4897 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

The xtime_nsec value in the timekeeper structure is shifted by a few
bits to improve precision. This happens to be the same value as the
clock->shift. To improve readability add xtime_shift to the timekeeper
and use it instead of the clock->shift. Likewise add ntp_error_shift
and replace all (NTP_SCALE_SHIFT - clock->shift) expressions.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 kernel/time/timekeeping.c |   33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -27,6 +27,8 @@ struct timekeeper {
 	u32	raw_interval;
 	u64	xtime_nsec;
 	s64	ntp_error;
+	int	xtime_shift;
+	int	ntp_error_shift;
 };
 
 struct timekeeper timekeeper;
@@ -66,8 +68,10 @@ static void timekeeper_setup_internals(s
 		((u64) interval * clock->mult_orig) >> clock->shift;
 
 	timekeeper.xtime_nsec = 0;
+	timekeeper.xtime_shift = clock->shift;
 
 	timekeeper.ntp_error = 0;
+	timekeeper.ntp_error_shift = NTP_SCALE_SHIFT - clock->shift;
 }
 
 /*
@@ -621,8 +625,7 @@ static __always_inline int timekeeping_b
 	 * Now calculate the error in (1 << look_ahead) ticks, but first
 	 * remove the single look ahead already included in the error.
 	 */
-	tick_error = tick_length >>
-			(NTP_SCALE_SHIFT - timekeeper.clock->shift + 1);
+	tick_error = tick_length >> (timekeeper.ntp_error_shift + 1);
 	tick_error -= timekeeper.xtime_interval >> 1;
 	error = ((error - tick_error) >> look_ahead) + tick_error;
 
@@ -653,8 +656,7 @@ static void timekeeping_adjust(s64 offse
 	s64 error, interval = timekeeper.cycle_interval;
 	int adj;
 
-	error = timekeeper.ntp_error >>
-		(NTP_SCALE_SHIFT - timekeeper.clock->shift - 1);
+	error = timekeeper.ntp_error >> (timekeeper.ntp_error_shift - 1);
 	if (error > interval) {
 		error >>= 2;
 		if (likely(error <= interval))
@@ -676,7 +678,7 @@ static void timekeeping_adjust(s64 offse
 	timekeeper.xtime_interval += interval;
 	timekeeper.xtime_nsec -= offset;
 	timekeeper.ntp_error -= (interval - offset) <<
-			(NTP_SCALE_SHIFT - timekeeper.clock->shift);
+ 				timekeeper.ntp_error_shift;
 }
 
 /**
@@ -688,7 +690,7 @@ void update_wall_time(void)
 {
 	struct clocksource *clock;
 	cycle_t offset;
-	s64 nsecs;
+	u64 nsecs;
 
 	/* Make sure we're fully resumed: */
 	if (unlikely(timekeeping_suspended))
@@ -700,7 +702,7 @@ void update_wall_time(void)
 #else
 	offset = timekeeper.cycle_interval;
 #endif
-	timekeeper.xtime_nsec = (s64)xtime.tv_nsec << clock->shift;
+	timekeeper.xtime_nsec = (s64)xtime.tv_nsec << timekeeper.xtime_shift;
 
 	/* normally this loop will run just once, however in the
 	 * case of lost or late ticks, it will accumulate correctly.
@@ -711,8 +713,9 @@ void update_wall_time(void)
 		clock->cycle_last += timekeeper.cycle_interval;
 
 		timekeeper.xtime_nsec += timekeeper.xtime_interval;
-		if (timekeeper.xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
-			timekeeper.xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
+		nsecs = (u64) NSEC_PER_SEC << timekeeper.xtime_shift;
+		if (timekeeper.xtime_nsec >= nsecs) {
+			timekeeper.xtime_nsec -= nsecs;
 			xtime.tv_sec++;
 			second_overflow();
 		}
@@ -726,7 +729,7 @@ void update_wall_time(void)
 		/* accumulate error between NTP and clock interval */
 		timekeeper.ntp_error += tick_length;
 		timekeeper.ntp_error -= timekeeper.xtime_interval <<
-					(NTP_SCALE_SHIFT - clock->shift);
+					timekeeper.ntp_error_shift;
 	}
 
 	/* correct the clock when NTP error is too big */
@@ -751,16 +754,16 @@ void update_wall_time(void)
 	if (unlikely((s64)timekeeper.xtime_nsec < 0)) {
 		s64 neg = -(s64)timekeeper.xtime_nsec;
 		timekeeper.xtime_nsec = 0;
-		timekeeper.ntp_error += neg << (NTP_SCALE_SHIFT - clock->shift);
+		timekeeper.ntp_error += neg << timekeeper.ntp_error_shift;
 	}
 
 	/* store full nanoseconds into xtime after rounding it up and
 	 * add the remainder to the error difference.
 	 */
-	xtime.tv_nsec = ((s64)timekeeper.xtime_nsec >> clock->shift) + 1;
-	timekeeper.xtime_nsec -= (s64)xtime.tv_nsec << clock->shift;
-	timekeeper.ntp_error += timekeeper.xtime_nsec <<
-				(NTP_SCALE_SHIFT - clock->shift);
+	xtime.tv_nsec =	((s64) timekeeper.xtime_nsec >> timekeeper.xtime_shift) + 1;
+	timekeeper.xtime_nsec -= (s64) xtime.tv_nsec << timekeeper.xtime_shift;
+	timekeeper.ntp_error +=	timekeeper.xtime_nsec <<
+				timekeeper.ntp_error_shift;
 
 	nsecs = clocksource_cyc2ns(offset, clock->mult, clock->shift);
 	update_xtime_cache(nsecs);

-- 
blue skies,
   Martin.

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

[RFC][patch 10/12] move NTP adjusted clock multiplier to struct timekeeper

[Martin Schwidefsky]

[-- Attachment #1: timekeeper-mult.diff --]
[-- Type: text/plain, Size: 7157 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

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@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 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.

[RFC][patch 11/12] timekeeper read clock helper functions

[Martin Schwidefsky]

[-- Attachment #1: timekeeper-helper.diff --]
[-- Type: text/plain, Size: 5067 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Add timekeeper_read_clock_ntp and timekeeper_read_clock_raw and use
them for getnstimeofday, ktime_get, ktime_get_ts and getrawmonotonic.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 kernel/time/timekeeping.c |   91 +++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 53 deletions(-)

Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -84,6 +84,40 @@ static void timekeeper_setup_internals(s
 	timekeeper.shift = clock->shift;
 }
 
+/* Timekeeper helper functions. */
+static inline s64 timekeeper_read_clock_ntp(void)
+{
+	cycle_t cycle_now, cycle_delta;
+	struct clocksource *clock;
+
+	/* read clocksource: */
+	clock = timekeeper.clock;
+	cycle_now = clock->read(clock);
+
+	/* calculate the delta since the last update_wall_time: */
+	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
+
+	/* return delta convert to nanoseconds using ntp adjusted mult. */
+	return clocksource_cyc2ns(cycle_delta, timekeeper.mult,
+				  timekeeper.shift);
+}
+
+static inline s64 timekeeper_read_clock_raw(void)
+{
+	cycle_t cycle_now, cycle_delta;
+	struct clocksource *clock;
+
+	/* read clocksource: */
+	clock = timekeeper.clock;
+	cycle_now = clock->read(clock);
+
+	/* calculate the delta since the last update_wall_time: */
+	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
+
+	/* return delta convert to nanoseconds using ntp adjusted mult. */
+	return clocksource_cyc2ns(cycle_delta, clock->mult, clock->shift);
+}
+
 /*
  * This read-write spinlock protects us from races in SMP while
  * playing with xtime.
@@ -172,8 +206,6 @@ static void timekeeping_forward_now(void
  */
 void getnstimeofday(struct timespec *ts)
 {
-	cycle_t cycle_now, cycle_delta;
-	struct clocksource *clock;
 	unsigned long seq;
 	s64 nsecs;
 
@@ -183,17 +215,7 @@ void getnstimeofday(struct timespec *ts)
 		seq = read_seqbegin(&xtime_lock);
 
 		*ts = xtime;
-
-		/* read clocksource: */
-		clock = timekeeper.clock;
-		cycle_now = clock->read(clock);
-
-		/* calculate the delta since the last update_wall_time: */
-		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
-
-		/* convert to nanoseconds: */
-		nsecs = clocksource_cyc2ns(cycle_delta, timekeeper.mult,
-					   timekeeper.shift);
+		nsecs = timekeeper_read_clock_ntp();
 
 		/* If arch requires, add in gettimeoffset() */
 		nsecs += arch_gettimeoffset();
@@ -207,8 +229,6 @@ EXPORT_SYMBOL(getnstimeofday);
 
 ktime_t ktime_get(void)
 {
-	cycle_t cycle_now, cycle_delta;
-	struct clocksource *clock;
 	unsigned int seq;
 	s64 secs, nsecs;
 
@@ -218,17 +238,7 @@ ktime_t ktime_get(void)
 		seq = read_seqbegin(&xtime_lock);
 		secs = xtime.tv_sec + wall_to_monotonic.tv_sec;
 		nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
-
-		/* read clocksource: */
-		clock = timekeeper.clock;
-		cycle_now = clock->read(clock);
-
-		/* calculate the delta since the last update_wall_time: */
-		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
-
-		/* convert to nanoseconds: */
-		nsecs += clocksource_cyc2ns(cycle_delta, timekeeper.mult,
-					    timekeeper.shift);
+		nsecs += timekeeper_read_clock_ntp();
 
 	} while (read_seqretry(&xtime_lock, seq));
 	/*
@@ -249,8 +259,6 @@ EXPORT_SYMBOL_GPL(ktime_get);
  */
 void ktime_get_ts(struct timespec *ts)
 {
-	cycle_t cycle_now, cycle_delta;
-	struct clocksource *clock;
 	struct timespec tomono;
 	unsigned int seq;
 	s64 nsecs;
@@ -261,17 +269,7 @@ void ktime_get_ts(struct timespec *ts)
 		seq = read_seqbegin(&xtime_lock);
 		*ts = xtime;
 		tomono = wall_to_monotonic;
-
-		/* read clocksource: */
-		clock = timekeeper.clock;
-		cycle_now = clock->read(clock);
-
-		/* calculate the delta since the last update_wall_time: */
-		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
-
-		/* convert to nanoseconds: */
-		nsecs = clocksource_cyc2ns(cycle_delta, timekeeper.mult,
-					   timekeeper.shift);
+		nsecs = timekeeper_read_clock_ntp();
 
 	} while (read_seqretry(&xtime_lock, seq));
 
@@ -433,23 +431,10 @@ void getrawmonotonic(struct timespec *ts
 {
 	unsigned long seq;
 	s64 nsecs;
-	cycle_t cycle_now, cycle_delta;
-	struct clocksource *clock;
 
 	do {
 		seq = read_seqbegin(&xtime_lock);
-
-		/* read clocksource: */
-		clock = timekeeper.clock;
-		cycle_now = clock->read(clock);
-
-		/* calculate the delta since the last update_wall_time: */
-		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
-
-		/* convert to nanoseconds: */
-		nsecs = clocksource_cyc2ns(cycle_delta, clock->mult,
-					   clock->shift);
-
+		nsecs = timekeeper_read_clock_raw();
 		*ts = raw_time;
 
 	} while (read_seqretry(&xtime_lock, seq));

-- 
blue skies,
   Martin.

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

[RFC][patch 12/12] update clocksource with stop_machine

[Martin Schwidefsky]

[-- Attachment #1: timekeeper-stop-machine.diff --]
[-- Type: text/plain, Size: 10385 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

update_wall_time calls change_clocksource HZ times per second to check
if a new clock source is available. In close to 100% of all calls there
is no new clock. Replace the tick based check by an update done with
stop_machine.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: john stultz <johnstul@us.ibm.com>
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 include/linux/clocksource.h |    2 
 kernel/time/clocksource.c   |  117 ++++++++++++++++----------------------------
 kernel/time/timekeeping.c   |   40 +++++++++------
 3 files changed, 73 insertions(+), 86 deletions(-)

Index: linux-2.6/include/linux/clocksource.h
===================================================================
--- linux-2.6.orig/include/linux/clocksource.h
+++ linux-2.6/include/linux/clocksource.h
@@ -291,4 +291,6 @@ static inline void update_vsyscall_tz(vo
 }
 #endif
 
+extern void timekeeping_notify(struct clocksource *clock);
+
 #endif /* _LINUX_CLOCKSOURCE_H */
Index: linux-2.6/kernel/time/clocksource.c
===================================================================
--- linux-2.6.orig/kernel/time/clocksource.c
+++ linux-2.6/kernel/time/clocksource.c
@@ -109,35 +109,17 @@ EXPORT_SYMBOL(timecounter_cyc2time);
 /*[Clocksource internal variables]---------
  * curr_clocksource:
  *	currently selected clocksource. Initialized to clocksource_jiffies.
- * next_clocksource:
- *	pending next selected clocksource.
  * clocksource_list:
  *	linked list with the registered clocksources
- * clocksource_lock:
- *	protects manipulations to curr_clocksource and next_clocksource
- *	and the clocksource_list
+ * clocksource_mutex:
+ *	protects manipulations to curr_clocksource and the clocksource_list
  * override_name:
  *	Name of the user-specified clocksource.
  */
 static struct clocksource *curr_clocksource;
-static struct clocksource *next_clocksource;
 static LIST_HEAD(clocksource_list);
-static DEFINE_SPINLOCK(clocksource_lock);
+static DEFINE_MUTEX(clocksource_mutex);
 static char override_name[32];
-static int finished_booting;
-
-/* clocksource_done_booting - Called near the end of core bootup
- *
- * Hack to avoid lots of clocksource churn at boot time.
- * We use fs_initcall because we want this to start before
- * device_initcall but after subsys_initcall.
- */
-static int __init clocksource_done_booting(void)
-{
-	finished_booting = 1;
-	return 0;
-}
-fs_initcall(clocksource_done_booting);
 
 #ifdef CONFIG_CLOCKSOURCE_WATCHDOG
 static LIST_HEAD(watchdog_list);
@@ -355,18 +337,16 @@ static inline void clocksource_resume_wa
 void clocksource_resume(void)
 {
 	struct clocksource *cs;
-	unsigned long flags;
 
-	spin_lock_irqsave(&clocksource_lock, flags);
+	mutex_lock(&clocksource_mutex);
 
-	list_for_each_entry(cs, &clocksource_list, list) {
+	list_for_each_entry(cs, &clocksource_list, list)
 		if (cs->resume)
 			cs->resume();
-	}
 
 	clocksource_resume_watchdog();
 
-	spin_unlock_irqrestore(&clocksource_lock, flags);
+	mutex_unlock(&clocksource_mutex);
 }
 
 /**
@@ -382,33 +362,19 @@ void clocksource_touch_watchdog(void)
 }
 
 #ifdef CONFIG_GENERIC_TIME
-/**
- * clocksource_get_next - Returns the selected clocksource
- *
- */
-struct clocksource *clocksource_get_next(void)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocksource_lock, flags);
-	if (next_clocksource && finished_booting) {
-		curr_clocksource = next_clocksource;
-		next_clocksource = NULL;
-	}
-	spin_unlock_irqrestore(&clocksource_lock, flags);
 
-	return curr_clocksource;
-}
+static int finished_booting;
 
 /**
  * clocksource_select - Select the best clocksource available
  */
-static int clocksource_select(void)
+static void clocksource_select(void)
 {
 	struct clocksource *best, *cs;
-	int rc;
 
-	rc = 0;
+	if (!finished_booting)
+		return;
+
 	best = NULL;
 	list_for_each_entry(cs, &clocksource_list, list) {
 		/* Check for the override clocksource. */
@@ -434,17 +400,31 @@ static int clocksource_select(void)
 		if (!best || cs->rating > best->rating)
 			best = cs;
 	}
-	if (curr_clocksource != best)
-		next_clocksource = best;
-	return rc;
+	if (curr_clocksource != best) {
+		printk(KERN_INFO "Switching to clocksource %s\n", best->name);
+		curr_clocksource = best;
+		timekeeping_notify(curr_clocksource);
+	}
 }
 
-#else /* CONFIG_GENERIC_TIME */
-
-static inline int clocksource_select(void)
+/*
+ * clocksource_done_booting - Called near the end of core bootup
+ *
+ * Hack to avoid lots of clocksource churn at boot time.
+ * We use fs_initcall because we want this to start before
+ * device_initcall but after subsys_initcall.
+ */
+static int __init clocksource_done_booting(void)
 {
+	finished_booting = 1;
+	clocksource_select();
 	return 0;
 }
+fs_initcall(clocksource_done_booting);
+
+#else /* CONFIG_GENERIC_TIME */
+
+static inline void clocksource_select(void) { }
 
 #endif
 
@@ -456,13 +436,11 @@ static inline int clocksource_select(voi
  */
 int clocksource_register(struct clocksource *cs)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocksource_lock, flags);
+	mutex_lock(&clocksource_mutex);
 	list_add(&cs->list, &clocksource_list);
 	clocksource_select();
-	spin_unlock_irqrestore(&clocksource_lock, flags);
 	clocksource_enqueue_watchdog(cs);
+	mutex_unlock(&clocksource_mutex);
 	return 0;
 }
 EXPORT_SYMBOL(clocksource_register);
@@ -472,12 +450,10 @@ EXPORT_SYMBOL(clocksource_register);
  */
 void clocksource_change_rating(struct clocksource *cs, int rating)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocksource_lock, flags);
+	mutex_lock(&clocksource_mutex);
 	cs->rating = rating;
 	clocksource_select();
-	spin_unlock_irqrestore(&clocksource_lock, flags);
+	mutex_unlock(&clocksource_mutex);
 }
 EXPORT_SYMBOL(clocksource_change_rating);
 
@@ -486,13 +462,11 @@ EXPORT_SYMBOL(clocksource_change_rating)
  */
 void clocksource_unregister(struct clocksource *cs)
 {
-	unsigned long flags;
-
+	mutex_lock(&clocksource_mutex);
 	clocksource_dequeue_watchdog(cs);
-	spin_lock_irqsave(&clocksource_lock, flags);
 	list_del(&cs->list);
 	clocksource_select();
-	spin_unlock_irqrestore(&clocksource_lock, flags);
+	mutex_unlock(&clocksource_mutex);
 }
 EXPORT_SYMBOL(clocksource_unregister);
 
@@ -510,9 +484,9 @@ sysfs_show_current_clocksources(struct s
 {
 	ssize_t count = 0;
 
-	spin_lock_irq(&clocksource_lock);
+	mutex_lock(&clocksource_mutex);
 	count = snprintf(buf, PAGE_SIZE, "%s\n", curr_clocksource->name);
-	spin_unlock_irq(&clocksource_lock);
+	mutex_unlock(&clocksource_mutex);
 
 	return count;
 }
@@ -538,14 +512,14 @@ static ssize_t sysfs_override_clocksourc
 	if (buf[count-1] == '\n')
 		count--;
 
-	spin_lock_irq(&clocksource_lock);
+	mutex_lock(&clocksource_mutex);
 
 	if (count > 0)
 		memcpy(override_name, buf, count);
 	override_name[count] = 0;
 	clocksource_select();
 
-	spin_unlock_irq(&clocksource_lock);
+	mutex_unlock(&clocksource_mutex);
 
 	return count;
 }
@@ -565,7 +539,7 @@ sysfs_show_available_clocksources(struct
 	struct clocksource *src;
 	ssize_t count = 0;
 
-	spin_lock_irq(&clocksource_lock);
+	mutex_lock(&clocksource_mutex);
 	list_for_each_entry(src, &clocksource_list, list) {
 		/*
 		 * Don't show non-HRES clocksource if the tick code is
@@ -577,7 +551,7 @@ sysfs_show_available_clocksources(struct
 				  max((ssize_t)PAGE_SIZE - count, (ssize_t)0),
 				  "%s ", src->name);
 	}
-	spin_unlock_irq(&clocksource_lock);
+	mutex_unlock(&clocksource_mutex);
 
 	count += snprintf(buf + count,
 			  max((ssize_t)PAGE_SIZE - count, (ssize_t)0), "\n");
@@ -632,11 +606,10 @@ device_initcall(init_clocksource_sysfs);
  */
 static int __init boot_override_clocksource(char* str)
 {
-	unsigned long flags;
-	spin_lock_irqsave(&clocksource_lock, flags);
+	mutex_lock(&clocksource_mutex);
 	if (str)
 		strlcpy(override_name, str, sizeof(override_name));
-	spin_unlock_irqrestore(&clocksource_lock, flags);
+	mutex_unlock(&clocksource_mutex);
 	return 1;
 }
 
Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -18,6 +18,7 @@
 #include <linux/jiffies.h>
 #include <linux/time.h>
 #include <linux/tick.h>
+#include <linux/stop_machine.h>
 
 /* Structure holding internal timekeeping values. */
 struct timekeeper {
@@ -168,6 +169,7 @@ void timekeeping_leap_insert(int leapsec
 }
 
 #ifdef CONFIG_GENERIC_TIME
+
 /**
  * timekeeping_forward_now - update clock to the current time
  *
@@ -340,30 +342,40 @@ EXPORT_SYMBOL(do_settimeofday);
  *
  * Accumulates current time interval and initializes new clocksource
  */
-static void change_clocksource(void)
+static int change_clocksource(void *data)
 {
 	struct clocksource *new, *old;
 
-	new = clocksource_get_next();
-
-	if (!new || timekeeper.clock == new)
-		return;
+	new = (struct clocksource *) data;
 
 	timekeeping_forward_now();
+	if (!new->enable || new->enable(new) == 0) {
+		old = timekeeper.clock;
+		timekeeper_setup_internals(new);
+		if (old->disable)
+			old->disable(old);
+	}
+	return 0;
+}
 
-	if (new->enable && ! new->enable(new))
+/**
+ * timekeeping_notify - Install a new clock source
+ * @clock:		pointer to the clock source
+ *
+ * This function is called from clocksource.c after a new, better clock
+ * source has been registered. The caller holds the clocksource_mutex.
+ */
+void timekeeping_notify(struct clocksource *clock)
+{
+	if (timekeeper.clock == clock)
 		return;
-
-	old = timekeeper.clock;
-	timekeeper_setup_internals(new);
-	if (old->disable)
-		old->disable(old);
-
+	stop_machine(change_clocksource, clock, NULL);
 	tick_clock_notify();
 }
+
 #else /* GENERIC_TIME */
+
 static inline void timekeeping_forward_now(void) { }
-static inline void change_clocksource(void) { }
 
 /**
  * ktime_get - get the monotonic time in ktime_t format
@@ -404,6 +416,7 @@ void ktime_get_ts(struct timespec *ts)
 				ts->tv_nsec + tomono.tv_nsec);
 }
 EXPORT_SYMBOL_GPL(ktime_get_ts);
+
 #endif /* !GENERIC_TIME */
 
 /**
@@ -761,7 +774,6 @@ void update_wall_time(void)
 	update_xtime_cache(nsecs);
 
 	/* check to see if there is a new clocksource to use */
-	change_clocksource();
 	update_vsyscall(&xtime, timekeeper.clock);
 }
 

-- 
blue skies,
   Martin.

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

Re: [RFC][patch 02/12] remove clocksource inline functions

[Daniel Walker]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (clocksource-inline.diff)
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Remove clocksource_read, clocksource_enable and clocksource_disable
> inline functions. No functional change.
> 

Your still not really explaining this one, is this suppose to be
cleaner? Or is this related to some other part of your clean up?

Daniel

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> There is still more room for improvement. Some sore points are:
> 
> 1) The cycle_last value still is in the struct clocksource. It should
>    be in the struct timekeeper but the check against cycles_last in
> the
>    read function of the TSC clock source makes it hard.
> 2) read_persistent_clock returns seconds. With a really good initial
>    time source this is not very precise. read_persistent_clock should
>    return a struct timespec.
> 3) xtime, raw_time, total_sleep_time, timekeeping_suspended, jiffies,
>    the ntp state and probably a few other values may be better located
>    in the struct timekeeper as well.


You could also consolidate the clocksource_unregister() path and the
clocksource_change_rating(0) path  , both are basically doing the same
thing.. Neither one is heavily used..

Daniel

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Martin Schwidefsky]

On Wed, 29 Jul 2009 09:10:31 -0600
dwalker@fifo99.com wrote:

> On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > There is still more room for improvement. Some sore points are:
> > 
> > 1) The cycle_last value still is in the struct clocksource. It should
> >    be in the struct timekeeper but the check against cycles_last in
> > the
> >    read function of the TSC clock source makes it hard.
> > 2) read_persistent_clock returns seconds. With a really good initial
> >    time source this is not very precise. read_persistent_clock should
> >    return a struct timespec.
> > 3) xtime, raw_time, total_sleep_time, timekeeping_suspended, jiffies,
> >    the ntp state and probably a few other values may be better located
> >    in the struct timekeeper as well.
> 
> 
> You could also consolidate the clocksource_unregister() path and the
> clocksource_change_rating(0) path  , both are basically doing the same
> thing.. Neither one is heavily used..

I'm not quite sure I got this. If I look at the code:

/**
 * clocksource_change_rating - Change the rating of a registered clocksource
 */
void clocksource_change_rating(struct clocksource *cs, int rating)
{
        mutex_lock(&clocksource_mutex);
        cs->rating = rating;
        clocksource_select();
        mutex_unlock(&clocksource_mutex);
}
EXPORT_SYMBOL(clocksource_change_rating);

/**
 * clocksource_unregister - remove a registered clocksource
 */
void clocksource_unregister(struct clocksource *cs)
{
        mutex_lock(&clocksource_mutex);
        clocksource_dequeue_watchdog(cs);
        list_del(&cs->list);
        clocksource_select();
        mutex_unlock(&clocksource_mutex);
}
EXPORT_SYMBOL(clocksource_unregister);

the two functions do different things. What exactly is the idea you've
got in mind?

-- 
blue skies,
   Martin.

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

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Wed, 2009-07-29 at 18:50 +0200, Martin Schwidefsky wrote:

> void clocksource_change_rating(struct clocksource *cs, int rating)

> the two functions do different things. What exactly is the idea you've
> got in mind?

It's only the case when the rating goes to zero .. That makes the
clocksource unusable, which is very much like unregistering it..

Daniel

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Martin Schwidefsky]

On Wed, 29 Jul 2009 11:02:21 -0600
dwalker@fifo99.com wrote:

> On Wed, 2009-07-29 at 18:50 +0200, Martin Schwidefsky wrote:
> 
> > void clocksource_change_rating(struct clocksource *cs, int rating)
> 
> > the two functions do different things. What exactly is the idea you've
> > got in mind?
> 
> It's only the case when the rating goes to zero .. That makes the
> clocksource unusable, which is very much like unregistering it..

True, the clocksource code won't pick the clock any more as long as
there is an alternative clock available. It still shows up in the list
of clocks though and you can do an override with it. 

-- 
blue skies,
   Martin.

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

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Wed, 2009-07-29 at 19:09 +0200, Martin Schwidefsky wrote:
> On Wed, 29 Jul 2009 11:02:21 -0600
> dwalker@fifo99.com wrote:
> 
> > On Wed, 2009-07-29 at 18:50 +0200, Martin Schwidefsky wrote:
> > 
> > > void clocksource_change_rating(struct clocksource *cs, int rating)
> > 
> > > the two functions do different things. What exactly is the idea you've
> > > got in mind?
> > 
> > It's only the case when the rating goes to zero .. That makes the
> > clocksource unusable, which is very much like unregistering it..
> 
> True, the clocksource code won't pick the clock any more as long as
> there is an alternative clock available. It still shows up in the list
> of clocks though and you can do an override with it. 

I'm not sure allowing that type of override a good idea tho .. I don't
think it's considered a usable clock when the rating goes to zero.

Daniel

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Wed, 2009-07-29 at 19:09 +0200, Martin Schwidefsky wrote:
> On Wed, 29 Jul 2009 11:02:21 -0600
> dwalker@fifo99.com wrote:
> 
> > On Wed, 2009-07-29 at 18:50 +0200, Martin Schwidefsky wrote:
> > 
> > > void clocksource_change_rating(struct clocksource *cs, int rating)
> > 
> > > the two functions do different things. What exactly is the idea you've
> > > got in mind?
> > 
> > It's only the case when the rating goes to zero .. That makes the
> > clocksource unusable, which is very much like unregistering it..
> 
> True, the clocksource code won't pick the clock any more as long as
> there is an alternative clock available. It still shows up in the list
> of clocks though and you can do an override with it. 

Now that you mention it, if the clocks are sorted by rating (something
your removing) then the sysfs override available listing isn't in rated
order .. So if you do have zero rated clocks you wouldn't have any idea
from looking at the available list.. It could be solved just by printing
them in rated order in the sysfs code tho.

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Martin Schwidefsky]

On Wed, 29 Jul 2009 10:34:37 -0700
Daniel Walker <dwalker@fifo99.com> wrote:

> On Wed, 2009-07-29 at 19:09 +0200, Martin Schwidefsky wrote:
> > On Wed, 29 Jul 2009 11:02:21 -0600
> > dwalker@fifo99.com wrote:
> > 
> > > On Wed, 2009-07-29 at 18:50 +0200, Martin Schwidefsky wrote:
> > > 
> > > > void clocksource_change_rating(struct clocksource *cs, int rating)
> > > 
> > > > the two functions do different things. What exactly is the idea you've
> > > > got in mind?
> > > 
> > > It's only the case when the rating goes to zero .. That makes the
> > > clocksource unusable, which is very much like unregistering it..
> > 
> > True, the clocksource code won't pick the clock any more as long as
> > there is an alternative clock available. It still shows up in the list
> > of clocks though and you can do an override with it. 
> 
> Now that you mention it, if the clocks are sorted by rating (something
> your removing) then the sysfs override available listing isn't in rated
> order .. So if you do have zero rated clocks you wouldn't have any idea
> from looking at the available list.. It could be solved just by printing
> them in rated order in the sysfs code tho.

Ok, I'll readd the sorting. It is a user visible change after all.

-- 
blue skies,
   Martin.

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

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Martin Schwidefsky]

On Wed, 29 Jul 2009 11:17:16 -0600
dwalker@fifo99.com wrote:

> On Wed, 2009-07-29 at 19:09 +0200, Martin Schwidefsky wrote:
> > On Wed, 29 Jul 2009 11:02:21 -0600
> > dwalker@fifo99.com wrote:
> > 
> > > On Wed, 2009-07-29 at 18:50 +0200, Martin Schwidefsky wrote:
> > > 
> > > > void clocksource_change_rating(struct clocksource *cs, int rating)
> > > 
> > > > the two functions do different things. What exactly is the idea you've
> > > > got in mind?
> > > 
> > > It's only the case when the rating goes to zero .. That makes the
> > > clocksource unusable, which is very much like unregistering it..
> > 
> > True, the clocksource code won't pick the clock any more as long as
> > there is an alternative clock available. It still shows up in the list
> > of clocks though and you can do an override with it. 
> 
> I'm not sure allowing that type of override a good idea tho .. I don't
> think it's considered a usable clock when the rating goes to zero.

Override as the root user -> your foot, no? The whole override stuff is
there for the case that the clocksource selection picked a broken clock
and you want to force the system into a semi-working state. Ideally the
whole override would go away, but that is probably wishful thinking..

-- 
blue skies,
   Martin.

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

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Thu, 2009-07-30 at 12:53 +0200, Martin Schwidefsky wrote:

> > I'm not sure allowing that type of override a good idea tho .. I don't
> > think it's considered a usable clock when the rating goes to zero.
> 
> Override as the root user -> your foot, no? The whole override stuff is
> there for the case that the clocksource selection picked a broken clock
> and you want to force the system into a semi-working state. Ideally the
> whole override would go away, but that is probably wishful thinking..

I would agree if the system doesn't crash as a result, if it just starts
to operate funny then that's maybe acceptable. If you keep the change
rating function, you could potentially remove the unregister path..

Daniel

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Martin Schwidefsky]

On Thu, 30 Jul 2009 05:49:33 -0700
Daniel Walker <dwalker@fifo99.com> wrote:

> On Thu, 2009-07-30 at 12:53 +0200, Martin Schwidefsky wrote:
> 
> > > I'm not sure allowing that type of override a good idea tho .. I don't
> > > think it's considered a usable clock when the rating goes to zero.
> > 
> > Override as the root user -> your foot, no? The whole override stuff is
> > there for the case that the clocksource selection picked a broken clock
> > and you want to force the system into a semi-working state. Ideally the
> > whole override would go away, but that is probably wishful thinking..
> 
> I would agree if the system doesn't crash as a result, if it just starts
> to operate funny then that's maybe acceptable. If you keep the change
> rating function, you could potentially remove the unregister path..

Why shouldn't it be possible to have a clocksource as a module? I think
that the unregister path should stay. To really make it work we'd need
a function to force the system out of the one-shot mode though.

-- 
blue skies,
   Martin.

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

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Thu, 2009-07-30 at 15:04 +0200, Martin Schwidefsky wrote:
> On Thu, 30 Jul 2009 05:49:33 -0700
> Daniel Walker <dwalker@fifo99.com> wrote:
> 
> > On Thu, 2009-07-30 at 12:53 +0200, Martin Schwidefsky wrote:
> > 
> > > > I'm not sure allowing that type of override a good idea tho .. I don't
> > > > think it's considered a usable clock when the rating goes to zero.
> > > 
> > > Override as the root user -> your foot, no? The whole override stuff is
> > > there for the case that the clocksource selection picked a broken clock
> > > and you want to force the system into a semi-working state. Ideally the
> > > whole override would go away, but that is probably wishful thinking..
> > 
> > I would agree if the system doesn't crash as a result, if it just starts
> > to operate funny then that's maybe acceptable. If you keep the change
> > rating function, you could potentially remove the unregister path..
> 
> Why shouldn't it be possible to have a clocksource as a module? I think
> that the unregister path should stay. To really make it work we'd need
> a function to force the system out of the one-shot mode though.

Because I don't think there is a sane reason to allow it. It should be
more like if someone has a need for it, then let them add back the
unregister path and explain why they need it.

Daniel

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Martin Schwidefsky]

On Thu, 30 Jul 2009 07:49:12 -0600
dwalker@fifo99.com wrote:

> On Thu, 2009-07-30 at 15:04 +0200, Martin Schwidefsky wrote:
> > On Thu, 30 Jul 2009 05:49:33 -0700
> > Daniel Walker <dwalker@fifo99.com> wrote:
> > 
> > > On Thu, 2009-07-30 at 12:53 +0200, Martin Schwidefsky wrote:
> > > 
> > > > > I'm not sure allowing that type of override a good idea tho .. I don't
> > > > > think it's considered a usable clock when the rating goes to zero.
> > > > 
> > > > Override as the root user -> your foot, no? The whole override stuff is
> > > > there for the case that the clocksource selection picked a broken clock
> > > > and you want to force the system into a semi-working state. Ideally the
> > > > whole override would go away, but that is probably wishful thinking..
> > > 
> > > I would agree if the system doesn't crash as a result, if it just starts
> > > to operate funny then that's maybe acceptable. If you keep the change
> > > rating function, you could potentially remove the unregister path..
> > 
> > Why shouldn't it be possible to have a clocksource as a module? I think
> > that the unregister path should stay. To really make it work we'd need
> > a function to force the system out of the one-shot mode though.
> 
> Because I don't think there is a sane reason to allow it. It should be
> more like if someone has a need for it, then let them add back the
> unregister path and explain why they need it.

I hear your. Without the need to unregister a clock the whole watchdog
unregistration can be dropped as well before it ever has been added.

There are currently two clocksources that use clocksource_unregister,
the pit clock and the sgi_rtc clock. The sgi_rtc clock in
arch/x86/kernel/uv_time.c could be modified slightly to avoid the
clocksource_unregister call. The pit clock in arch/x86/kernel/i8253.c
is odd, it uses clocksource_unregister in the set_mode callback if the
mode is set to SHUTDOWN, UNUSED or ONESHOT. Very strange..

-- 
blue skies,
   Martin.

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

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[john stultz]

On Thu, 2009-07-30 at 12:53 +0200, Martin Schwidefsky wrote:
> On Wed, 29 Jul 2009 11:17:16 -0600
> dwalker@fifo99.com wrote:
> 
> > On Wed, 2009-07-29 at 19:09 +0200, Martin Schwidefsky wrote:
> > > On Wed, 29 Jul 2009 11:02:21 -0600
> > > dwalker@fifo99.com wrote:
> > > 
> > > > On Wed, 2009-07-29 at 18:50 +0200, Martin Schwidefsky wrote:
> > > > 
> > > > > void clocksource_change_rating(struct clocksource *cs, int rating)
> > > > 
> > > > > the two functions do different things. What exactly is the idea you've
> > > > > got in mind?
> > > > 
> > > > It's only the case when the rating goes to zero .. That makes the
> > > > clocksource unusable, which is very much like unregistering it..
> > > 
> > > True, the clocksource code won't pick the clock any more as long as
> > > there is an alternative clock available. It still shows up in the list
> > > of clocks though and you can do an override with it. 
> > 
> > I'm not sure allowing that type of override a good idea tho .. I don't
> > think it's considered a usable clock when the rating goes to zero.
> 
> Override as the root user -> your foot, no? The whole override stuff is
> there for the case that the clocksource selection picked a broken clock
> and you want to force the system into a semi-working state. Ideally the
> whole override would go away, but that is probably wishful thinking..

Its also not only for when a system is broken, but quite often is used
when the system selects a slower clocksource out of caution and the user
wants to override that decision. The kernel really should get it right,
but there is always the case of an old kernel on new hardware that might
require it. 

thanks
-john

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[john stultz]

On Thu, 2009-07-30 at 06:49 -0700, Daniel Walker wrote:
> On Thu, 2009-07-30 at 15:04 +0200, Martin Schwidefsky wrote:
> > On Thu, 30 Jul 2009 05:49:33 -0700
> > Daniel Walker <dwalker@fifo99.com> wrote:
> > 
> > > On Thu, 2009-07-30 at 12:53 +0200, Martin Schwidefsky wrote:
> > > 
> > > > > I'm not sure allowing that type of override a good idea tho .. I don't
> > > > > think it's considered a usable clock when the rating goes to zero.
> > > > 
> > > > Override as the root user -> your foot, no? The whole override stuff is
> > > > there for the case that the clocksource selection picked a broken clock
> > > > and you want to force the system into a semi-working state. Ideally the
> > > > whole override would go away, but that is probably wishful thinking..
> > > 
> > > I would agree if the system doesn't crash as a result, if it just starts
> > > to operate funny then that's maybe acceptable. If you keep the change
> > > rating function, you could potentially remove the unregister path..
> > 
> > Why shouldn't it be possible to have a clocksource as a module? I think
> > that the unregister path should stay. To really make it work we'd need
> > a function to force the system out of the one-shot mode though.
> 
> Because I don't think there is a sane reason to allow it. It should be
> more like if someone has a need for it, then let them add back the
> unregister path and explain why they need it.

Clocksources as modules was one of the initial design goals I had way
back. The benefit being that an older distro kernel could be made to
support newer stranger hardware via a clocksource driver. While the
hardware vendors have for the most part consolidated on HPET/ACPI PM
which has mostly avoided the need, I still think its worth preserving.

thanks
-john

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Thu, 2009-07-30 at 10:16 -0700, john stultz wrote:
> Clocksources as modules was one of the initial design goals I had way
> back. The benefit being that an older distro kernel could be made to
> support newer stranger hardware via a clocksource driver. While the
> hardware vendors have for the most part consolidated on HPET/ACPI PM
> which has mostly avoided the need, I still think its worth preserving.

If the PIT case is a real use case for unregister than we can keep it
around. If not, then that path just becomes unused and all unused code
is open for removal from my perspective.

If the case you describe above is a good one, then someone eventually
will add back the unregister path. Which should come with a good reason
and with an actual user of the code..

Daniel

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Andreas Mohr]

Hi,

> On Thu, 2009-07-30 at 10:16 -0700, john stultz wrote:
> > Clocksources as modules was one of the initial design goals I had way
> > back. The benefit being that an older distro kernel could be made to
> > support newer stranger hardware via a clocksource driver. While the
> > hardware vendors have for the most part consolidated on HPET/ACPI PM
> > which has mostly avoided the need, I still think its worth preserving.
> 
> If the PIT case is a real use case for unregister than we can keep it
> around. If not, then that path just becomes unused and all unused code
> is open for removal from my perspective.
> 
> If the case you describe above is a good one, then someone eventually
> will add back the unregister path. Which should come with a good reason
> and with an actual user of the code..
> 
> Daniel

I'm still having a tendency towards unhappiness about my snd-azf3328
PCI soundcard driver and its new use of clock_event_device,
without a proper unload path existing (read: clock_event_device is the sole one
of about 7 different driver components - PCM, I2S, OPL3, MPU401, AC97 mixer,
joystick, ALSA sequencer timer / clock_event_device - which now suddenly
managed to prevent the driver from being unloadable for the first time
in its entire history) and not too many explanations as to how to
get this working optimally...

Would that be enough of a justification? ;-P

Plus [OT], would anyone perhaps have an explanation why I'm getting stalls
every couple seconds (probably a race between set_next_event() programming
the next value already and timer-related interrupt handling causing a
nearby trigger to get lost, but I don't know how to resolve this
- how do other drivers manage to handle this easily racey activity??).
BTW mouse interrupt activity does NOT manage to cancel these stalls.
This is especially noticeable during video playback.

Thanks,

Andreas Mohr

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[john stultz]

On Thu, 2009-07-30 at 11:08 -0700, Daniel Walker wrote:
> On Thu, 2009-07-30 at 10:16 -0700, john stultz wrote:
> > Clocksources as modules was one of the initial design goals I had way
> > back. The benefit being that an older distro kernel could be made to
> > support newer stranger hardware via a clocksource driver. While the
> > hardware vendors have for the most part consolidated on HPET/ACPI PM
> > which has mostly avoided the need, I still think its worth preserving.
> 
> If the PIT case is a real use case for unregister than we can keep it
> around. If not, then that path just becomes unused and all unused code
> is open for removal from my perspective.
> 
> If the case you describe above is a good one, then someone eventually
> will add back the unregister path. Which should come with a good reason
> and with an actual user of the code..

The case I describe above is one where the user of the code doesn't
necessarily have the ability to add back the unregister path. 

Old distro kernels can be difficult to make changes to when new hardware
is later released, so being able to just backport a module, compile and
load it to get a unexpectedly strange new bit of hardware to work with
an older distro kernel seems valuable enough to keep the code around to
me.

thanks
-john

Re: [RFC][patch 01/12] introduce timekeeping_leap_insert

[john stultz]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (leap_insert.diff)
> From: john stultz <johnstul@us.ibm.com>
> 
> ---
>  include/linux/time.h      |    1 +
>  kernel/time/ntp.c         |    7 ++-----
>  kernel/time/timekeeping.c |    7 +++++++
>  3 files changed, 10 insertions(+), 5 deletions(-)


Signed-off-by: John Stultz <johnstul@us.ibm.com>

-john


> Index: linux-2.6/include/linux/time.h
> ===================================================================
> --- linux-2.6.orig/include/linux/time.h
> +++ linux-2.6/include/linux/time.h
> @@ -147,6 +147,7 @@ extern struct timespec timespec_trunc(st
>  extern int timekeeping_valid_for_hres(void);
>  extern void update_wall_time(void);
>  extern void update_xtime_cache(u64 nsec);
> +extern void timekeeping_leap_insert(int leapsecond);
> 
>  struct tms;
>  extern void do_sys_times(struct tms *);
> Index: linux-2.6/kernel/time/ntp.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/ntp.c
> +++ linux-2.6/kernel/time/ntp.c
> @@ -194,8 +194,7 @@ static enum hrtimer_restart ntp_leap_sec
>  	case TIME_OK:
>  		break;
>  	case TIME_INS:
> -		xtime.tv_sec--;
> -		wall_to_monotonic.tv_sec++;
> +		timekeeping_leap_insert(-1);
>  		time_state = TIME_OOP;
>  		printk(KERN_NOTICE
>  			"Clock: inserting leap second 23:59:60 UTC\n");
> @@ -203,9 +202,8 @@ static enum hrtimer_restart ntp_leap_sec
>  		res = HRTIMER_RESTART;
>  		break;
>  	case TIME_DEL:
> -		xtime.tv_sec++;
> +		timekeeping_leap_insert(1);
>  		time_tai--;
> -		wall_to_monotonic.tv_sec--;
>  		time_state = TIME_WAIT;
>  		printk(KERN_NOTICE
>  			"Clock: deleting leap second 23:59:59 UTC\n");
> @@ -219,7 +217,6 @@ static enum hrtimer_restart ntp_leap_sec
>  			time_state = TIME_OK;
>  		break;
>  	}
> -	update_vsyscall(&xtime, clock);
> 
>  	write_sequnlock(&xtime_lock);
> 
> Index: linux-2.6/kernel/time/timekeeping.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/timekeeping.c
> +++ linux-2.6/kernel/time/timekeeping.c
> @@ -58,6 +58,13 @@ void update_xtime_cache(u64 nsec)
> 
>  struct clocksource *clock;
> 
> +/* must hold xtime_lock */
> +void timekeeping_leap_insert(int leapsecond)
> +{
> +	xtime.tv_sec += leapsecond;
> +	wall_to_monotonic.tv_sec -= leapsecond;
> +	update_vsyscall(&xtime, clock);
> +}
> 
>  #ifdef CONFIG_GENERIC_TIME
>  /**
> 

Re: [RFC][patch 02/12] remove clocksource inline functions

[john stultz]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (clocksource-inline.diff)
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Remove clocksource_read, clocksource_enable and clocksource_disable
> inline functions. No functional change.
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: john stultz <johnstul@us.ibm.com>
> Cc: Daniel Walker <dwalker@fifo99.com>
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

Again, Magnus's fix for mult_orig that Thomas has (or *should* have)
queued at this point for 2.6.31 will collide with this, but the fix is
trivial.

Acked-by: John Stultz <johnstul@us.ibm.com>


> ---
>  include/linux/clocksource.h |   46 --------------------------------------------
>  kernel/time/timekeeping.c   |   32 +++++++++++++++++-------------
>  2 files changed, 18 insertions(+), 60 deletions(-)
> 
> Index: linux-2.6/kernel/time/timekeeping.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/timekeeping.c
> +++ linux-2.6/kernel/time/timekeeping.c
> @@ -79,7 +79,7 @@ static void clocksource_forward_now(void
>  	cycle_t cycle_now, cycle_delta;
>  	s64 nsec;
> 
> -	cycle_now = clocksource_read(clock);
> +	cycle_now = clock->read(clock);
>  	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
>  	clock->cycle_last = cycle_now;
> 
> @@ -114,7 +114,7 @@ void getnstimeofday(struct timespec *ts)
>  		*ts = xtime;
> 
>  		/* read clocksource: */
> -		cycle_now = clocksource_read(clock);
> +		cycle_now = clock->read(clock);
> 
>  		/* calculate the delta since the last update_wall_time: */
>  		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -146,7 +146,7 @@ ktime_t ktime_get(void)
>  		nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
> 
>  		/* read clocksource: */
> -		cycle_now = clocksource_read(clock);
> +		cycle_now = clock->read(clock);
> 
>  		/* calculate the delta since the last update_wall_time: */
>  		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -186,7 +186,7 @@ void ktime_get_ts(struct timespec *ts)
>  		tomono = wall_to_monotonic;
> 
>  		/* read clocksource: */
> -		cycle_now = clocksource_read(clock);
> +		cycle_now = clock->read(clock);
> 
>  		/* calculate the delta since the last update_wall_time: */
>  		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -274,16 +274,18 @@ static void change_clocksource(void)
> 
>  	clocksource_forward_now();
> 
> -	if (clocksource_enable(new))
> +	if (new->enable && ! new->enable(new))
>  		return;
> +	/* save mult_orig on enable */
> +	new->mult_orig = new->mult;
> 
>  	new->raw_time = clock->raw_time;
>  	old = clock;
>  	clock = new;
> -	clocksource_disable(old);
> +	if (old->disable)
> +		old->disable(old);
> 
> -	clock->cycle_last = 0;
> -	clock->cycle_last = clocksource_read(clock);
> +	clock->cycle_last = clock->read(clock);
>  	clock->error = 0;
>  	clock->xtime_nsec = 0;
>  	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
> @@ -373,7 +375,7 @@ void getrawmonotonic(struct timespec *ts
>  		seq = read_seqbegin(&xtime_lock);
> 
>  		/* read clocksource: */
> -		cycle_now = clocksource_read(clock);
> +		cycle_now = clock->read(clock);
> 
>  		/* calculate the delta since the last update_wall_time: */
>  		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -435,9 +437,12 @@ void __init timekeeping_init(void)
>  	ntp_init();
> 
>  	clock = clocksource_get_next();
> -	clocksource_enable(clock);
> +	if (clock->enable)
> +		clock->enable(clock);
> +	/* save mult_orig on enable */
> +	clock->mult_orig = clock->mult;
>  	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
> -	clock->cycle_last = clocksource_read(clock);
> +	clock->cycle_last = clock->read(clock);
> 
>  	xtime.tv_sec = sec;
>  	xtime.tv_nsec = 0;
> @@ -477,8 +482,7 @@ static int timekeeping_resume(struct sys
>  	}
>  	update_xtime_cache(0);
>  	/* re-base the last cycle value */
> -	clock->cycle_last = 0;
> -	clock->cycle_last = clocksource_read(clock);
> +	clock->cycle_last = clock->read(clock);
>  	clock->error = 0;
>  	timekeeping_suspended = 0;
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
> @@ -630,7 +634,7 @@ void update_wall_time(void)
>  		return;
> 
>  #ifdef CONFIG_GENERIC_TIME
> -	offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
> +	offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
>  #else
>  	offset = clock->cycle_interval;
>  #endif
> Index: linux-2.6/include/linux/clocksource.h
> ===================================================================
> --- linux-2.6.orig/include/linux/clocksource.h
> +++ linux-2.6/include/linux/clocksource.h
> @@ -268,52 +268,6 @@ static inline u32 clocksource_hz2mult(u3
>  }
> 
>  /**
> - * clocksource_read: - Access the clocksource's current cycle value
> - * @cs:		pointer to clocksource being read
> - *
> - * Uses the clocksource to return the current cycle_t value
> - */
> -static inline cycle_t clocksource_read(struct clocksource *cs)
> -{
> -	return cs->read(cs);
> -}
> -
> -/**
> - * clocksource_enable: - enable clocksource
> - * @cs:		pointer to clocksource
> - *
> - * Enables the specified clocksource. The clocksource callback
> - * function should start up the hardware and setup mult and field
> - * members of struct clocksource to reflect hardware capabilities.
> - */
> -static inline int clocksource_enable(struct clocksource *cs)
> -{
> -	int ret = 0;
> -
> -	if (cs->enable)
> -		ret = cs->enable(cs);
> -
> -	/* save mult_orig on enable */
> -	cs->mult_orig = cs->mult;
> -
> -	return ret;
> -}
> -
> -/**
> - * clocksource_disable: - disable clocksource
> - * @cs:		pointer to clocksource
> - *
> - * Disables the specified clocksource. The clocksource callback
> - * function should power down the now unused hardware block to
> - * save power.
> - */
> -static inline void clocksource_disable(struct clocksource *cs)
> -{
> -	if (cs->disable)
> -		cs->disable(cs);
> -}
> -
> -/**
>   * cyc2ns - converts clocksource cycles to nanoseconds
>   * @cs:		Pointer to clocksource
>   * @cycles:	Cycles
> 

Re: [RFC][patch 11/12] timekeeper read clock helper functions

[john stultz]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (timekeeper-helper.diff)
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Add timekeeper_read_clock_ntp and timekeeper_read_clock_raw and use
> them for getnstimeofday, ktime_get, ktime_get_ts and getrawmonotonic.
>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: john stultz <johnstul@us.ibm.com>
> Cc: Daniel Walker <dwalker@fifo99.com>
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  kernel/time/timekeeping.c |   91 +++++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 53 deletions(-)
> 
> Index: linux-2.6/kernel/time/timekeeping.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/timekeeping.c
> +++ linux-2.6/kernel/time/timekeeping.c
> @@ -84,6 +84,40 @@ static void timekeeper_setup_internals(s
>  	timekeeper.shift = clock->shift;
>  }
> 
> +/* Timekeeper helper functions. */
> +static inline s64 timekeeper_read_clock_ntp(void)
> +{
> +	cycle_t cycle_now, cycle_delta;
> +	struct clocksource *clock;
> +
> +	/* read clocksource: */
> +	clock = timekeeper.clock;
> +	cycle_now = clock->read(clock);
> +

I know it seems nice to have it here, but I think these helpers would be
more reusable in other contexts if they took the cycle_now value as an
argument. Also I'd drop the ntp bit, just to avoid confusing it with
some ntp specific function. So:

 timekeeping_get_ns(cycle_t now);
 timekeeping_get_ns_raw(cycle_t now);

That way in some situations we don't have to make two accesses to the
hardware if we want to get both values at the same point.

Seem reasonable?

thanks
-john



> +	/* calculate the delta since the last update_wall_time: */
> +	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> +
> +	/* return delta convert to nanoseconds using ntp adjusted mult. */
> +	return clocksource_cyc2ns(cycle_delta, timekeeper.mult,
> +				  timekeeper.shift);
> +}
> +
> +static inline s64 timekeeper_read_clock_raw(void)
> +{
> +	cycle_t cycle_now, cycle_delta;
> +	struct clocksource *clock;
> +
> +	/* read clocksource: */
> +	clock = timekeeper.clock;
> +	cycle_now = clock->read(clock);
> +
> +	/* calculate the delta since the last update_wall_time: */
> +	cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> +
> +	/* return delta convert to nanoseconds using ntp adjusted mult. */
> +	return clocksource_cyc2ns(cycle_delta, clock->mult, clock->shift);
> +}
> +
>  /*
>   * This read-write spinlock protects us from races in SMP while
>   * playing with xtime.
> @@ -172,8 +206,6 @@ static void timekeeping_forward_now(void
>   */
>  void getnstimeofday(struct timespec *ts)
>  {
> -	cycle_t cycle_now, cycle_delta;
> -	struct clocksource *clock;
>  	unsigned long seq;
>  	s64 nsecs;
> 
> @@ -183,17 +215,7 @@ void getnstimeofday(struct timespec *ts)
>  		seq = read_seqbegin(&xtime_lock);
> 
>  		*ts = xtime;
> -
> -		/* read clocksource: */
> -		clock = timekeeper.clock;
> -		cycle_now = clock->read(clock);
> -
> -		/* calculate the delta since the last update_wall_time: */
> -		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> -
> -		/* convert to nanoseconds: */
> -		nsecs = clocksource_cyc2ns(cycle_delta, timekeeper.mult,
> -					   timekeeper.shift);
> +		nsecs = timekeeper_read_clock_ntp();
> 
>  		/* If arch requires, add in gettimeoffset() */
>  		nsecs += arch_gettimeoffset();
> @@ -207,8 +229,6 @@ EXPORT_SYMBOL(getnstimeofday);
> 
>  ktime_t ktime_get(void)
>  {
> -	cycle_t cycle_now, cycle_delta;
> -	struct clocksource *clock;
>  	unsigned int seq;
>  	s64 secs, nsecs;
> 
> @@ -218,17 +238,7 @@ ktime_t ktime_get(void)
>  		seq = read_seqbegin(&xtime_lock);
>  		secs = xtime.tv_sec + wall_to_monotonic.tv_sec;
>  		nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
> -
> -		/* read clocksource: */
> -		clock = timekeeper.clock;
> -		cycle_now = clock->read(clock);
> -
> -		/* calculate the delta since the last update_wall_time: */
> -		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> -
> -		/* convert to nanoseconds: */
> -		nsecs += clocksource_cyc2ns(cycle_delta, timekeeper.mult,
> -					    timekeeper.shift);
> +		nsecs += timekeeper_read_clock_ntp();
> 
>  	} while (read_seqretry(&xtime_lock, seq));
>  	/*
> @@ -249,8 +259,6 @@ EXPORT_SYMBOL_GPL(ktime_get);
>   */
>  void ktime_get_ts(struct timespec *ts)
>  {
> -	cycle_t cycle_now, cycle_delta;
> -	struct clocksource *clock;
>  	struct timespec tomono;
>  	unsigned int seq;
>  	s64 nsecs;
> @@ -261,17 +269,7 @@ void ktime_get_ts(struct timespec *ts)
>  		seq = read_seqbegin(&xtime_lock);
>  		*ts = xtime;
>  		tomono = wall_to_monotonic;
> -
> -		/* read clocksource: */
> -		clock = timekeeper.clock;
> -		cycle_now = clock->read(clock);
> -
> -		/* calculate the delta since the last update_wall_time: */
> -		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> -
> -		/* convert to nanoseconds: */
> -		nsecs = clocksource_cyc2ns(cycle_delta, timekeeper.mult,
> -					   timekeeper.shift);
> +		nsecs = timekeeper_read_clock_ntp();
> 
>  	} while (read_seqretry(&xtime_lock, seq));
> 
> @@ -433,23 +431,10 @@ void getrawmonotonic(struct timespec *ts
>  {
>  	unsigned long seq;
>  	s64 nsecs;
> -	cycle_t cycle_now, cycle_delta;
> -	struct clocksource *clock;
> 
>  	do {
>  		seq = read_seqbegin(&xtime_lock);
> -
> -		/* read clocksource: */
> -		clock = timekeeper.clock;
> -		cycle_now = clock->read(clock);
> -
> -		/* calculate the delta since the last update_wall_time: */
> -		cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> -
> -		/* convert to nanoseconds: */
> -		nsecs = clocksource_cyc2ns(cycle_delta, clock->mult,
> -					   clock->shift);
> -
> +		nsecs = timekeeper_read_clock_raw();
>  		*ts = raw_time;
> 
>  	} while (read_seqretry(&xtime_lock, seq));
> 

Re: [RFC][patch 02/12] remove clocksource inline functions

[Christoph Hellwig]

On Wed, Jul 29, 2009 at 07:15:30AM -0700, Daniel Walker wrote:
> On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > plain text document attachment (clocksource-inline.diff)
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > 
> > Remove clocksource_read, clocksource_enable and clocksource_disable
> > inline functions. No functional change.
> > 
> 
> Your still not really explaining this one, is this suppose to be
> cleaner? Or is this related to some other part of your clean up?

It removes an entirely useless layer of obsfucation.

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

[john stultz]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (timekeeper-mult.diff)
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> 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@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: john stultz <johnstul@us.ibm.com>
> Cc: Daniel Walker <dwalker@fifo99.com>
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  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

Re: [RFC][patch 09/12] add xtime_shift and ntp_error_shift to struct timekeeper

[john stultz]

On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (timekeeper-shift.diff)
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> The xtime_nsec value in the timekeeper structure is shifted by a few
> bits to improve precision. This happens to be the same value as the
> clock->shift. To improve readability add xtime_shift to the timekeeper
> and use it instead of the clock->shift. Likewise add ntp_error_shift
> and replace all (NTP_SCALE_SHIFT - clock->shift) expressions.
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: john stultz <johnstul@us.ibm.com>
> Cc: Daniel Walker <dwalker@fifo99.com>
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  kernel/time/timekeeping.c |   33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> Index: linux-2.6/kernel/time/timekeeping.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/timekeeping.c
> +++ linux-2.6/kernel/time/timekeeping.c
> @@ -27,6 +27,8 @@ struct timekeeper {
>  	u32	raw_interval;
>  	u64	xtime_nsec;
>  	s64	ntp_error;
> +	int	xtime_shift;
> +	int	ntp_error_shift;
>  };

I suspect ntp_error_shift is one of the lease intuitive values in the
timekeeper structure. They all probably need nice comments explaining
what they store, but especially this one. 

Something like:

struct clocksource *clock; /* current clocksource used for timekeeping*/
cycle_t cycle_interval;	   /* fixed chunk of cycles used in accumulation */
u64     xtime_interval;    /* number clock shifted nsecs accumulated per cycle_interval */
u32	raw_interval;      /* raw nsecs accumulated per cycle_interval */
u64	xtime_nsec;        /* clock shifted nsecs remainder not stored in xtime.tv_nsec */
s64	ntp_error;         /* Difference between accumulated time and NTP time in ntp shifted nsecs */
int	xtime_shift;       /* The shift value of the current clocksource */
int	ntp_error_shift;   /* Shift conversion between clock shifted nsecs and ntp shifted nsecs */


Other then that this patch looks ok, but will need rigorous testing, as
its very complicated and difficult to understand code thats being
changed.

thanks
-john

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Thu, 2009-07-30 at 13:56 -0700, john stultz wrote:
> On Thu, 2009-07-30 at 11:08 -0700, Daniel Walker wrote:
> > On Thu, 2009-07-30 at 10:16 -0700, john stultz wrote:
> > > Clocksources as modules was one of the initial design goals I had way
> > > back. The benefit being that an older distro kernel could be made to
> > > support newer stranger hardware via a clocksource driver. While the
> > > hardware vendors have for the most part consolidated on HPET/ACPI PM
> > > which has mostly avoided the need, I still think its worth preserving.
> > 
> > If the PIT case is a real use case for unregister than we can keep it
> > around. If not, then that path just becomes unused and all unused code
> > is open for removal from my perspective.
> > 
> > If the case you describe above is a good one, then someone eventually
> > will add back the unregister path. Which should come with a good reason
> > and with an actual user of the code..
> 
> The case I describe above is one where the user of the code doesn't
> necessarily have the ability to add back the unregister path. 

I'm not sure I understand your example.. Your saying a situation where
the kernel can't modified and reloaded, and the hardware clocks aren't
fully implemented in code yet?

> Old distro kernels can be difficult to make changes to when new hardware
> is later released, so being able to just backport a module, compile and
> load it to get a unexpectedly strange new bit of hardware to work with
> an older distro kernel seems valuable enough to keep the code around to
> me.

You can just as easily back port the code as a built in, and reload the
kernel right? Why would it need to be a module?

Daniel

Re: [RFC][patch 11/12] timekeeper read clock helper functions

[Martin Schwidefsky]

On Thu, 30 Jul 2009 14:39:54 -0700
john stultz <johnstul@us.ibm.com> wrote:

> On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > plain text document attachment (timekeeper-helper.diff)
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > 
> > Add timekeeper_read_clock_ntp and timekeeper_read_clock_raw and use
> > them for getnstimeofday, ktime_get, ktime_get_ts and getrawmonotonic.
> >
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: john stultz <johnstul@us.ibm.com>
> > Cc: Daniel Walker <dwalker@fifo99.com>
> > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > ---
> >  kernel/time/timekeeping.c |   91 +++++++++++++++++++---------------------------
> >  1 file changed, 38 insertions(+), 53 deletions(-)
> > 
> > Index: linux-2.6/kernel/time/timekeeping.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/time/timekeeping.c
> > +++ linux-2.6/kernel/time/timekeeping.c
> > @@ -84,6 +84,40 @@ static void timekeeper_setup_internals(s
> >  	timekeeper.shift = clock->shift;
> >  }
> > 
> > +/* Timekeeper helper functions. */
> > +static inline s64 timekeeper_read_clock_ntp(void)
> > +{
> > +	cycle_t cycle_now, cycle_delta;
> > +	struct clocksource *clock;
> > +
> > +	/* read clocksource: */
> > +	clock = timekeeper.clock;
> > +	cycle_now = clock->read(clock);
> > +
> 
> I know it seems nice to have it here, but I think these helpers would be
> more reusable in other contexts if they took the cycle_now value as an
> argument. Also I'd drop the ntp bit, just to avoid confusing it with
> some ntp specific function. So:
> 
>  timekeeping_get_ns(cycle_t now);
>  timekeeping_get_ns_raw(cycle_t now);
> 
> That way in some situations we don't have to make two accesses to the
> hardware if we want to get both values at the same point.
> 
> Seem reasonable?

The new names are fine but if we pull out the ->read call to the
caller we again have a rather strange mix. The caller gets the cycle
value using some clock, the helper uses the value of the timekeeper
clock or the timerkeeper mult/shift. I would like to keep the ->read
call in the helper. Is there a situation where we need both
calculations for the same cycles value? There is none in the current
code as far as I can see.

-- 
blue skies,
   Martin.

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

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

[Martin Schwidefsky]

On Thu, 30 Jul 2009 15:04:48 -0700
john stultz <johnstul@us.ibm.com> wrote:

> On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > plain text document attachment (timekeeper-mult.diff)
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > 
> > 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@elte.hu>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: john stultz <johnstul@us.ibm.com>
> > Cc: Daniel Walker <dwalker@fifo99.com>
> > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > ---
> >  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.

Re: [RFC][patch 11/12] timekeeper read clock helper functions

[john stultz]

On Fri, 2009-07-31 at 09:45 +0200, Martin Schwidefsky wrote:
> On Thu, 30 Jul 2009 14:39:54 -0700
> john stultz <johnstul@us.ibm.com> wrote:
> 
> > On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > > plain text document attachment (timekeeper-helper.diff)
> > > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > 
> > > Add timekeeper_read_clock_ntp and timekeeper_read_clock_raw and use
> > > them for getnstimeofday, ktime_get, ktime_get_ts and getrawmonotonic.
> > >
> > > Cc: Ingo Molnar <mingo@elte.hu>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: john stultz <johnstul@us.ibm.com>
> > > Cc: Daniel Walker <dwalker@fifo99.com>
> > > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > ---
> > >  kernel/time/timekeeping.c |   91 +++++++++++++++++++---------------------------
> > >  1 file changed, 38 insertions(+), 53 deletions(-)
> > > 
> > > Index: linux-2.6/kernel/time/timekeeping.c
> > > ===================================================================
> > > --- linux-2.6.orig/kernel/time/timekeeping.c
> > > +++ linux-2.6/kernel/time/timekeeping.c
> > > @@ -84,6 +84,40 @@ static void timekeeper_setup_internals(s
> > >  	timekeeper.shift = clock->shift;
> > >  }
> > > 
> > > +/* Timekeeper helper functions. */
> > > +static inline s64 timekeeper_read_clock_ntp(void)
> > > +{
> > > +	cycle_t cycle_now, cycle_delta;
> > > +	struct clocksource *clock;
> > > +
> > > +	/* read clocksource: */
> > > +	clock = timekeeper.clock;
> > > +	cycle_now = clock->read(clock);
> > > +
> > 
> > I know it seems nice to have it here, but I think these helpers would be
> > more reusable in other contexts if they took the cycle_now value as an
> > argument. Also I'd drop the ntp bit, just to avoid confusing it with
> > some ntp specific function. So:
> > 
> >  timekeeping_get_ns(cycle_t now);
> >  timekeeping_get_ns_raw(cycle_t now);
> > 
> > That way in some situations we don't have to make two accesses to the
> > hardware if we want to get both values at the same point.
> > 
> > Seem reasonable?
> 
> The new names are fine but if we pull out the ->read call to the
> caller we again have a rather strange mix. The caller gets the cycle
> value using some clock, the helper uses the value of the timekeeper
> clock or the timerkeeper mult/shift. I would like to keep the ->read
> call in the helper. Is there a situation where we need both
> calculations for the same cycles value? There is none in the current
> code as far as I can see.

One instance: Changing it would allow us to use this code in
timekeeping_forward() as we want to be able to accumulate the current
cycles into xtime and then set cycles_last equal to the read value at
that time. By embedding the read into the timekeeping_read_clock_x() it
avoids us from using the function with any of the management calls. If
you look at the mega-patch I sent out earlier, I think there were a few
uses for such helper functions in update_wall_time and other spots.

Also I have had some discussions with folks that would like to have the
ability to generate multiple CLOCK_ID values at the same "instance".
There isn't a good interface to userland for such a feature, but I'm
hesitant to make it difficult to compute on the kernel side.

thanks
-john

Re: [RFC][patch 10/12] move NTP adjusted clock multiplier to struct 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@us.ibm.com> wrote:
> 
> > On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > > plain text document attachment (timekeeper-mult.diff)
> > > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > 
> > > 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@elte.hu>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: john stultz <johnstul@us.ibm.com>
> > > Cc: Daniel Walker <dwalker@fifo99.com>
> > > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > ---
> > >  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

Re: [RFC][patch 09/12] add xtime_shift and ntp_error_shift to struct timekeeper

[Martin Schwidefsky]

On Thu, 30 Jul 2009 15:15:23 -0700
john stultz <johnstul@us.ibm.com> wrote:

> On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > plain text document attachment (timekeeper-shift.diff)
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > 
> > The xtime_nsec value in the timekeeper structure is shifted by a few
> > bits to improve precision. This happens to be the same value as the
> > clock->shift. To improve readability add xtime_shift to the timekeeper
> > and use it instead of the clock->shift. Likewise add ntp_error_shift
> > and replace all (NTP_SCALE_SHIFT - clock->shift) expressions.
> > 
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: john stultz <johnstul@us.ibm.com>
> > Cc: Daniel Walker <dwalker@fifo99.com>
> > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > ---
> >  kernel/time/timekeeping.c |   33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> > 
> > Index: linux-2.6/kernel/time/timekeeping.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/time/timekeeping.c
> > +++ linux-2.6/kernel/time/timekeeping.c
> > @@ -27,6 +27,8 @@ struct timekeeper {
> >  	u32	raw_interval;
> >  	u64	xtime_nsec;
> >  	s64	ntp_error;
> > +	int	xtime_shift;
> > +	int	ntp_error_shift;
> >  };
> 
> I suspect ntp_error_shift is one of the lease intuitive values in the
> timekeeper structure. They all probably need nice comments explaining
> what they store, but especially this one. 
> 
> Something like:
> 
> struct clocksource *clock; /* current clocksource used for timekeeping*/
> cycle_t cycle_interval;	   /* fixed chunk of cycles used in accumulation */
> u64     xtime_interval;    /* number clock shifted nsecs accumulated per cycle_interval */
> u32	raw_interval;      /* raw nsecs accumulated per cycle_interval */
> u64	xtime_nsec;        /* clock shifted nsecs remainder not stored in xtime.tv_nsec */
> s64	ntp_error;         /* Difference between accumulated time and NTP time in ntp shifted nsecs */
> int	xtime_shift;       /* The shift value of the current clocksource */
> int	ntp_error_shift;   /* Shift conversion between clock shifted nsecs and ntp shifted nsecs */

For xtime_shift I would like to use a different comment. As I think
about the value it is the number of bits the xtime_nsec value is
shifted to improve precision. It is the same as clock->shift but the
semantics is different. How about:

        /* Shift conversion between the xtime_nsec remainder and nsecs */
        int     xtime_shift;

The other comments are good, I've added them.


> Other then that this patch looks ok, but will need rigorous testing, as
> its very complicated and difficult to understand code thats being
> changed.

This patch in particular is rather easy to verify, but the overall series
will definitely need some time in linux-next.

-- 
blue skies,
   Martin.

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

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

[Martin Schwidefsky]

On Fri, 31 Jul 2009 01:12:59 -0700
john stultz <johnstul@us.ibm.com> wrote:

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

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[john stultz]

On Thu, 2009-07-30 at 22:33 -0700, Daniel Walker wrote: 
> On Thu, 2009-07-30 at 13:56 -0700, john stultz wrote:
> > On Thu, 2009-07-30 at 11:08 -0700, Daniel Walker wrote:
> > > On Thu, 2009-07-30 at 10:16 -0700, john stultz wrote:
> > > > Clocksources as modules was one of the initial design goals I had way
> > > > back. The benefit being that an older distro kernel could be made to
> > > > support newer stranger hardware via a clocksource driver. While the
> > > > hardware vendors have for the most part consolidated on HPET/ACPI PM
> > > > which has mostly avoided the need, I still think its worth preserving.
> > > 
> > > If the PIT case is a real use case for unregister than we can keep it
> > > around. If not, then that path just becomes unused and all unused code
> > > is open for removal from my perspective.
> > > 
> > > If the case you describe above is a good one, then someone eventually
> > > will add back the unregister path. Which should come with a good reason
> > > and with an actual user of the code..
> > 
> > The case I describe above is one where the user of the code doesn't
> > necessarily have the ability to add back the unregister path. 
> 
> I'm not sure I understand your example.. Your saying a situation where
> the kernel can't modified and reloaded, and the hardware clocks aren't
> fully implemented in code yet?

Right. Distro kernels. 

> > Old distro kernels can be difficult to make changes to when new hardware
> > is later released, so being able to just backport a module, compile and
> > load it to get a unexpectedly strange new bit of hardware to work with
> > an older distro kernel seems valuable enough to keep the code around to
> > me.
> 
> You can just as easily back port the code as a built in, and reload the
> kernel right? Why would it need to be a module?

Again, distro kernels. Users can't rebuild them without possibly losing
the support they've paid for, and often recompiling them can cause 3rd
party drivers to fail to work (some distros preserve kernel ABI
stability between minor releases). Waiting 6 months or two years for the
next release where everything is fixed upstream isn't going to make
users happy.

Now, with most hardware vendors implementing decent HPET/ACPI PM
counters, maybe this case is more me reacting to a bad situation I had
to deal with in the past then what we can realistically expect in the
future. But given hardware designers like to break assumptions to
squeeze out performance or features, I'd suspect there will be future
situations where having some extra flexibility would be valuable.

Imaginary example: broken BIOS has incorrect HPET freq and the TSCs are
not in sync. Savvy IT dude finds the problem, copies the HPET driver,
names it hpet-fix and hard codes the proper HPET freq in. Sets the
rating higher then HPET, builds it as a module and loads it on the
affected hardware.

Suddenly there's a solution where otherwise none might be possible.

thanks
-john

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

[Martin Schwidefsky]

On Fri, 31 Jul 2009 01:12:59 -0700
john stultz <johnstul@us.ibm.com> wrote:

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

Re: [RFC][patch 00/12] clocksource / timekeeping rework V2

[Daniel Walker]

On Fri, 2009-07-31 at 01:34 -0700, john stultz wrote:

> 
> Again, distro kernels. Users can't rebuild them without possibly losing
> the support they've paid for, and often recompiling them can cause 3rd
> party drivers to fail to work (some distros preserve kernel ABI
> stability between minor releases). Waiting 6 months or two years for the
> next release where everything is fixed upstream isn't going to make
> users happy.

It wouldn't need to be a module. The distro would update the kernel as
needed.. Just the act of loading an unauthorized kernel module would
potentially invalidate any distro support someone might get.. Distro's
typically provide backported fixes also.

> Now, with most hardware vendors implementing decent HPET/ACPI PM
> counters, maybe this case is more me reacting to a bad situation I had
> to deal with in the past then what we can realistically expect in the
> future. But given hardware designers like to break assumptions to
> squeeze out performance or features, I'd suspect there will be future
> situations where having some extra flexibility would be valuable.
> 
> Imaginary example: broken BIOS has incorrect HPET freq and the TSCs are
> not in sync. Savvy IT dude finds the problem, copies the HPET driver,
> names it hpet-fix and hard codes the proper HPET freq in. Sets the
> rating higher then HPET, builds it as a module and loads it on the
> affected hardware.

The IT guy more than likely would need to rebuild the kernel multiple
times to discover what the problem was .. In the end the distro would
push a fix for this to mainline, and provide a new kernel for the distro
users with a backported fix.

If there a potential for a clocksource to have some type of issue like
what you describe for the HPET, wouldn't it be easier to have all those
as tunable boot args or sysfs options .. 

Daniel

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

[john stultz]

On Fri, 2009-07-31 at 11:00 +0200, Martin Schwidefsky wrote:
> On Fri, 31 Jul 2009 01:12:59 -0700
> john stultz <johnstul@us.ibm.com> wrote:
> 
> > On Fri, 2009-07-31 at 09:52 +0200, Martin Schwidefsky wrote:
> > > On Thu, 30 Jul 2009 15:04:48 -0700
> > > john stultz <johnstul@us.ibm.com> wrote:
> > > 
> > > > On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > > > > plain text document attachment (timekeeper-mult.diff)
> > > > > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > > > 
> > > > > 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@elte.hu>
> > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > Cc: john stultz <johnstul@us.ibm.com>
> > > > > Cc: Daniel Walker <dwalker@fifo99.com>
> > > > > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > > > ---
> > > > >  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

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

[Martin Schwidefsky]

On Fri, 31 Jul 2009 16:32:58 -0700
john stultz <johnstul@us.ibm.com> wrote:

> On Fri, 2009-07-31 at 11:00 +0200, Martin Schwidefsky wrote:
> > 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.

Acuumulation interval is more generic but it IS the NTP interval, no?
Do we plan to ever use something different than the 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;
> > };
> 
> Other then that it looks good to me!

Ok, I'll update the patches.

-- 
blue skies,
   Martin.

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

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

[Linus Walleij]

2009/7/29 Martin Schwidefsky <schwidefsky@de.ibm.com>:

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

Doesn't the overall effect of your patch mean that the above can simply be
replaced by

ret = cyc2ns(&clocksource_32k, ret);

Or clocksource_cyc2ns() if that was renamed (I don't have the entire
picture here...)

>        return ret;
>  }

Yours,
Linus Walleij

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

[Martin Schwidefsky]

On Thu, 13 Aug 2009 13:15:06 +0200
Linus Walleij <linus.ml.walleij@gmail.com> wrote:

> 2009/7/29 Martin Schwidefsky <schwidefsky@de.ibm.com>:
> 
> > 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;
> 
> Doesn't the overall effect of your patch mean that the above can simply be
> replaced by
> 
> ret = cyc2ns(&clocksource_32k, ret);
> 
> Or clocksource_cyc2ns() if that was renamed (I don't have the entire
> picture here...)
> 
> >        return ret;
> >  }

Yes, you can do that. More specifically it will be

  clocksource_cyc2ns(ret, clocksource_32k.mult, clocksource_32k.shift)

after all patches are applied.

-- 
blue skies,
   Martin.

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