2005-03-03 17:13:40

by Lee Revell

[permalink] [raw]
Subject: [PATCH] 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]>

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



2005-03-04 00:53:35

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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

2005-03-04 01:26:06

by Lee Revell

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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


2005-03-04 10:31:30

by George Anzinger

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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

--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/

2005-03-04 20:52:01

by Lee Revell

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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





2005-03-04 22:18:16

by George Anzinger

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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 [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/

2005-03-08 20:30:26

by Lee Revell

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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

2005-03-08 23:32:06

by George Anzinger

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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 [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/

2005-03-10 08:42:56

by George Anzinger

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

Source: MontaVista Software, Inc. George Anzinger [email protected]
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 [email protected]

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


Attachments:
cmos-time-fix.patch (4.29 kB)

2005-03-11 22:29:24

by Lee Revell

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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

2005-03-11 22:43:41

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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

2005-03-12 02:01:11

by George Anzinger

[permalink] [raw]
Subject: Re: [PATCH] clean up FIXME in do_timer_interrupt

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


Attachments:
cmos_time_lock.patch (1.18 kB)

2005-03-19 09:33:35

by George Anzinger

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

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



Attachments:
cmos_time_lock.patch (1.18 kB)

2005-03-19 21:22:10

by Andrew Morton

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

George Anzinger <[email protected]> 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 <[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;
> }

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?

2005-03-19 22:24:10

by George Anzinger

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

Andrew Morton wrote:
> George Anzinger <[email protected]> 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 <[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;
>> }
>
>
> 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 [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/