Re: [PATCH] clean up FIXME in do_timer_interrupt

[George Anzinger]

Ok, here is a patch.  See what you think.  This patch assumes that Lee's patch 
has been merged (although it eliminates all of it).

George

George Anzinger wrote:
> Lee Revell wrote:
>
> > On Fri, 2005-03-04 at 12:58 -0800, George Anzinger wrote:
> >
> > > Lee Revell wrote:
> > >
> > > > On Fri, 2005-03-04 at 02:28 -0800, George Anzinger wrote:
> > > > The thing that brought this code to my attention is that with 
> > > > PREEMPT_RT
> > > > this happens to be the longest non-preemptible code path in the kernel.
> > > > On my 1.3 Ghz machine set_rtc_mmss takes about 50 usecs, combined with
> > > > the rest of timer irq we end up disabling preemption for about 90 
> > > > usecs.
> > > > Unfortunately I don't have the trace anymore.
> > > >
> > > > Anyway the upshot is if we hung this off a timer it looks like we would
> > > > improve the worst case latency with PREEMPT_RT by almost 50%.  Unless
> > > > there is some reason it has to be done synchronously of course.
> > >
> > >
> > > Well, it does have to be done at the right WRT the second, but I 
> > > suspect we can hit that as well with a timer as it is hit now.  Also, 
> > > if we are _really_ off the mark, this can be defered till the next 
> > > second.
> >
> >
> > Do you have a patch?
>
>
> Not at the moment, but I will work one up.
>
> > Andrew merged my trivial patch to clean up the logic, but a real fix
> > would be better.
> >
> > Lee

--
George Anzinger   george@xxxxxxxxxx
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Source: MontaVista Software, Inc. George Anzinger george@xxxxxxxxxx
Type:  Enhancement 
Disposition: pending
Description:

This patch changes the update of the cmos clock to be timer driven
rather than poll driven by the timer interrupt function.  If the clock
is not being synced to an outside source the timer is removed and thus
system overhead is nill in that case.  The update frequency is still ~11
minutes and missing the update window still causes a retry in 60
seconds.

signed off by George Anzinger george@xxxxxxxxxx

 arch/i386/kernel/time.c |   67 +++++++++++++++++++++++++++++++++---------------
 kernel/time.c           |    9 ++++++
 2 files changed, 56 insertions(+), 20 deletions(-)

Index: linux-2.6.12-rc/arch/i386/kernel/time.c
===================================================================
--- linux-2.6.12-rc.orig/arch/i386/kernel/time.c
+++ linux-2.6.12-rc/arch/i386/kernel/time.c
@@ -186,8 +186,6 @@ static int set_rtc_mmss(unsigned long no
 	return retval;
 }
 
-/* last time the cmos clock got updated */
-static long last_rtc_update;
 
 int timer_ack;
 
@@ -239,24 +237,6 @@ static inline void do_timer_interrupt(in
 
 	do_timer_interrupt_hook(regs);
 
-	/*
-	 * If we have an externally synchronized Linux clock, then update
-	 * CMOS clock accordingly every ~11 minutes. Set_rtc_mmss() has to be
-	 * called as close as possible to 500 ms before the new second starts.
-	 */
-	if ((time_status & STA_UNSYNC) == 0 &&
-	    xtime.tv_sec > last_rtc_update + 660 &&
-	    (xtime.tv_nsec / 1000)
-			>= USEC_AFTER - ((unsigned) TICK_SIZE) / 2 &&
-	    (xtime.tv_nsec / 1000)
-			<= USEC_BEFORE + ((unsigned) TICK_SIZE) / 2) {
-	        last_rtc_update = xtime.tv_sec;
-		if (efi_enabled) {
-		    if (efi_set_rtc_mmss(xtime.tv_sec))
-			last_rtc_update -= 600;
-		} else if (set_rtc_mmss(xtime.tv_sec))
-			last_rtc_update -= 600;
-	}
 
 	if (MCA_bus) {
 		/* The PS/2 uses level-triggered interrupts.  You can't
@@ -313,7 +293,54 @@ unsigned long get_cmos_time(void)
 
 	return retval;
 }
+static void sync_cmos_clock(unsigned long dummy);
 
+static struct timer_list sync_cmos_timer = 
+                                      TIMER_INITIALIZER(sync_cmos_clock, 0, 0);
+
+static void sync_cmos_clock(unsigned long dummy)
+{
+	struct timeval now, next;
+	int fail = 1;
+	/*
+	 * If we have an externally synchronized Linux clock, then update
+	 * CMOS clock accordingly every ~11 minutes. Set_rtc_mmss() has to be
+	 * called as close as possible to 500 ms before the new second starts.
+	 * This code is run on a timer.  If the clock is set, that timer
+	 * may not expire at the correct time.  Thus, we adjust...
+	 */
+	if ((time_status & STA_UNSYNC) != 0)
+		/*
+		 * Not synced, exit, do not restart a timer (if one is 
+		 * running, let it run out).
+		 */
+		return;
+
+	do_gettimeofday(&now);
+	if (now.tv_usec >= USEC_AFTER - ((unsigned) TICK_SIZE) / 2 &&
+	    now.tv_usec <= USEC_BEFORE + ((unsigned) TICK_SIZE) / 2) {
+		fail = set_rtc_mmss(now.tv_sec);
+	}
+	next.tv_usec = USEC_AFTER - now.tv_usec;
+	if (next.tv_usec <= 0)
+		next.tv_usec += USEC_PER_SEC;
+	if (!fail) {
+		next.tv_sec = 659;
+	} else {
+		next.tv_sec = 0;
+	}
+	if (next.tv_usec >= USEC_PER_SEC) {
+		next.tv_sec++;
+		next.tv_usec -= USEC_PER_SEC;
+	}
+	sync_cmos_timer.expires = jiffies + timeval_to_jiffies(&next);
+	add_timer(&sync_cmos_timer);
+		
+}
+void notify_arch_cmos_timer(void)
+{
+	sync_cmos_clock(0);
+}
 static long clock_cmos_diff, sleep_start;
 
 static int timer_suspend(struct sys_device *dev, u32 state)
Index: linux-2.6.12-rc/kernel/time.c
===================================================================
--- linux-2.6.12-rc.orig/kernel/time.c
+++ linux-2.6.12-rc/kernel/time.c
@@ -215,6 +215,14 @@ long pps_stbcnt;		/* stability limit exc
 /* hook for a loadable hardpps kernel module */
 void (*hardpps_ptr)(struct timeval *);
 
+/* we call this to notify the arch when the clock is being
+ * controlled.  If no such arch routine, do nothing.
+ */
+void __attribute__ ((weak)) notify_arch_cmos_timer(void)
+{
+	return;
+}
+
 /* adjtimex mainly allows reading (and writing, if superuser) of
  * kernel time-keeping variables. used by xntpd.
  */
@@ -398,6 +406,7 @@ leave:	if ((time_status & (STA_UNSYNC|ST
 	txc->stbcnt	   = pps_stbcnt;
 	write_sequnlock_irq(&xtime_lock);
 	do_gettimeofday(&txc->time);
+	notify_arch_cmos_timer();
 	return(result);
 }