mce_timer must be passed to setup_timer() in all cases, no matter
whether it is going to be actually used. Otherwise, when the CPU gets
brought down, its call to del_timer_sync() will never return, as the
timer won't have a base associated, and hence lock_timer_base() will
loop infinitely.
(Patch applying to -tip is attached.)
Signed-off-by: Jan Beulich <[email protected]>
Cc: <[email protected]>
---
arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-2.6.32-rc8/arch/x86/kernel/cpu/mcheck/mce.c
+++ 2.6.32-rc8-x86-mce-setup-timer-always/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1374,13 +1374,14 @@ static void mce_init_timer(void)
struct timer_list *t = &__get_cpu_var(mce_timer);
int *n = &__get_cpu_var(mce_next_interval);
+ setup_timer(t, mcheck_timer, smp_processor_id());
+
if (mce_ignore_ce)
return;
*n = check_interval * HZ;
if (!*n)
return;
- setup_timer(t, mcheck_timer, smp_processor_id());
t->expires = round_jiffies(jiffies + *n);
add_timer_on(t, smp_processor_id());
}
Jan Beulich wrote:
> mce_timer must be passed to setup_timer() in all cases, no matter
> whether it is going to be actually used. Otherwise, when the CPU gets
> brought down, its call to del_timer_sync() will never return, as the
> timer won't have a base associated, and hence lock_timer_base() will
> loop infinitely.
No, what we need to fix is hotplug callbacks.
So correct fix should be like "del/add timer conditionally when hotplug."
Thanks,
H.Seto
> (Patch applying to -tip is attached.)
>
> Signed-off-by: Jan Beulich <[email protected]>
> Cc: <[email protected]>
>
> ---
> arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- linux-2.6.32-rc8/arch/x86/kernel/cpu/mcheck/mce.c
> +++ 2.6.32-rc8-x86-mce-setup-timer-always/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1374,13 +1374,14 @@ static void mce_init_timer(void)
> struct timer_list *t = &__get_cpu_var(mce_timer);
> int *n = &__get_cpu_var(mce_next_interval);
>
> + setup_timer(t, mcheck_timer, smp_processor_id());
> +
> if (mce_ignore_ce)
> return;
>
> *n = check_interval * HZ;
> if (!*n)
> return;
> - setup_timer(t, mcheck_timer, smp_processor_id());
> t->expires = round_jiffies(jiffies + *n);
> add_timer_on(t, smp_processor_id());
> }
>>> Hidetoshi Seto <[email protected]> 02.12.09 09:47 >>>
>Jan Beulich wrote:
>> mce_timer must be passed to setup_timer() in all cases, no matter
>> whether it is going to be actually used. Otherwise, when the CPU gets
>> brought down, its call to del_timer_sync() will never return, as the
>> timer won't have a base associated, and hence lock_timer_base() will
>> loop infinitely.
>
>No, what we need to fix is hotplug callbacks.
>So correct fix should be like "del/add timer conditionally when hotplug."
Why? This makes the logic just more complicated (you'd need to track
whether the timer was ever setup or added), and I can't see any
non-tolerable side effect of calling setup_timer() without ever adding
the timer anywhere.
Jan
Jan Beulich wrote:
>>>> Hidetoshi Seto <[email protected]> 02.12.09 09:47 >>>
>> Jan Beulich wrote:
>>> mce_timer must be passed to setup_timer() in all cases, no matter
>>> whether it is going to be actually used. Otherwise, when the CPU gets
>>> brought down, its call to del_timer_sync() will never return, as the
>>> timer won't have a base associated, and hence lock_timer_base() will
>>> loop infinitely.
>> No, what we need to fix is hotplug callbacks.
>> So correct fix should be like "del/add timer conditionally when hotplug."
>
> Why? This makes the logic just more complicated (you'd need to track
> whether the timer was ever setup or added), and I can't see any
> non-tolerable side effect of calling setup_timer() without ever adding
> the timer anywhere.
Ah, sorry, I mistook your patch.
It seems that I just found an another bug here...
Reviewed-by: Hidetoshi Seto <[email protected]>
Thanks,
H.Seto
Ingo, Peter,
Could you pick his patch up?
I confirmed that now it can be applied on Linus's tree and
also tip:x86/urgent.
Thanks,
H.Seto
===
From: Jan Beulich <[email protected]>
Subject: [PATCH] x86/mce: timer must be setup unconditionally
mce_timer must be passed to setup_timer() in all cases, no matter
whether it is going to be actually used. Otherwise, when the CPU gets
brought down, its call to del_timer_sync() will never return, as the
timer won't have a base associated, and hence lock_timer_base() will
loop infinitely.
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Hidetoshi Seto <[email protected]>
Cc: <[email protected]>
---
arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index d7ebf25..a96e5cd 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1388,13 +1388,14 @@ static void __mcheck_cpu_init_timer(void)
struct timer_list *t = &__get_cpu_var(mce_timer);
int *n = &__get_cpu_var(mce_next_interval);
+ setup_timer(t, mce_start_timer, smp_processor_id());
+
if (mce_ignore_ce)
return;
*n = check_interval * HZ;
if (!*n)
return;
- setup_timer(t, mce_start_timer, smp_processor_id());
t->expires = round_jiffies(jiffies + *n);
add_timer_on(t, smp_processor_id());
}
--
1.6.5.3
* Hidetoshi Seto <[email protected]> wrote:
> Ingo, Peter,
>
> Could you pick his patch up?
> I confirmed that now it can be applied on Linus's tree and
> also tip:x86/urgent.
Sure. Can i add your signoff too?
Ingo
Ingo Molnar wrote:
> * Hidetoshi Seto <[email protected]> wrote:
>
>> Ingo, Peter,
>>
>> Could you pick his patch up?
>> I confirmed that now it can be applied on Linus's tree and
>> also tip:x86/urgent.
>
> Sure. Can i add your signoff too?
Of course yes.
Signed-off-by: Hidetoshi Seto <[email protected]>
Thanks,
H.Seto
On 12/07/2009 07:14 PM, Ingo Molnar wrote:
>
> * Hidetoshi Seto <[email protected]> wrote:
>
>> Ingo, Peter,
>>
>> Could you pick his patch up?
>> I confirmed that now it can be applied on Linus's tree and
>> also tip:x86/urgent.
>
> Sure. Can i add your signoff too?
>
Oops... for some reason I thought it already had been applied.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
Commit-ID: bc09effabf0c5c6c7021e5ef9af15a23579b32a8
Gitweb: http://git.kernel.org/tip/bc09effabf0c5c6c7021e5ef9af15a23579b32a8
Author: Jan Beulich <[email protected]>
AuthorDate: Tue, 8 Dec 2009 11:21:37 +0900
Committer: Ingo Molnar <[email protected]>
CommitDate: Tue, 8 Dec 2009 05:34:39 +0100
x86/mce: Set up timer unconditionally
mce_timer must be passed to setup_timer() in all cases, no
matter whether it is going to be actually used. Otherwise, when
the CPU gets brought down, its call to del_timer_sync() will
never return, as the timer won't have a base associated, and
hence lock_timer_base() will loop infinitely.
Signed-off-by: Jan Beulich <[email protected]>
Signed-off-by: Hidetoshi Seto <[email protected]>
Cc: <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index d7ebf25..a96e5cd 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1388,13 +1388,14 @@ static void __mcheck_cpu_init_timer(void)
struct timer_list *t = &__get_cpu_var(mce_timer);
int *n = &__get_cpu_var(mce_next_interval);
+ setup_timer(t, mce_start_timer, smp_processor_id());
+
if (mce_ignore_ce)
return;
*n = check_interval * HZ;
if (!*n)
return;
- setup_timer(t, mce_start_timer, smp_processor_id());
t->expires = round_jiffies(jiffies + *n);
add_timer_on(t, smp_processor_id());
}