[PATCH] clean up FIXME in do_timer_interrupt

[PATCH] clean up FIXME in do_timer_interrupt

[Lee Revell]

AFAICT this code is equivalent and cleans up the (efi_)set_rtc_mmss code
referred to as "horrible... FIXME" in the comments.  Completely
untested.

Signed-Off-By: Lee Revell <rlrevell@joe-job.com>

--- linux-2.6.11-rc4/arch/i386/kernel/time.c.orig	2005-03-03 11:52:32.000000000 -0500
+++ linux-2.6.11-rc4/arch/i386/kernel/time.c	2005-03-03 12:02:20.000000000 -0500
@@ -254,16 +254,10 @@
 			>= USEC_AFTER - ((unsigned) TICK_SIZE) / 2 &&
 	    (xtime.tv_nsec / 1000)
 			<= USEC_BEFORE + ((unsigned) TICK_SIZE) / 2) {
-		/* horrible...FIXME */
-		if (efi_enabled) {
-	 		if (efi_set_rtc_mmss(xtime.tv_sec) == 0)
-				last_rtc_update = xtime.tv_sec;
-			else
-				last_rtc_update = xtime.tv_sec - 600;
-		} else if (set_rtc_mmss(xtime.tv_sec) == 0)
-			last_rtc_update = xtime.tv_sec;
-		else
-			last_rtc_update = xtime.tv_sec - 600; /* do it again in 60 s */
+	        last_rtc_update = xtime.tv_sec;
+		if (efi_enabled && efi_set_rtc_mmss(xtime.tv_sec) || 
+			set_rtc_mmss(xtime.tv_sec))
+			last_rtc_update -= 600;
 	}
 
 	if (MCA_bus) {

Re: [PATCH] clean up FIXME in do_timer_interrupt

[Andrew Morton]

Lee Revell <rlrevell@joe-job.com> wrote:
>
> AFAICT this code is equivalent and cleans up the (efi_)set_rtc_mmss code
> referred to as "horrible... FIXME" in the comments.  Completely
> untested.
> 
> Signed-Off-By: Lee Revell <rlrevell@joe-job.com>
> 
> --- linux-2.6.11-rc4/arch/i386/kernel/time.c.orig	2005-03-03 11:52:32.000000000 -0500
> +++ linux-2.6.11-rc4/arch/i386/kernel/time.c	2005-03-03 12:02:20.000000000 -0500
> @@ -254,16 +254,10 @@
>  			>= USEC_AFTER - ((unsigned) TICK_SIZE) / 2 &&
>  	    (xtime.tv_nsec / 1000)
>  			<= USEC_BEFORE + ((unsigned) TICK_SIZE) / 2) {
> -		/* horrible...FIXME */
> -		if (efi_enabled) {
> -	 		if (efi_set_rtc_mmss(xtime.tv_sec) == 0)
> -				last_rtc_update = xtime.tv_sec;
> -			else
> -				last_rtc_update = xtime.tv_sec - 600;
> -		} else if (set_rtc_mmss(xtime.tv_sec) == 0)
> -			last_rtc_update = xtime.tv_sec;
> -		else
> -			last_rtc_update = xtime.tv_sec - 600; /* do it again in 60 s */
> +	        last_rtc_update = xtime.tv_sec;
> +		if (efi_enabled && efi_set_rtc_mmss(xtime.tv_sec) || 
> +			set_rtc_mmss(xtime.tv_sec))
> +			last_rtc_update -= 600;

It's not equivalent.

If efi_enabled is true and efi_set_rtc_mmss(xtime.tv_sec) returns zero, the
new code will run set_rtc_mmss(xtime.tv_sec) whereas the old code won't.

Re: [PATCH] clean up FIXME in do_timer_interrupt

[Lee Revell]

On Thu, 2005-03-03 at 16:45 -0800, Andrew Morton wrote:
> If efi_enabled is true and efi_set_rtc_mmss(xtime.tv_sec) returns zero, the
> new code will run set_rtc_mmss(xtime.tv_sec) whereas the old code won't.

Argh, I should know better then to send patches before having coffee.

Here's a new patch.  Still ugly, but might be a worthwhile cleanup.

Lee

--- linux-2.6.11-rc4-V0.7.39-02/arch/i386/kernel/time.c	2005-02-14 18:10:49.000000000 -0500
+++ linux-2.6.11-rc4/arch/i386/kernel/time.c	2005-03-03 20:15:39.000000000 -0500
@@ -254,16 +254,12 @@
 			>= USEC_AFTER - ((unsigned) TICK_SIZE) / 2 &&
 	    (xtime.tv_nsec / 1000)
 			<= USEC_BEFORE + ((unsigned) TICK_SIZE) / 2) {
-		/* horrible...FIXME */
+	        last_rtc_update = xtime.tv_sec;
 		if (efi_enabled) {
-	 		if (efi_set_rtc_mmss(xtime.tv_sec) == 0)
-				last_rtc_update = xtime.tv_sec;
-			else
-				last_rtc_update = xtime.tv_sec - 600;
-		} else if (set_rtc_mmss(xtime.tv_sec) == 0)
-			last_rtc_update = xtime.tv_sec;
-		else
-			last_rtc_update = xtime.tv_sec - 600; /* do it again in 60 s */
+		    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) {

Re: [PATCH] clean up FIXME in do_timer_interrupt

[George Anzinger]

Lee Revell wrote:
> On Thu, 2005-03-03 at 16:45 -0800, Andrew Morton wrote:
> 
>>If efi_enabled is true and efi_set_rtc_mmss(xtime.tv_sec) returns zero, the
>>new code will run set_rtc_mmss(xtime.tv_sec) whereas the old code won't.
> 
> 
> Argh, I should know better then to send patches before having coffee.
> 
> Here's a new patch.  Still ugly, but might be a worthwhile cleanup.

Lets ask the obvious question: Why isn't this update hung on a timer?  It seems 
silly to check this 6000 times per update.  I am sure we can sync a timer to the 
same degree we do timer interrupts, so there _must_ be some other reason.  Right?

George
> 
> Lee
> 
> --- linux-2.6.11-rc4-V0.7.39-02/arch/i386/kernel/time.c	2005-02-14 18:10:49.000000000 -0500
> +++ linux-2.6.11-rc4/arch/i386/kernel/time.c	2005-03-03 20:15:39.000000000 -0500
> @@ -254,16 +254,12 @@
>  			>= USEC_AFTER - ((unsigned) TICK_SIZE) / 2 &&
>  	    (xtime.tv_nsec / 1000)
>  			<= USEC_BEFORE + ((unsigned) TICK_SIZE) / 2) {
> -		/* horrible...FIXME */
> +	        last_rtc_update = xtime.tv_sec;
>  		if (efi_enabled) {
> -	 		if (efi_set_rtc_mmss(xtime.tv_sec) == 0)
> -				last_rtc_update = xtime.tv_sec;
> -			else
> -				last_rtc_update = xtime.tv_sec - 600;
> -		} else if (set_rtc_mmss(xtime.tv_sec) == 0)
> -			last_rtc_update = xtime.tv_sec;
> -		else
> -			last_rtc_update = xtime.tv_sec - 600; /* do it again in 60 s */
> +		    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) {
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH] clean up FIXME in do_timer_interrupt

[Lee Revell]

On Fri, 2005-03-04 at 02:28 -0800, George Anzinger wrote:
> Lee Revell wrote:
> > On Thu, 2005-03-03 at 16:45 -0800, Andrew Morton wrote:
> > 
> >>If efi_enabled is true and efi_set_rtc_mmss(xtime.tv_sec) returns zero, the
> >>new code will run set_rtc_mmss(xtime.tv_sec) whereas the old code won't.
> > 
> > 
> > Argh, I should know better then to send patches before having coffee.
> > 
> > Here's a new patch.  Still ugly, but might be a worthwhile cleanup.
> 
> Lets ask the obvious question: Why isn't this update hung on a timer?  It seems 
> silly to check this 6000 times per update.  I am sure we can sync a timer to the 
> same degree we do timer interrupts, so there _must_ be some other reason.  Right?
> 

Thanks George, I knew there was an obvious question here, I just didn't
know what it was ;-).

The thin 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.

Lee

Re: [PATCH] clean up FIXME in do_timer_interrupt

[George Anzinger]

Lee Revell wrote:
> On Fri, 2005-03-04 at 02:28 -0800, George Anzinger wrote:
> 
>>Lee Revell wrote:
>>
>>>On Thu, 2005-03-03 at 16:45 -0800, Andrew Morton wrote:
>>>
>>>
>>>>If efi_enabled is true and efi_set_rtc_mmss(xtime.tv_sec) returns zero, the
>>>>new code will run set_rtc_mmss(xtime.tv_sec) whereas the old code won't.
>>>
>>>
>>>Argh, I should know better then to send patches before having coffee.
>>>
>>>Here's a new patch.  Still ugly, but might be a worthwhile cleanup.
>>
>>Lets ask the obvious question: Why isn't this update hung on a timer?  It seems 
>>silly to check this 6000 times per update.  I am sure we can sync a timer to the 
>>same degree we do timer interrupts, so there _must_ be some other reason.  Right?
>>
> 
> 
> Thanks George, I knew there was an obvious question here, I just didn't
> know what it was ;-).
> 
> 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.


-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH] clean up FIXME in do_timer_interrupt

[Lee Revell]

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?

Andrew merged my trivial patch to clean up the logic, but a real fix
would be better.

Lee

Re: [PATCH] clean up FIXME in do_timer_interrupt

[George Anzinger]

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@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH] clean up FIXME in do_timer_interrupt

[George Anzinger]

[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]

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@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

[-- Attachment #2: cmos-time-fix.patch --]
[-- Type: text/plain, Size: 4395 bytes --]

Source: MontaVista Software, Inc. George Anzinger george@mvista.com
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@mvista.com

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

Re: [PATCH] clean up FIXME in do_timer_interrupt

[Lee Revell]

On Thu, 2005-03-10 at 00:42 -0800, George Anzinger wrote:
> 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.

No replies yet.  Are there any objections to this patch?

If not, it should be applied as it reduces the worst case latency of the
-RT kernel from about 90 to 40 usecs on my hardware.

The effect on mainline should be negligible.

Lee

Re: [PATCH] clean up FIXME in do_timer_interrupt

[Andrew Morton]

Lee Revell <rlrevell@joe-job.com> wrote:
>
> On Thu, 2005-03-10 at 00:42 -0800, George Anzinger wrote:
> > 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.
> 
> No replies yet.  Are there any objections to this patch?

Nope.  I think it's neat.  I queued it up.

Re: [PATCH] clean up FIXME in do_timer_interrupt

[George Anzinger]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

Andrew Morton wrote:
> Lee Revell <rlrevell@joe-job.com> wrote:
> 
>>On Thu, 2005-03-10 at 00:42 -0800, George Anzinger wrote:
>>
>>>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.
>>
>>No replies yet.  Are there any objections to this patch?
> 
> 
> Nope.  I think it's neat.  I queued it up.

I had a nightmare about ntp coming in at the "wrong" time resulting in a 
deadlock.  Attached locking changes will make me sleep better :)

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

[-- Attachment #2: cmos_time_lock.patch --]
[-- Type: text/plain, Size: 1204 bytes --]

Source: MontaVista Software, Inc.
Type: Defect Fix 
Disposition: Pending
Description:

I was not happy with the locking on this.  Two changes:
1) Turn off irq while setting the clock.
2) Call the timer code only through the timer interface 
   (set a short timer to do it from the ntp call).

Signed-off-by: George Anzinger <george@mvista.com>

 time.c |    6 +++---
 1 files changed, 3 insertions(+), 3 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
@@ -176,12 +176,12 @@ static int set_rtc_mmss(unsigned long no
 	int retval;
 
 	/* gets recalled with irq locally disabled */
-	spin_lock(&rtc_lock);
+	spin_lock_irq(&rtc_lock);
 	if (efi_enabled)
 		retval = efi_set_rtc_mmss(nowtime);
 	else
 		retval = mach_set_rtc_mmss(nowtime);
-	spin_unlock(&rtc_lock);
+	spin_unlock_irq(&rtc_lock);
 
 	return retval;
 }
@@ -338,7 +338,7 @@ static void sync_cmos_clock(unsigned lon
 }
 void notify_arch_cmos_timer(void)
 {
-	sync_cmos_clock(0);
+	mod_timer(&sync_cmos_timer, jiffies + 1);
 }
 static long clock_cmos_diff, sleep_start;
 

Re: [PATCH] clean up FIXME in do_timer_interrupt-lock fix

[George Anzinger]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

Did you pick this up?  First sent on 3-11.

Andrew Morton wrote:
> Lee Revell <rlrevell@joe-job.com> wrote:
> 
>>On Thu, 2005-03-10 at 00:42 -0800, George Anzinger wrote:
>>
>>>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.
>>
>>No replies yet.  Are there any objections to this patch?
> 
> 
> Nope.  I think it's neat.  I queued it up.

I had a nightmare about ntp coming in at the "wrong" time resulting in a
deadlock.  Attached locking changes will make me sleep better :)

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/


[-- Attachment #2: cmos_time_lock.patch --]
[-- Type: text/plain, Size: 1205 bytes --]

Source: MontaVista Software, Inc.
Type: Defect Fix 
Disposition: Pending
Description:

I was not happy with the locking on this.  Two changes:
1) Turn off irq while setting the clock.
2) Call the timer code only through the timer interface 
   (set a short timer to do it from the ntp call).

Signed-off-by: George Anzinger <george@mvista.com>

 time.c |    6 +++---
 1 files changed, 3 insertions(+), 3 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
@@ -176,12 +176,12 @@ static int set_rtc_mmss(unsigned long no
 	int retval;
 
 	/* gets recalled with irq locally disabled */
-	spin_lock(&rtc_lock);
+	spin_lock_irq(&rtc_lock);
 	if (efi_enabled)
 		retval = efi_set_rtc_mmss(nowtime);
 	else
 		retval = mach_set_rtc_mmss(nowtime);
-	spin_unlock(&rtc_lock);
+	spin_unlock_irq(&rtc_lock);
 
 	return retval;
 }
@@ -338,7 +338,7 @@ static void sync_cmos_clock(unsigned lon
 }
 void notify_arch_cmos_timer(void)
 {
-	sync_cmos_clock(0);
+	mod_timer(&sync_cmos_timer, jiffies + 1);
 }
 static long clock_cmos_diff, sleep_start;
 

Re: [PATCH] clean up FIXME in do_timer_interrupt-lock fix

[Andrew Morton]

George Anzinger <george@mvista.com> wrote:
>
> Did you pick this up?  First sent on 3-11.

I did, although now looking at it I have issues.

>  I was not happy with the locking on this.  Two changes:
>  1) Turn off irq while setting the clock.
>  2) Call the timer code only through the timer interface 
>     (set a short timer to do it from the ntp call).

I would consider this to be an inadequate description :(

>  Signed-off-by: George Anzinger <george@mvista.com>
> 
>   time.c |    6 +++---
>   1 files changed, 3 insertions(+), 3 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
>  @@ -176,12 +176,12 @@ static int set_rtc_mmss(unsigned long no
>   	int retval;
>   
>   	/* gets recalled with irq locally disabled */
>  -	spin_lock(&rtc_lock);
>  +	spin_lock_irq(&rtc_lock);
>   	if (efi_enabled)
>   		retval = efi_set_rtc_mmss(nowtime);
>   	else
>   		retval = mach_set_rtc_mmss(nowtime);
>  -	spin_unlock(&rtc_lock);
>  +	spin_unlock_irq(&rtc_lock);
>   
>   	return retval;
>   }

If the comment is correct, and this code is called with local irq's
disabled then this patch should be using spin_lock_irqsave()

>  @@ -338,7 +338,7 @@ static void sync_cmos_clock(unsigned lon
>   }
>   void notify_arch_cmos_timer(void)
>   {
>  -	sync_cmos_clock(0);
>  +	mod_timer(&sync_cmos_timer, jiffies + 1);
>   }
>   static long clock_cmos_diff, sleep_start;
>   

Your description says what this does, but it doesn't way why it was done?

Re: [PATCH] clean up FIXME in do_timer_interrupt-lock fix

[George Anzinger]

Andrew Morton wrote:
> George Anzinger <george@mvista.com> wrote:
> 
>>Did you pick this up?  First sent on 3-11.
> 
> 
> I did, although now looking at it I have issues.
> 
> 
>> I was not happy with the locking on this.  Two changes:
>> 1) Turn off irq while setting the clock.
>> 2) Call the timer code only through the timer interface 
>>    (set a short timer to do it from the ntp call).
> 
I wanted the calls to sync_cmos_clock() to be made in a consistent environment. 
  This was not true when calling it directly from the NTP call code.  The change 
means that sync_cmos_clock() is ALWAYS called from run_timers(), i.e. as a timer 
call back function.
> 
> I would consider this to be an inadequate description :(
> 
> 
>> Signed-off-by: George Anzinger <george@mvista.com>
>>
>>  time.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 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
>> @@ -176,12 +176,12 @@ static int set_rtc_mmss(unsigned long no
>>  	int retval;
>>  
>>  	/* gets recalled with irq locally disabled */
>> -	spin_lock(&rtc_lock);
>> +	spin_lock_irq(&rtc_lock);
>>  	if (efi_enabled)
>>  		retval = efi_set_rtc_mmss(nowtime);
>>  	else
>>  		retval = mach_set_rtc_mmss(nowtime);
>> -	spin_unlock(&rtc_lock);
>> +	spin_unlock_irq(&rtc_lock);
>>  
>>  	return retval;
>>  }
> 
> 
> If the comment is correct, and this code is called with local irq's
> disabled then this patch should be using spin_lock_irqsave()

With the change below, it is always called from the timer call back code which, 
I believe, is always called with irq on.  Looks like I missed the comment :(
> 
> 
>> @@ -338,7 +338,7 @@ static void sync_cmos_clock(unsigned lon
>>  }
>>  void notify_arch_cmos_timer(void)
>>  {
>> -	sync_cmos_clock(0);
>> +	mod_timer(&sync_cmos_timer, jiffies + 1);
>>  }
>>  static long clock_cmos_diff, sleep_start;
>>  
> 
> 
> Your description says what this does, but it doesn't way why it was done?
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/