The spin_lock_debug/rcu_cpu_stall detector uses
trigger_all_cpu_backtrace() to dump cpu backtrace.
Therefore it is possible that trigger_all_cpu_backtrace()
could be called at the same time on different CPUs, which
triggers and 'unknown reason NMI' warning. The following case
illustrates the problem:
CPU1 CPU2 ... CPU N
trigger_all_cpu_backtrace()
set "backtrace_mask" to cpu mask
|
generate NMI interrupts generate NMI interrupts ...
\ | /
\ | /
The "backtrace_mask" will be cleaned by the first NMI interrupt
at nmi_watchdog_tick(), then the following NMI interrupts generated
by other cpus's arch_trigger_all_cpu_backtrace() will be took as
unknown reason NMI interrupts.
This patch uses a lock to avoid the problem, and stop the
arch_trigger_all_cpu_backtrace() calling to avoid dumping double cpu
backtrace info when there is already a trigger_all_cpu_backtrace()
in progress.
Signed-off-by: Dongdong Deng <[email protected]>
Reviewed-by: Bruce Ashfield <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: [email protected]
CC: [email protected]
Signed-off-by: Don Zickus <[email protected]>
---
arch/x86/kernel/apic/hw_nmi.c | 23 +++++++++++++++++++++++
arch/x86/kernel/apic/nmi.c | 23 +++++++++++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index cefd694..bfdab3b 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -26,9 +26,27 @@ u64 hw_nmi_get_sample_period(void)
}
#ifdef ARCH_HAS_NMI_WATCHDOG
+/* "in progress" flag of arch_trigger_all_cpu_backtrace */
+static unsigned long backtrace_flag;
+
void arch_trigger_all_cpu_backtrace(void)
{
int i;
+ unsigned long flags;
+
+ /*
+ * Have to disable irq here, as the
+ * arch_trigger_all_cpu_backtrace() could be
+ * triggered by "spin_lock()" with irqs on.
+ */
+ local_irq_save(flags);
+
+ if (test_and_set_bit(0, &backtrace_flag))
+ /*
+ * If there is already a trigger_all_cpu_backtrace() in progress
+ * (backtrace_flag == 1), don't output double cpu dump infos.
+ */
+ goto out_restore_irq;
cpumask_copy(to_cpumask(backtrace_mask), cpu_online_mask);
@@ -41,6 +59,11 @@ void arch_trigger_all_cpu_backtrace(void)
break;
mdelay(1);
}
+
+ clear_bit(0, &backtrace_flag);
+
+out_restore_irq:
+ local_irq_restore(flags);
}
static int __kprobes
diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
index c90041c..bd9cb79 100644
--- a/arch/x86/kernel/apic/nmi.c
+++ b/arch/x86/kernel/apic/nmi.c
@@ -549,9 +549,27 @@ int do_nmi_callback(struct pt_regs *regs, int cpu)
return 0;
}
+/* "in progress" flag of arch_trigger_all_cpu_backtrace */
+static unsigned long backtrace_flag;
+
void arch_trigger_all_cpu_backtrace(void)
{
int i;
+ unsigned long flags;
+
+ /*
+ * Have to disable irq here, as the
+ * arch_trigger_all_cpu_backtrace() could be
+ * triggered by "spin_lock()" with irqs on.
+ */
+ local_irq_save(flags);
+
+ if (test_and_set_bit(0, &backtrace_flag))
+ /*
+ * If there is already a trigger_all_cpu_backtrace() in progress
+ * (backtrace_flag == 1), don't output double cpu dump infos.
+ */
+ goto out_restore_irq;
cpumask_copy(to_cpumask(backtrace_mask), cpu_online_mask);
@@ -564,4 +582,9 @@ void arch_trigger_all_cpu_backtrace(void)
break;
mdelay(1);
}
+
+ clear_bit(0, &backtrace_flag);
+
+out_restore_irq:
+ local_irq_restore(flags);
}
--
1.6.0.4
Hm, another thing i noticed is that there's two of these:
> #ifdef ARCH_HAS_NMI_WATCHDOG
> +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
> +static unsigned long backtrace_flag;
> +
> void arch_trigger_all_cpu_backtrace(void)
> {
> int i;
> + unsigned long flags;
> +
> + /*
> + * Have to disable irq here, as the
> + * arch_trigger_all_cpu_backtrace() could be
> + * triggered by "spin_lock()" with irqs on.
> + */
> + local_irq_save(flags);
> +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
> +static unsigned long backtrace_flag;
> +
> void arch_trigger_all_cpu_backtrace(void)
> {
> int i;
> + unsigned long flags;
> +
> + /*
> + * Have to disable irq here, as the
> + * arch_trigger_all_cpu_backtrace() could be
> + * triggered by "spin_lock()" with irqs on.
> + */
> + local_irq_save(flags);
> +
> + if (test_and_set_bit(0, &backtrace_flag))
A fair amount of code is being duplicated in two places - which is not nice. Lets
try to create a shared facility instead?
Thanks,
Ingo
Ingo Molnar wrote:
> Hm, another thing i noticed is that there's two of these:
>
>> #ifdef ARCH_HAS_NMI_WATCHDOG
>> +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
>> +static unsigned long backtrace_flag;
>> +
>> void arch_trigger_all_cpu_backtrace(void)
>> {
>> int i;
>> + unsigned long flags;
>> +
>> + /*
>> + * Have to disable irq here, as the
>> + * arch_trigger_all_cpu_backtrace() could be
>> + * triggered by "spin_lock()" with irqs on.
>> + */
>> + local_irq_save(flags);
>
>> +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
>> +static unsigned long backtrace_flag;
>> +
>> void arch_trigger_all_cpu_backtrace(void)
>> {
>> int i;
>> + unsigned long flags;
>> +
>> + /*
>> + * Have to disable irq here, as the
>> + * arch_trigger_all_cpu_backtrace() could be
>> + * triggered by "spin_lock()" with irqs on.
>> + */
>> + local_irq_save(flags);
>> +
>> + if (test_and_set_bit(0, &backtrace_flag))
>
> A fair amount of code is being duplicated in two places - which is not nice. Lets
> try to create a shared facility instead?
Yep, It is a good idea, I will try to do that. :-)
Dongdong
>
> Thanks,
>
> Ingo
>
On Thu, Nov 11, 2010 at 7:23 PM, DDD <[email protected]> wrote:
> Ingo Molnar wrote:
>>
>> Hm, another thing i noticed is that there's two of these:
>>
>>> #ifdef ARCH_HAS_NMI_WATCHDOG
>>> +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
>>> +static unsigned long backtrace_flag;
>>> +
>>> void arch_trigger_all_cpu_backtrace(void)
>>> {
>>> int i;
>>> + unsigned long flags;
>>> +
>>> + /*
>>> + * Have to disable irq here, as the
>>> + * arch_trigger_all_cpu_backtrace() could be
>>> + * triggered by "spin_lock()" with irqs on.
>>> + */
>>> + local_irq_save(flags);
>>
>>> +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
>>> +static unsigned long backtrace_flag;
>>> +
>>> void arch_trigger_all_cpu_backtrace(void)
>>> {
>>> int i;
>>> + unsigned long flags;
>>> +
>>> + /*
>>> + * Have to disable irq here, as the
>>> + * arch_trigger_all_cpu_backtrace() could be
>>> + * triggered by "spin_lock()" with irqs on.
>>> + */
>>> + local_irq_save(flags);
>>> +
>>> + if (test_and_set_bit(0, &backtrace_flag))
>>
>> A fair amount of code is being duplicated in two places - which is not
>> nice. Lets try to create a shared facility instead?
>
> Yep, It is a good idea, I will try to do that. :-)
Hmm, it is hard to create a shared facility for
arch_trigger_all_cpu_backtrace().
The arch_trigger_all_cpu_backtrace() need a local variable(backtrace_mask)
which was used by the other difference functions in nmi.c and hw_nmi.c.
If we want to create a shared facility for arch_trigger_all_cpu_backtrace(),
we have to create an independent .c file for the single one function
arch_trigger_all_cpu_backtrace(). I think it is worse than before...
Maybe Don could say some things here as he is the owner of hw_nmi.c. :-)
Thanks,
Dongdong
On Thu, Nov 11, 2010 at 12:17:02PM +0100, Ingo Molnar wrote:
>
> Hm, another thing i noticed is that there's two of these:
I guess it depends how we want to deprecate the old nmi watchdog.
Dongdong was just patching the old and new code.
There are 3 ways we can go here.
- rip out the 'arch_trigger_all_cpu_backtrace' from the old nmi_watchdog,
rework the Makefile and have both watchdogs use common code in hw_nmi.c
- patch both places
- rely on the fact that the old watchdog doesn't really work any more and
only patch the new watchdog
I can put together a patch to do the first one, then have Dongdong put his
patch on top. Selfishly I wouldn't mind just doing the third option. ;-)
Thoughts?
Cheers,
Don
>
> > #ifdef ARCH_HAS_NMI_WATCHDOG
> > +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
> > +static unsigned long backtrace_flag;
> > +
> > void arch_trigger_all_cpu_backtrace(void)
> > {
> > int i;
> > + unsigned long flags;
> > +
> > + /*
> > + * Have to disable irq here, as the
> > + * arch_trigger_all_cpu_backtrace() could be
> > + * triggered by "spin_lock()" with irqs on.
> > + */
> > + local_irq_save(flags);
>
> > +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
> > +static unsigned long backtrace_flag;
> > +
> > void arch_trigger_all_cpu_backtrace(void)
> > {
> > int i;
> > + unsigned long flags;
> > +
> > + /*
> > + * Have to disable irq here, as the
> > + * arch_trigger_all_cpu_backtrace() could be
> > + * triggered by "spin_lock()" with irqs on.
> > + */
> > + local_irq_save(flags);
> > +
> > + if (test_and_set_bit(0, &backtrace_flag))
>
> A fair amount of code is being duplicated in two places - which is not nice. Lets
> try to create a shared facility instead?
>
> Thanks,
>
> Ingo
* Don Zickus <[email protected]> wrote:
> On Thu, Nov 11, 2010 at 12:17:02PM +0100, Ingo Molnar wrote:
> >
> > Hm, another thing i noticed is that there's two of these:
>
> I guess it depends how we want to deprecate the old nmi watchdog.
ah, indeed, i forgot about the old watchdog.
Lets zap it right now and see who complains? Mind sending a patch for that? (please
also pick up Dongdong's patch)
Thanks,
Ingo
Dongdong Deng wrote:
> The spin_lock_debug/rcu_cpu_stall detector uses
> trigger_all_cpu_backtrace() to dump cpu backtrace.
> Therefore it is possible that trigger_all_cpu_backtrace()
> could be called at the same time on different CPUs, which
> triggers and 'unknown reason NMI' warning. The following case
> illustrates the problem:
>
> CPU1 CPU2 ... CPU N
> trigger_all_cpu_backtrace()
> set "backtrace_mask" to cpu mask
> |
> generate NMI interrupts generate NMI interrupts ...
> \ | /
> \ | /
> The "backtrace_mask" will be cleaned by the first NMI interrupt
> at nmi_watchdog_tick(), then the following NMI interrupts generated
> by other cpus's arch_trigger_all_cpu_backtrace() will be took as
> unknown reason NMI interrupts.
>
> This patch uses a lock to avoid the problem, and stop the
> arch_trigger_all_cpu_backtrace() calling to avoid dumping double cpu
> backtrace info when there is already a trigger_all_cpu_backtrace()
> in progress.
>
> Signed-off-by: Dongdong Deng <[email protected]>
> Reviewed-by: Bruce Ashfield <[email protected]>
> CC: Thomas Gleixner <[email protected]>
> CC: Ingo Molnar <[email protected]>
> CC: "H. Peter Anvin" <[email protected]>
> CC: [email protected]
> CC: [email protected]
> Signed-off-by: Don Zickus <[email protected]>
> ---
> arch/x86/kernel/apic/hw_nmi.c | 23 +++++++++++++++++++++++
> arch/x86/kernel/apic/nmi.c | 23 +++++++++++++++++++++++
> 2 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
> index cefd694..bfdab3b 100644
> --- a/arch/x86/kernel/apic/hw_nmi.c
> +++ b/arch/x86/kernel/apic/hw_nmi.c
> @@ -26,9 +26,27 @@ u64 hw_nmi_get_sample_period(void)
> }
>
> #ifdef ARCH_HAS_NMI_WATCHDOG
> +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
> +static unsigned long backtrace_flag;
> +
> void arch_trigger_all_cpu_backtrace(void)
> {
> int i;
> + unsigned long flags;
> +
> + /*
> + * Have to disable irq here, as the
> + * arch_trigger_all_cpu_backtrace() could be
> + * triggered by "spin_lock()" with irqs on.
> + */
> + local_irq_save(flags);
> +
> + if (test_and_set_bit(0, &backtrace_flag))
> + /*
> + * If there is already a trigger_all_cpu_backtrace() in progress
> + * (backtrace_flag == 1), don't output double cpu dump infos.
> + */
> + goto out_restore_irq;
>
> cpumask_copy(to_cpumask(backtrace_mask), cpu_online_mask);
>
> @@ -41,6 +59,11 @@ void arch_trigger_all_cpu_backtrace(void)
> break;
> mdelay(1);
> }
> +
> + clear_bit(0, &backtrace_flag);
Hi Don,
The clear_bit() on x86 (arch/x86/include/asm/bitops.h) does not
contain a memory barrier, Could you add a memory barrier ops here when
you pick up this patch?
+ clear_bit(0, &backtrace_flag);
+ smp_mb__after_clear_bit();
Thanks,
Dongdong
> +
> +out_restore_irq:
> + local_irq_restore(flags);
> }
>
> static int __kprobes
> diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
> index c90041c..bd9cb79 100644
> --- a/arch/x86/kernel/apic/nmi.c
> +++ b/arch/x86/kernel/apic/nmi.c
> @@ -549,9 +549,27 @@ int do_nmi_callback(struct pt_regs *regs, int cpu)
> return 0;
> }
>
> +/* "in progress" flag of arch_trigger_all_cpu_backtrace */
> +static unsigned long backtrace_flag;
> +
> void arch_trigger_all_cpu_backtrace(void)
> {
> int i;
> + unsigned long flags;
> +
> + /*
> + * Have to disable irq here, as the
> + * arch_trigger_all_cpu_backtrace() could be
> + * triggered by "spin_lock()" with irqs on.
> + */
> + local_irq_save(flags);
> +
> + if (test_and_set_bit(0, &backtrace_flag))
> + /*
> + * If there is already a trigger_all_cpu_backtrace() in progress
> + * (backtrace_flag == 1), don't output double cpu dump infos.
> + */
> + goto out_restore_irq;
>
> cpumask_copy(to_cpumask(backtrace_mask), cpu_online_mask);
>
> @@ -564,4 +582,9 @@ void arch_trigger_all_cpu_backtrace(void)
> break;
> mdelay(1);
> }
> +
> + clear_bit(0, &backtrace_flag);
> +
> +out_restore_irq:
> + local_irq_restore(flags);
> }
On Fri, Nov 12, 2010 at 10:38:13AM +0800, DDD wrote:
> Dongdong Deng wrote:
> >The spin_lock_debug/rcu_cpu_stall detector uses
> >trigger_all_cpu_backtrace() to dump cpu backtrace.
> >Therefore it is possible that trigger_all_cpu_backtrace()
> >could be called at the same time on different CPUs, which
> >triggers and 'unknown reason NMI' warning. The following case
> >illustrates the problem:
> >
> > CPU1 CPU2 ... CPU N
> > trigger_all_cpu_backtrace()
> > set "backtrace_mask" to cpu mask
> > |
> >generate NMI interrupts generate NMI interrupts ...
> > \ | /
> > \ | /
> > The "backtrace_mask" will be cleaned by the first NMI interrupt
> > at nmi_watchdog_tick(), then the following NMI interrupts generated
> >by other cpus's arch_trigger_all_cpu_backtrace() will be took as
> >unknown reason NMI interrupts.
> >
> >This patch uses a lock to avoid the problem, and stop the
> >arch_trigger_all_cpu_backtrace() calling to avoid dumping double cpu
> >backtrace info when there is already a trigger_all_cpu_backtrace()
> >in progress.
> >
> >Signed-off-by: Dongdong Deng <[email protected]>
> >Reviewed-by: Bruce Ashfield <[email protected]>
> >CC: Thomas Gleixner <[email protected]>
> >CC: Ingo Molnar <[email protected]>
> >CC: "H. Peter Anvin" <[email protected]>
> >CC: [email protected]
> >CC: [email protected]
> >Signed-off-by: Don Zickus <[email protected]>
> >---
> > arch/x86/kernel/apic/hw_nmi.c | 23 +++++++++++++++++++++++
> > arch/x86/kernel/apic/nmi.c | 23 +++++++++++++++++++++++
> > 2 files changed, 46 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
> >index cefd694..bfdab3b 100644
> >--- a/arch/x86/kernel/apic/hw_nmi.c
> >+++ b/arch/x86/kernel/apic/hw_nmi.c
> >@@ -26,9 +26,27 @@ u64 hw_nmi_get_sample_period(void)
> > }
> > #ifdef ARCH_HAS_NMI_WATCHDOG
> >+/* "in progress" flag of arch_trigger_all_cpu_backtrace */
> >+static unsigned long backtrace_flag;
> >+
> > void arch_trigger_all_cpu_backtrace(void)
> > {
> > int i;
> >+ unsigned long flags;
> >+
> >+ /*
> >+ * Have to disable irq here, as the
> >+ * arch_trigger_all_cpu_backtrace() could be
> >+ * triggered by "spin_lock()" with irqs on.
> >+ */
> >+ local_irq_save(flags);
> >+
> >+ if (test_and_set_bit(0, &backtrace_flag))
> >+ /*
> >+ * If there is already a trigger_all_cpu_backtrace() in progress
> >+ * (backtrace_flag == 1), don't output double cpu dump infos.
> >+ */
> >+ goto out_restore_irq;
> > cpumask_copy(to_cpumask(backtrace_mask), cpu_online_mask);
> >@@ -41,6 +59,11 @@ void arch_trigger_all_cpu_backtrace(void)
> > break;
> > mdelay(1);
> > }
> >+
> >+ clear_bit(0, &backtrace_flag);
>
> Hi Don,
>
> The clear_bit() on x86 (arch/x86/include/asm/bitops.h) does not
> contain a memory barrier, Could you add a memory barrier ops here
> when you pick up this patch?
>
> + clear_bit(0, &backtrace_flag);
> + smp_mb__after_clear_bit();
>
>
Ok. Thanks.
Cheers,
Don