Re: [PATCH] clean up FIXME in do_timer_interrupt-lock fix
Andrew Morton wrote:
George Anzinger 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
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(_lock);
+ spin_lock_irq(_lock);
if (efi_enabled)
retval = efi_set_rtc_mmss(nowtime);
else
retval = mach_set_rtc_mmss(nowtime);
- spin_unlock(_lock);
+ spin_unlock_irq(_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(_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/
-
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/
Re: [PATCH] clean up FIXME in do_timer_interrupt-lock fix
George Anzinger 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
>
> 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(_lock);
> +spin_lock_irq(_lock);
> if (efi_enabled)
> retval = efi_set_rtc_mmss(nowtime);
> else
> retval = mach_set_rtc_mmss(nowtime);
> -spin_unlock(_lock);
> +spin_unlock_irq(_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(_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?
-
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/
Re: [PATCH] clean up FIXME in do_timer_interrupt-lock fix
Did you pick this up? First sent on 3-11.
Andrew Morton wrote:
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.
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/
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
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(_lock);
+ spin_lock_irq(_lock);
if (efi_enabled)
retval = efi_set_rtc_mmss(nowtime);
else
retval = mach_set_rtc_mmss(nowtime);
- spin_unlock(_lock);
+ spin_unlock_irq(_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(_cmos_timer, jiffies + 1);
}
static long clock_cmos_diff, sleep_start;
Re: [PATCH] clean up FIXME in do_timer_interrupt-lock fix
Did you pick this up? First sent on 3-11.
Andrew Morton wrote:
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.
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/
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 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?
-
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/
Re: [PATCH] clean up FIXME in do_timer_interrupt-lock fix
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/
-
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/
