[PATCH] timekeeping: drop irq-context clocksource polling

[PATCH] timekeeping: drop irq-context clocksource polling

[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..

Signed-Off-By: Daniel Walker <dwalker@mvista.com>

---
 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
===================================================================
--- /dev/null
+++ linux-2.6.20/include/linux/timekeeping.h
@@ -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
===================================================================
--- linux-2.6.20.orig/kernel/time/clocksource.c
+++ linux-2.6.20/kernel/time/clocksource.c
@@ -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
===================================================================
--- linux-2.6.20.orig/kernel/time/timekeeping.c
+++ linux-2.6.20/kernel/time/timekeeping.c
@@ -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);
 }
-- 

-- 

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[john stultz]

On Thu, 2007-04-05 at 14:03 -0700, Daniel Walker 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.
> 
> 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 <dwalker@mvista.com>

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.

-john



> ---
>  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
> ===================================================================
> --- /dev/null
> +++ linux-2.6.20/include/linux/timekeeping.h
> @@ -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
> ===================================================================
> --- linux-2.6.20.orig/kernel/time/clocksource.c
> +++ linux-2.6.20/kernel/time/clocksource.c
> @@ -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
> ===================================================================
> --- linux-2.6.20.orig/kernel/time/timekeeping.c
> +++ linux-2.6.20/kernel/time/timekeeping.c
> @@ -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);
>  }
> -- 
> 

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Daniel Walker]

On Thu, 2007-04-05 at 14:25 -0700, john stultz wrote:
> On Thu, 2007-04-05 at 14:03 -0700, Daniel Walker 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.
> > 
> > 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 <dwalker@mvista.com>
> 
> 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?

Daniel

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[john stultz]

On Thu, 2007-04-05 at 14:29 -0700, Daniel Walker wrote:
> On Thu, 2007-04-05 at 14:25 -0700, john stultz wrote:
> > On Thu, 2007-04-05 at 14:03 -0700, Daniel Walker 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.
> > > 
> > > 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 <dwalker@mvista.com>
> > 
> > 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?

Nope. You're ok there. I just missed it and then I tried to cancel my
mail from being sent, but apparently it still got out. whoops! :)

-john

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Andrew Morton]

On Thu, 05 Apr 2007 14:25:19 -0700
john stultz <johnstul@us.ibm.com> wrote:

> On Thu, 2007-04-05 at 14:03 -0700, Daniel Walker 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.
> > 
> > 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 <dwalker@mvista.com>
> 
> 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?

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[john stultz]

On Thu, 2007-04-05 at 14:36 -0700, Andrew Morton wrote:
> On Thu, 05 Apr 2007 14:25:19 -0700
> john stultz <johnstul@us.ibm.com> wrote:
> 
> > On Thu, 2007-04-05 at 14:03 -0700, Daniel Walker 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.
> > > 
> > > 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 <dwalker@mvista.com>
> > 
> > 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?

Yep. Sorry for the confusion, reading too hastily. 

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

Agreed.

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

-john

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Andrew Morton]

On Thu, 05 Apr 2007 14:03:16 -0700 Daniel Walker <dwalker@mvista.com> 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.

Oh well.  If there's a problem I'll catch it.

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Daniel Walker]

On Fri, 2007-04-06 at 18:21 -0700, Andrew Morton wrote:
> On Thu, 05 Apr 2007 14:03:16 -0700 Daniel Walker <dwalker@mvista.com> 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.

Yeah .. I tested 2 or 3 ARM configs that didn't have GENERIC_TIME , but
they failed .. I just assumed it was work in progress from the ARM
tree ..

Daniel

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Andrew Morton]

On Thu, 05 Apr 2007 14:03:16 -0700 Daniel Walker <dwalker@mvista.com> 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.

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Daniel Walker]

On Sat, 2007-04-07 at 03:19 -0700, Andrew Morton wrote:
> On Thu, 05 Apr 2007 14:03:16 -0700 Daniel Walker <dwalker@mvista.com> 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 .

Daniel

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Thomas Gleixner]

On Sat, 2007-04-07 at 10:43 -0700, Daniel Walker wrote:
> 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 .

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

	tglx


	

	

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Daniel Walker]

On Sat, 2007-04-07 at 22:50 +0200, Thomas Gleixner wrote:
> On Sat, 2007-04-07 at 10:43 -0700, Daniel Walker wrote:
> > 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 .
> 
> Simply because it fiddles with variables which are relevant for
> timekeeping.

loops_per_jiffy perhaps?

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Thomas Gleixner]

On Sat, 2007-04-07 at 14:30 -0700, Daniel Walker wrote:
> On Sat, 2007-04-07 at 22:50 +0200, Thomas Gleixner wrote:
> > On Sat, 2007-04-07 at 10:43 -0700, Daniel Walker wrote:
> > > 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 .
> > 
> > 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.

	tglx

Re: [PATCH] timekeeping: drop irq-context clocksource polling

[Daniel Walker]

On Sun, 2007-04-08 at 10:33 +0200, Thomas Gleixner wrote:
> 
> 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.

That's what I was thinking .. However, I wanted to wait for John to
comment on it also ..

Dainel