clean up FIXME in do_timer_interrupt

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 <[email protected]>

arch/i386/kernel/time.c renamed from arch/i386/kernel/time.c.orig +0 -4

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

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.

Quoted 2 lines · Andrew Morton

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

arch/i386/kernel/time.c +5 -10

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 6 lines · Lee Revell

Quoted 2 lines · Andrew Morton

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.

Quoted 3 lines

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?

Quoted 9 lines · George Anzinger

Quoted 7 lines · Lee Revell

Quoted 2 lines · Andrew Morton

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 7 lines · Lee Revell

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.

Quoted 3 lines

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.

Quoted 13 lines · George Anzinger

Quoted 10 lines · Lee Revell

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.

Quoted 2 lines

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

Quoted 16 lines · Lee Revell

Quoted 14 lines · George Anzinger

Quoted 10 lines · Lee Revell

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

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

Lee

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.

Quoted 6 lines · George Anzinger

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?

Quoted 9 lines · Lee Revell

Quoted 6 lines · George Anzinger

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.

Quoted 1 line

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

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

Quoted 9 lines · Lee Revell

Quoted 6 lines · George Anzinger

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.

Quoted 1 line

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

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

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

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 <[email protected]>

  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;

Quoted 1 line · George Anzinger

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

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

Quoted 4 lines

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 :(

Quoted 22 lines

Signed-off-by George Anzinger <[email protected]>

 time.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6.12-rc/arch/i386/kernel/time.c
===================================================================

arch/i386/kernel/time.c +2 -2

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

arch/i386/kernel/time.c static int set_rtc_mmss(unsigned long no · line 187

Quoted 2 lines

arch/i386/kernel/time.c +0 -0

 	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()

arch/i386/kernel/time.c static void sync_cmos_clock(unsigned lon · line 344

Quoted 11 lines

arch/i386/kernel/time.c +1 -1

@@ -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?