[permalink] [raw]

Subject: Re: [PATCH 2/2] x86 mce: use new printk recursion disabling interface

On Mon, May 28, 2012 at 10:07:59AM +0800, ShuoX Liu wrote:
> Boris,
> I checked code and found some other functions in do_machine_check() also
> would printk something. Such as add_taint(). So i think we'd better
> place the recursion check at the beginning and the end of
> do_machine_check(). Also more printks later(maybe) added will benefit
> from this. Do you agree?

I'm not sure we want to disable printk recursion for add_taint() - it
doesn't spit out any useful information wrt MCE so we could ignore it.

Btw, I forgot to ask: this printk recursion disabling, do you have a
real usecase where you don't get the MCE info in dmesg and with your
patch it works or is this purely hypothetical?

Thanks.

--
Regards/Gruss,
Boris.

2012-05-31 00:29:38

by Yanmin Zhang

[permalink] [raw]

Subject: Re: [PATCH 2/2] x86 mce: use new printk recursion disabling interface

On Wed, 2012-05-30 at 11:08 +0200, Borislav Petkov wrote:
> On Mon, May 28, 2012 at 10:07:59AM +0800, ShuoX Liu wrote:
> > Boris,
> > I checked code and found some other functions in do_machine_check() also
> > would printk something. Such as add_taint(). So i think we'd better
> > place the recursion check at the beginning and the end of
> > do_machine_check(). Also more printks later(maybe) added will benefit
> > from this. Do you agree?
>
> I'm not sure we want to disable printk recursion for add_taint() - it
> doesn't spit out any useful information wrt MCE so we could ignore it.
add_taint might be not a good case here. We could move the recursion check
flag setting around mce_panic.

>
> Btw, I forgot to ask: this printk recursion disabling, do you have a
> real usecase where you don't get the MCE info in dmesg and with your
> patch it works or is this purely hypothetical?
We hit it when running a MTBF testing on a Android atom mobile.

Thanks,
Yanmin

2012-06-04 03:07:08

by Liu ShuoX

[permalink] [raw]

2012-06-05 09:57:42

by Liu ShuoX

[permalink] [raw]

Subject: [PATCH v5 2/2] x86 mce: use new printk recursion disabling interface

From: ShuoX Liu <[email protected]>

On x86 machines, some times MCE happens just when kernel calls printk
to output some log info to serial console, while usually MCE module in
kernel is used to print out some hardware error information, such like
bad cache or bad memory bank. That causes printk recursion and printk
would omit MCE printk output.

We hit it when running MTBF testing on Android ATOM mobiles.

Here in mce_panic, we choose to disable printk recursion to make sure
MCE logs printed out.

Signed-off-by: Yanmin Zhang <[email protected]>
Signed-off-by: ShuoX Liu <[email protected]>
---
arch/x86/kernel/cpu/mcheck/mce.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 2afcbd2..906e838 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -306,6 +306,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
{
int i, apei_err = 0;

+ printk_recursion_check_disable();
if (!fake_panic) {
/*
* Make sure only one CPU runs in machine check panic
@@ -360,6 +361,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
panic(msg);
} else
pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg);
+ printk_recursion_check_enable();
}

/* Support code for software error injection */
--
1.7.1

2012-06-05 15:15:47

by Borislav Petkov

[permalink] [raw]

Subject: Re: [PATCH v5 2/2] x86 mce: use new printk recursion disabling interface

On Tue, Jun 05, 2012 at 05:55:22PM +0800, ShuoX Liu wrote:
> From: ShuoX Liu <[email protected]>
>
> On x86 machines, some times MCE happens just when kernel calls printk
> to output some log info to serial console, while usually MCE module in
> kernel is used to print out some hardware error information, such like
> bad cache or bad memory bank. That causes printk recursion and printk
> would omit MCE printk output.
>
> We hit it when running MTBF testing on Android ATOM mobiles.
>
> Here in mce_panic, we choose to disable printk recursion to make sure
> MCE logs printed out.
>
> Signed-off-by: Yanmin Zhang <[email protected]>
> Signed-off-by: ShuoX Liu <[email protected]>
> ---
> arch/x86/kernel/cpu/mcheck/mce.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 2afcbd2..906e838 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -306,6 +306,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
> {
> int i, apei_err = 0;
>
> + printk_recursion_check_disable();
> if (!fake_panic) {
> /*
> * Make sure only one CPU runs in machine check panic
> @@ -360,6 +361,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
> panic(msg);
> } else
> pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg);
> + printk_recursion_check_enable();

Ok, let me ask this again: why not disable the printk recursion check in
the function that actually _prints_ the MCE, i.e. print_mce() instead of
here in mce_panic() which does a whole bunch of other stuff and it can
also return without printing any MCE to dmesg?

Are you interested in seeing the printk's from mce_panic? Why?

Thanks.

--
Regards/Gruss,
Boris.

2012-06-06 00:35:01

by Yanmin Zhang

[permalink] [raw]

Subject: Re: [PATCH v5 2/2] x86 mce: use new printk recursion disabling interface

On Tue, 2012-06-05 at 17:15 +0200, Borislav Petkov wrote:
> On Tue, Jun 05, 2012 at 05:55:22PM +0800, ShuoX Liu wrote:
> > From: ShuoX Liu <[email protected]>
> >
> > On x86 machines, some times MCE happens just when kernel calls printk
> > to output some log info to serial console, while usually MCE module in
> > kernel is used to print out some hardware error information, such like
> > bad cache or bad memory bank. That causes printk recursion and printk
> > would omit MCE printk output.
> >
> > We hit it when running MTBF testing on Android ATOM mobiles.
> >
> > Here in mce_panic, we choose to disable printk recursion to make sure
> > MCE logs printed out.
> >
> > Signed-off-by: Yanmin Zhang <[email protected]>
> > Signed-off-by: ShuoX Liu <[email protected]>
> > ---
> > arch/x86/kernel/cpu/mcheck/mce.c | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > index 2afcbd2..906e838 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > @@ -306,6 +306,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
> > {
> > int i, apei_err = 0;
> >
> > + printk_recursion_check_disable();
> > if (!fake_panic) {
> > /*
> > * Make sure only one CPU runs in machine check panic
> > @@ -360,6 +361,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
> > panic(msg);
> > } else
> > pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg);
> > + printk_recursion_check_enable();
>
> Ok, let me ask this again: why not disable the printk recursion check in
> the function that actually _prints_ the MCE, i.e. print_mce() instead of
> here in mce_panic() which does a whole bunch of other stuff and it can
> also return without printing any MCE to dmesg?
Sorry for forgetting the return checking.

> Are you interested in seeing the printk's from mce_panic? Why?
How about moving the disabling checking in print_mce? It seems not clean if we disable
the printk recursion checking just around every printk statement.

2012-06-06 08:33:50

by Liu ShuoX

[permalink] [raw]

On Fri, Jun 22, 2012 at 04:41:43PM -0700, Andrew Morton wrote:
> A couple of things here.
>
> a) mce_panic() has a "return" statement deep inside. So we return
> from mce_panic() with the recursion check disabled. whoops.

Yeah, you're staring at v4 and we fixed this later (currently discussing
v7, please follow this thread :))

> b) adding a nice comment is nice.

Agreed, if the actual use case pans out and we really need this
interface...

--
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551