timekeeping: drop irq-context clocksource polling

Before this change the timekeeping code would poll the clocksource list every interrupt. This changes that so the clocksource list is only checked when there has been and update, and no longer checks in interrupt context.

This also has a few small space and line cleanups.

Boot tested on i386, compile tested on x86_64 .. However, I couldn't find a !GENERIC_TIME that compiled without this change so it's untested..

Signed-off-by Daniel Walker <[email protected]>

---
 include/linux/timekeeping.h |   10 ++++++++++
 kernel/time/clocksource.c   |    7 +++++++
 kernel/time/timekeeping.c   |   17 ++++++++---------
 3 files changed, 25 insertions(+), 9 deletions(-)

Index: linux-2.6.20/include/linux/timekeeping.h
===================================================================

include/linux/timekeeping.h new file +10 -0

@@ -0,0 +1,10 @@
+#ifndef _LINUX_TIMEKEEPING_H
+#define _LINUX_TIMEKEEPING_H
+
+#ifdef CONFIG_GENERIC_TIME
+extern void timekeeping_change_clocksource(void);
+#else
+static inline void timekeeping_change_clocksource(void) { }
+#endif
+
+#endif

Index: linux-2.6.20/kernel/time/clocksource.c
===================================================================

kernel/time/clocksource.c +7 -0

@@ -30,6 +30,7 @@
 #include <linux/module.h>
 #include <linux/sched.h> /* for spin_unlock_irq() using preempt_count() m68k */
 #include <linux/tick.h>
+#include <linux/timekeeping.h>
 
 /* XXX - Would like a better way for initializing curr_clocksource */
 extern struct clocksource clocksource_jiffies;

@@ -279,6 +280,8 @@ int clocksource_register(struct clocksou
 
 	clocksource_check_watchdog(c);
 
+	timekeeping_change_clocksource();
+
 	return 0;
 }
 EXPORT_SYMBOL(clocksource_register);

@@ -297,6 +300,8 @@ void clocksource_change_rating(struct cl
 	clocksource_enqueue(cs);
 	next_clocksource = select_clocksource();
 	spin_unlock_irqrestore(&clocksource_lock, flags);
+
+	timekeeping_change_clocksource();
 }
 
 #ifdef CONFIG_SYSFS

@@ -374,6 +379,8 @@ static ssize_t sysfs_override_clocksourc
 
 	spin_unlock_irq(&clocksource_lock);
 
+	timekeeping_change_clocksource();
+
 	return ret;
 }
 

Index: linux-2.6.20/kernel/time/timekeeping.c
===================================================================

kernel/time/timekeeping.c +8 -9

@@ -18,7 +18,7 @@
 #include <linux/jiffies.h>
 #include <linux/time.h>
 #include <linux/tick.h>
-
+#include <linux/timekeeping.h>
 
 /*
  * This read-write spinlock protects us from races in SMP while

@@ -42,10 +42,8 @@ struct timespec wall_to_monotonic __attr
 
 EXPORT_SYMBOL(xtime);
 
-
 static struct clocksource *clock; /* pointer to current clocksource */
 
-
 #ifdef CONFIG_GENERIC_TIME
 /**
  * __get_nsec_offset - Returns nanoseconds since last call to periodic_hook

@@ -168,9 +166,10 @@ EXPORT_SYMBOL(do_settimeofday);
  *
  * Accumulates current time interval and initializes new clocksource
  */
-static void change_clocksource(void)
+void timekeeping_change_clocksource(void)
 {
 	struct clocksource *new;
+	unsigned long flags;
 	cycle_t now;
 	u64 nsec;
 

@@ -179,8 +178,10 @@ static void change_clocksource(void)
 	if (clock == new)
 		return;
 
+	write_seqlock_irqsave(&xtime_lock, flags);
+
 	now = clocksource_read(new);
-	nsec =  __get_nsec_offset();
+	nsec = __get_nsec_offset();
 	timespec_add_ns(&xtime, nsec);
 
 	clock = new;

@@ -192,11 +193,11 @@ static void change_clocksource(void)
 
 	tick_clock_notify();
 
+	write_sequnlock_irqrestore(&xtime_lock, flags);
+
 	printk(KERN_INFO "Time: %s clocksource has been installed.\n",
 	       clock->name);
 }
-#else
-static inline void change_clocksource(void) { }
 #endif
 
 /**

@@ -470,7 +471,5 @@ void update_wall_time(void)
 	xtime.tv_nsec = (s64)clock->xtime_nsec >> clock->shift;
 	clock->xtime_nsec -= (s64)xtime.tv_nsec << clock->shift;
 
-	/* check to see if there is a new clocksource to use */
-	change_clocksource();
 	update_vsyscall(&xtime, clock);
 }

Quoted 7 lines · Daniel Walker

Before this change the timekeeping code would poll the clocksource list every interrupt. This changes that so the clocksource list is only checked when there has been and update, and no longer checks in interrupt context.

This also has a few small space and line cleanups.

Quoted 4 lines

Boot tested on i386, compile tested on x86_64 .. However, I couldn't find a !GENERIC_TIME that compiled without this change so it's untested..

Signed-off-by Daniel Walker <[email protected]>

Err.. I think you need to be holding a write on the xtime_lock (as is done before calling update_wall_time()) when changing the clocksource.

Quoted 11 lines · john stultz

Quoted 10 lines · Daniel Walker

Before this change the timekeeping code would poll the clocksource list every interrupt. This changes that so the clocksource list is only checked when there has been and update, and no longer checks in interrupt context.

This also has a few small space and line cleanups.

Boot tested on i386, compile tested on x86_64 .. However, I couldn't find a !GENERIC_TIME that compiled without this change so it's untested..

Quoted 4 lines

Quoted 1 line

Signed-off-by Daniel Walker <[email protected]>

Err.. I think you need to be holding a write on the xtime_lock (as is done before calling update_wall_time()) when changing the clocksource.

I added a write_seqlock_irqsave() on xtime_lock in change_clocksource() .. Did I need more (different) protection than that?

Quoted 7 lines · Daniel Walker

Before this change the timekeeping code would poll the clocksource list every interrupt. This changes that so the clocksource list is only checked when there has been and update, and no longer checks in interrupt context.

This also has a few small space and line cleanups.

Quoted 4 lines

Boot tested on i386, compile tested on x86_64 .. However, I couldn't find a !GENERIC_TIME that compiled without this change so it's untested..

Signed-off-by Daniel Walker <[email protected]>

Err.. I think you need to be holding a write on the xtime_lock (as is done before calling update_wall_time()) when changing the clocksource.

Quoted 11 lines · On Thu, 05 Apr 2007 14:25:19 -0700 john stultz <[email protected]> wrote:

Quoted 10 lines · Daniel Walker

Before this change the timekeeping code would poll the clocksource list every interrupt. This changes that so the clocksource list is only checked when there has been and update, and no longer checks in interrupt context.

This also has a few small space and line cleanups.

Boot tested on i386, compile tested on x86_64 .. However, I couldn't find a !GENERIC_TIME that compiled without this change so it's untested..

Quoted 4 lines

Quoted 1 line

Signed-off-by Daniel Walker <[email protected]>

Err.. I think you need to be holding a write on the xtime_lock (as is done before calling update_wall_time()) when changing the clocksource.

The patch does add the appropriate locking to change_clocksource(), doesn't it?

It looks like a good change to me - we avoid taking the kernel-wide clocksource_lock every tick?

Boot tested on i386, compile tested on x86_64 .. However, I couldn't find a !GENERIC_TIME that compiled without this change so it's untested..

Quoted 2 lines · On Thu, 05 Apr 2007 14:03:16 -0700 Daniel Walker <[email protected]> wrote:

Boot tested on i386, compile tested on x86_64 .. However, I couldn't find a !GENERIC_TIME that compiled without this change so it's untested..

I'm scratching my head over this statement.

Do you mean that none of grep -lv GENERIC_TIME arch/*/Kconfig compile? Confused.

Before this change the timekeeping code would poll the clocksource list every interrupt. This changes that so the clocksource list is only checked when there has been and update, and no longer checks in interrupt context.

Quoted 4 lines · On Thu, 05 Apr 2007 14:03:16 -0700 Daniel Walker <[email protected]> wrote:

Before this change the timekeeping code would poll the clocksource list every interrupt. This changes that so the clocksource list is only checked when there has been and update, and no longer checks in interrupt context.

I get a complete lockup on i386 SMP - before the kernel has printed anything.

I'm suspecting a recursive taking of xtime_lock.

Looks like this path ,

arch/i386/kernel/tsc.c: time_cpufreq_notifier(); <-- takes xtime_lock

			 mark_tsc_unstable();
            		  clocksource_change_rating(&clocksource_tsc, 0);
			   timekeeping_change_clocksource(); <-- takes xtime_lock

I'm not sure why the time_cpufreq_notifier is taking the xtime_lock tho .

Quoted 8 lines · Daniel Walker

Looks like this path ,

arch/i386/kernel/tsc.c: time_cpufreq_notifier(); <-- takes xtime_lock

			 mark_tsc_unstable();
            		  clocksource_change_rating(&clocksource_tsc, 0);
			   timekeeping_change_clocksource(); <-- takes xtime_lock

Quoted 1 line

I'm not sure why the time_cpufreq_notifier is taking the xtime_lock tho .

Simply because it fiddles with variables which are relevant for timekeeping.

Quoted 9 lines · Thomas Gleixner

Quoted 8 lines · Daniel Walker

Looks like this path ,

arch/i386/kernel/tsc.c: time_cpufreq_notifier(); <-- takes xtime_lock

			 mark_tsc_unstable();
            		  clocksource_change_rating(&clocksource_tsc, 0);
			   timekeeping_change_clocksource(); <-- takes xtime_lock

Quoted 4 lines

Quoted 1 line

I'm not sure why the time_cpufreq_notifier is taking the xtime_lock tho .

Simply because it fiddles with variables which are relevant for timekeeping.

loops_per_jiffy perhaps?

Oh well, this is a leftover from the days where we tried to use TSC despite of frequency changes. It still modifies the scale factor of the tsc clocksource.

I agree that it can be removed as we switch off TSC anyway in that case.