: mce: don't print "human readable" message for corrected errors

Don't display the "human readable" warning for correctable errors in mce. There is no need for this information to be displayed.

Signed-off-by Prarit Bhargava <[email protected]>

arch/x86/kernel/cpu/mcheck/mce.c +9 -4

index 450a366..8024013 100644
@@ -100,10 +100,16 @@ ATOMIC_NOTIFIER_HEAD(x86_mce_decoder_chain);
 EXPORT_SYMBOL_GPL(x86_mce_decoder_chain);
 
 static int default_decode_mce(struct notifier_block *nb, unsigned long val,
-			       void *data)
+			      void *data)
 {
-	pr_emerg(HW_ERR "No human readable MCE decoding support on this CPU type.\n");
-	pr_emerg(HW_ERR "Run the message through 'mcelog --ascii' to decode.\n");
+	struct mce *m = (struct mce *)data;
+
+	if (m->status & MCI_STATUS_UC) {
+		pr_emerg(HW_ERR "No human readable MCE decoding support "
+			 "on this CPU type.\n");
+		pr_emerg(HW_ERR "Run the message through 'mcelog --ascii' "
+			 "to decode.\n");
+	}
 
 	return NOTIFY_STOP;
 }
--

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/

Quoted 5 lines · Prarit Bhargava

Don't display the "human readable" warning for correctable errors in mce. There is no need for this information to be displayed.

Signed-off-by Prarit Bhargava <[email protected]>

Why not? This way you turn reporting of _ALL_ correctable MCEs completely off and some users would actually like to run them through mcelog on Intel.

Quoted 11 lines · Borislav Petkov

Quoted 5 lines · Prarit Bhargava

Don't display the "human readable" warning for correctable errors in mce. There is no need for this information to be displayed.

Signed-off-by Prarit Bhargava <[email protected]>

Why not? This way you turn reporting of _ALL_ correctable MCEs completely off and some users would actually like to run them through mcelog on Intel.

I may be reading the code wrong ... but in the case of the default_decode_mce() callback there is *no* additional output displayed for correctable MCEs -- so the message is AFAICT useless.

ie) all you see on the console log is

[Hardware Error]: Machine check events logged

^^^ this notifies the user that something happened with MCE

[Hardware Error]: No human readable MCE decoding support on this CPU type.

^^^ this is purely informational and unnecessary

[Hardware Error]: Run the message through 'mcelog --ascii' to decode.

^^^ this makes absolutely no sense. There is no message to decode.

You won't see this in the case of the amd-edac because it has it's own callback.

Why not? This way you turn reporting of _ALL_ correctable MCEs completely off and some users would actually like to run them through mcelog on Intel.

We are also setting TAINT_MACHINE_CHECK for corrected errors - perhaps this made sense when systems were small and machine checks were rare and scary. But I think we need to start working with the reality that corrected errors are normal events.

From a OS point of view, we would like to know that there is flaky HW on

-Tony

Quoted 7 lines

We are also setting TAINT_MACHINE_CHECK for corrected errors - perhaps this made sense when systems were small and machine checks were rare and scary. But I think we need to start working with the reality that corrected errors are normal events.

It still makes sense for small lt 1TB systems, IMO. But ... maybe a flag of some sort is necessary to stop the TAINTing for systems larger than that. The CEs may point to something going wrong on a system. CEs in theory become UCEs eventually, right?

From a OS point of view, we would like to know that there is flaky HW on the system.

/me knows this is going to turn into a PFA discussion in 4 ... 3 ... 2
.... ;)

Yeah, there's the TAINT thing too, good point Tony. Well, we definitely don't want to get tainted for correctable errors - they're too "normal" to do so, IMHO.

I'm thinking remove the TAINT for CEs and don't call the default notifier if it is the only notifier call registered. Maybe something like

	if (num_notifiers(&x86_mce_decoder_chain) > 1)
		atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, &m);

or since the notifiers are priority sorted, don't call notifiers with -1 prio.

Or something to that effect.

Quoted 6 lines

I'm thinking remove the TAINT for CEs and don't call the default notifier if it is the only notifier call registered. Maybe something like

	if (num_notifiers(&x86_mce_decoder_chain) > 1)
		atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, &m);

Quoted 4 lines

or since the notifiers are priority sorted, don't call notifiers with -1 prio.

Or something to that effect.

What is the point of having a default notifier if it doesn't get called?

Any consideration of adding thresholding (ie only log the first X number of corrected errors) as is done on IA64? (see arch/ia64/kernel/mca.c)

So maybe we could drop the default notifier and do in print_mce():

	if (notifier_chain_empty(&x86_mce_decoder_chain))
		pr_emerg(HW_ERR "Run the message through 'mcelog --ascii' to decode.\n");

I think this could work, let me cook up something.

From: Borislav Petkov <[email protected]>

The default notifier doesn't make a lot of sense to call in the correctable errors case. Drop it and emit the mcelog decoding hint only in the uncorrectable errors case and when no notifier is registered.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/include/asm/mce.h       |    4 ++--
 arch/x86/kernel/cpu/mcheck/mce.c |   26 +++++++++-----------------
 2 files changed, 11 insertions(+), 19 deletions(-)

(quoted diff) +4 -1

+	} else
+		atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
 }
 
 #define PANIC_TIMEOUT 5 /* 5 seconds */

@@ -589,7 +582,8 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		 */
 		if (!(flags & MCP_DONTLOG) && !mce_dont_log_ce) {
 			mce_log(&m);
-			atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, &m);
+			if (atomic_read(&mce_decoders))
+				atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, &m);
 		}
 
 		/*

Quoted 13 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

The default notifier doesn't make a lot of sense to call in the correctable errors case. Drop it and emit the mcelog decoding hint only in the uncorrectable errors case and when no notifier is registered.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/include/asm/mce.h       |    4 ++--
 arch/x86/kernel/cpu/mcheck/mce.c |   26 +++++++++-----------------
 2 files changed, 11 insertions(+), 19 deletions(-)

Review

Quoted 2 lines

(quoted diff) +2 -0

+extern atomic_t mce_decoders;
+

Boris,

I don't think we need to do this. I think we can use the existing notifier chain tools to do this check for us ... *untested and uncompiled* patch below.

* Print out human-readable details about the MCE error,

Review

Quoted 6 lines

(quoted diff) +3 -1

 	 * (if the CPU has an implementation for that)
 	 */
-	atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	if (!atomic_read(&mce_decoders)) {
+		pr_emerg(HW_ERR "No human readable MCE decoding support on this CPU type.\n");
+		pr_emerg(HW_ERR "Run the above through 'mcelog --ascii' to decode.\n");

I thought we didn't want these lines at all for CE errors?

Quoted 17 lines

arch/x86/kernel/cpu/mcheck/mce.c +4 -1

+	} else
+		atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
 }
 
 #define PANIC_TIMEOUT 5 /* 5 seconds */

@@ -589,7 +582,8 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		 */
 		if (!(flags & MCP_DONTLOG) && !mce_dont_log_ce) {
 			mce_log(&m);
-			atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, &m);
+			if (atomic_read(&mce_decoders))
+				atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, &m);
 		}
 
 		/*

arch/x86/kernel/cpu/mcheck/mce.c __setup("mce", mcheck_enable); · line 1720

Quoted 8 lines

arch/x86/kernel/cpu/mcheck/mce.c +0 -2

@@ -1721,8 +1715,6 @@ __setup("mce", mcheck_enable);
 
 int __init mcheck_init(void)
 {
-	atomic_notifier_chain_register(&x86_mce_decoder_chain, &mce_dec_nb);
-
 	mcheck_intel_therm_init();
 
 	return 0;

Again, this patch has not been tested or compiled against latest upstream. I
have quickly tested it against RHEL6 (2.6.32,33,34,35,36,38,39 based) and
confirmed that I don't see the messages for CEs. I have NOT fully tested to see what happens when I add in a "dummy" notifier to see if the messages are not printed.

arch/x86/kernel/cpu/mcheck/mce.c +4 -15

index 3385ea2..b10a1f4 100644
@@ -100,25 +100,12 @@ static int			cpu_missing;
 
 /*
  * CPU/chipset specific EDAC code can register a notifier call here to print
- * MCE errors in a human-readable form.
+ * MCE errors in a human-readable form.  Notifiers must return NOTIFY_STOP
+ * upon completion.
  */
 ATOMIC_NOTIFIER_HEAD(x86_mce_decoder_chain);
 EXPORT_SYMBOL_GPL(x86_mce_decoder_chain);
 
-static int default_decode_mce(struct notifier_block *nb, unsigned long val,
-			       void *data)
-{
-	pr_emerg(HW_ERR "No human readable MCE decoding support on this CPU type.\n");
-	pr_emerg(HW_ERR "Run the message through 'mcelog --ascii' to decode.\n");
-
-	return NOTIFY_STOP;
-}
-
-static struct notifier_block mce_dec_nb = {
-	.notifier_call = default_decode_mce,
-	.priority      = -1,
-};
-
 /* MCA banks polled by the period polling timer for corrected events */
 DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = {
 	[0 ... BITS_TO_LONGS(MAX_NR_BANKS)-1] = ~0UL

@@ -212,6 +199,8 @@ void mce_log(struct mce *mce)
 
 static void print_mce(struct mce *m)
 {
+	int ret;
+
 	pr_emerg(HW_ERR "CPU %d: Machine Check Exception: %Lx Bank %d: %016Lx\n",
 	       m->extcpu, m->mcgstatus, m->bank, m->status);
 

@@ -236,10 +225,13 @@ static void print_mce(struct mce *m)
 		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid);
 
 	/*

Quoted 26 lines · Prarit Bhargava

Quoted 16 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

The default notifier doesn't make a lot of sense to call in the correctable errors case. Drop it and emit the mcelog decoding hint only in the uncorrectable errors case and when no notifier is registered.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/include/asm/mce.h       |    4 ++--
 arch/x86/kernel/cpu/mcheck/mce.c |   26 +++++++++-----------------
 2 files changed, 11 insertions(+), 19 deletions(-)

Review

Quoted 2 lines

(quoted diff) +2 -0

+extern atomic_t mce_decoders;
+

Boris,

I don't think we need to do this. I think we can use the existing notifier chain tools to do this check for us ... *untested and uncompiled* patch below.

* Print out human-readable details about the MCE error,

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +3 -1

 	 * (if the CPU has an implementation for that)
 	 */
-	atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	if (!atomic_read(&mce_decoders)) {
+		pr_emerg(HW_ERR "No human readable MCE decoding support on this CPU type.\n");
+		pr_emerg(HW_ERR "Run the above through 'mcelog --ascii' to decode.\n");

I thought we didn't want these lines at all for CE errors?

This is the UC only case - we call print_mce() before we panic.

Quoted 37 lines · Borislav Petkov

Quoted 35 lines · Prarit Bhargava

Quoted 16 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

The default notifier doesn't make a lot of sense to call in the correctable errors case. Drop it and emit the mcelog decoding hint only in the uncorrectable errors case and when no notifier is registered.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/include/asm/mce.h       |    4 ++--
 arch/x86/kernel/cpu/mcheck/mce.c |   26 +++++++++-----------------
 2 files changed, 11 insertions(+), 19 deletions(-)

Review

Quoted 2 lines

(quoted diff) +2 -0

+extern atomic_t mce_decoders;
+

Boris,

I don't think we need to do this. I think we can use the existing notifier chain tools to do this check for us ... *untested and uncompiled* patch below.

* Print out human-readable details about the MCE error,

Review

Quoted 7 lines

(quoted diff) +3 -1

 	 * (if the CPU has an implementation for that)
 	 */
-	atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	if (!atomic_read(&mce_decoders)) {
+		pr_emerg(HW_ERR "No human readable MCE decoding support on this CPU type.\n");
+		pr_emerg(HW_ERR "Run the above through 'mcelog --ascii' to decode.\n");

I thought we didn't want these lines at all for CE errors?

Quoted 2 lines

This is the UC only case - we call print_mce() before we panic.

.. right ... but we would still see nonsensical messages before the panic, which will confuse the end user. IMO, dump the messages unless it's UC. And we shouldn't print out the human readable message at all.

I'm installing F14 on a system that is known to generate CEs @ boot-time. I'll test patches+latest upstream to here to see what happens...

Quoted 5 lines

.. right ... but we would still see nonsensical messages before the panic, which will confuse the end user. IMO, dump the messages unless it's UC. And we shouldn't print out the human readable message at all.

I'd still leave the "mcelog ... " line though as a hint.

Quoted 5 lines

Quoted 5 lines

.. right ... but we would still see nonsensical messages before the panic, which will confuse the end user. IMO, dump the messages unless it's UC. And we shouldn't print out the human readable message at all.

Quoted 3 lines

I'd still leave the "mcelog ... " line though as a hint.

:)

There are no messages -- that's part of the problem. We're asking a user to do something for a message that doesn't exist ...

Quoted 12 lines · Prarit Bhargava

Quoted 10 lines

Quoted 6 lines

.. right ... but we would still see nonsensical messages before the panic, which will confuse the end user. IMO, dump the messages unless it's UC. And we shouldn't print out the human readable message at all.

I'd still leave the "mcelog ... " line though as a hint.

:)

Quoted 3 lines

There are no messages -- that's part of the problem. We're asking a user to do something for a message that doesn't exist ...

Yes and no - in the UC case you get all the MCA registers dumped in print_mce() - and those you want to decode. In the CE case, we won't be issuing any with the default notifier removed. I just sent you a -v2, I think it should do what we want.

Or am I missing something...?

Thanks.

[Hardware Error]: Run the above through 'mcelog --ascii' to decode.

potentially many, many times. The problem still is that there is no output to decode (in the default case).

Quoted 5 lines

[Hardware Error]: Run the above through 'mcelog --ascii' to decode.

potentially many, many times. The problem still is that there is no output to decode (in the default case).

printk_once()?

arch/x86/kernel/cpu/mcheck/mce.c static void print_mce(struct mce *m) · line 235

Quoted 12 lines

arch/x86/kernel/cpu/mcheck/mce.c +3 -1

@@ -239,7 +227,9 @@ static void print_mce(struct mce *m)
 	 * Print out human-readable details about the MCE error,
 	 * (if the CPU has an implementation for that)
 	 */
-	atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	if (ret != NOTIFY_STOP)
+		pr_emerg(HW_ERR "Run the above through 'mcelog --ascii' to decode.\n");
 }
 
 

Borislav,

I still think you need the check for UC here. When an UC occurs and mce_panic() is called the output will include:

[Hardware Error]: Run the above through 'mcelog --ascii' to decode.

potentially many, many times

. The problem still is that there is no
output to decode (in the default case).

Quoted 15 lines · Prarit Bhargava

arch/x86/kernel/cpu/mcheck/mce.c static void print_mce(struct mce *m) · line 235

Quoted 12 lines

arch/x86/kernel/cpu/mcheck/mce.c +3 -1

@@ -239,7 +227,9 @@ static void print_mce(struct mce *m)
 	 * Print out human-readable details about the MCE error,
 	 * (if the CPU has an implementation for that)
 	 */
-	atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	if (ret != NOTIFY_STOP)
+		pr_emerg(HW_ERR "Run the above through 'mcelog --ascii' to decode.\n");
 }
 
 

Borislav,

Oops. Let me *carefully* rephrase that so it is clear what I'm complaining about.

Quoted 6 lines

I still think you need the check for UC here. When an UC occurs and mce_panic() is called the output will include:

[Hardware Error]: Run the above through 'mcelog --ascii' to decode.

potentially many, many times

for _all_ unreported *correctable* errors.

Quoted 4 lines

. The problem still is that there is no
output to decode (in the default case).

ie) (sorry for the cut-and-paste)

        /* First print corrected ones that are still unlogged */
        for (i = 0; i < MCE_LOG_LEN; i++) {
                struct mce *m = &mcelog.entry[i];
                if (!(m->status & MCI_STATUS_VAL))
                        continue;
                if (!(m->status & MCI_STATUS_UC)) {
                        print_mce(m);
                        if (!apei_err)
                                apei_err = apei_write_mce(m);
                }
        }

will potentially result in many bogus messages during a time at which we definitely do not want bogus messages.

In the worst case, we will report 32 CEs before panicking. For that case we either do printk_once as Tony suggested or we ratelimit it. I'll update the patch.

In the worst case, we will report 32 CEs before panicking. For that case we either do printk_once as Tony suggested or we ratelimit it. I'll update the patch.

Quoted 4 lines · Borislav Petkov

In the worst case, we will report 32 CEs before panicking. For that case we either do printk_once as Tony suggested or we ratelimit it. I'll update the patch.

Ok, how about the following, I ratelimit the printk to the default of 10 messages per 5 seconds. I've also got the hardware MCE injection patches ready and will do some testing with them.

--
From: Borislav Petkov <[email protected]>
Date: Wed, 13 Apr 2011 14:32:06 +0200
Subject: [PATCH -v3] x86, MCE: Drop the default decoding notifier

The default notifier doesn't make a lot of sense to call in the correctable errors case. Drop it and emit the mcelog decoding hint only in the uncorrectable errors case and when no notifier is registered. Also, limit the issual of the "mcelog" message in the remote case when we dump unreported CEs before panicking.

While at it, remove unused old x86_mce_decode_callback from the header.

Signed-off-by Prarit Bhargava <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/include/asm/mce.h       |    2 --
 arch/x86/kernel/cpu/mcheck/mce.c |   23 ++++++-----------------
 2 files changed, 6 insertions(+), 19 deletions(-)

arch/x86/include/asm/mce.h +0 -2

index eb16e94..021979a 100644
@@ -142,8 +142,6 @@ static inline void winchip_mcheck_init(struct cpuinfo_x86 *c) {}
 static inline void enable_p5_mce(void) {}
 #endif
 
-extern void (*x86_mce_decode_callback)(struct mce *m);
-
 void mce_setup(struct mce *m);
 void mce_log(struct mce *m);
 DECLARE_PER_CPU(struct sys_device, mce_dev);

arch/x86/kernel/cpu/mcheck/mce.c +6 -17

index 68e2303..679bd9e 100644
@@ -105,20 +105,6 @@ static int			cpu_missing;
 ATOMIC_NOTIFIER_HEAD(x86_mce_decoder_chain);
 EXPORT_SYMBOL_GPL(x86_mce_decoder_chain);
 
-static int default_decode_mce(struct notifier_block *nb, unsigned long val,
-			       void *data)
-{
-	pr_emerg(HW_ERR "No human readable MCE decoding support on this CPU type.\n");
-	pr_emerg(HW_ERR "Run the message through 'mcelog --ascii' to decode.\n");
-
-	return NOTIFY_STOP;
-}
-
-static struct notifier_block mce_dec_nb = {
-	.notifier_call = default_decode_mce,
-	.priority      = -1,
-};
-
 /* MCA banks polled by the period polling timer for corrected events */
 DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = {
 	[0 ... BITS_TO_LONGS(MAX_NR_BANKS)-1] = ~0UL

@@ -212,6 +198,8 @@ void mce_log(struct mce *mce)
 
 static void print_mce(struct mce *m)
 {
+	int ret = 0;
+
 	pr_emerg(HW_ERR "CPU %d: Machine Check Exception: %Lx Bank %d: %016Lx\n",
 	       m->extcpu, m->mcgstatus, m->bank, m->status);
 

@@ -239,7 +227,10 @@ static void print_mce(struct mce *m)
 	 * Print out human-readable details about the MCE error,
 	 * (if the CPU has an implementation for that)
 	 */
-	atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+	if (ret != NOTIFY_STOP)
+		printk_ratelimited(KERN_EMERG HW_ERR
+				   "Run the above through 'mcelog --ascii'\n");
 }
 
 #define PANIC_TIMEOUT 5 /* 5 seconds */

@@ -1721,8 +1712,6 @@ __setup("mce", mcheck_enable);
 
 int __init mcheck_init(void)
 {
-	atomic_notifier_chain_register(&x86_mce_decoder_chain, &mce_dec_nb);
-
 	mcheck_intel_therm_init();
 
 	return 0;

Quoted 10 lines · Borislav Petkov

Quoted 4 lines · Borislav Petkov

In the worst case, we will report 32 CEs before panicking. For that case we either do printk_once as Tony suggested or we ratelimit it. I'll update the patch.

Ok, how about the following, I ratelimit the printk to the default of 10 messages per 5 seconds. I've also got the hardware MCE injection patches ready and will do some testing with them.

See my previous email ;) I think just putting in a printk_once after the CE call to print_mce() in mce_panic() might be better? At least that way we get the --ascii message for *EVERY* UC which IMO would be nice...

Quoted 16 lines · Prarit Bhargava

Quoted 13 lines · Borislav Petkov

Quoted 5 lines · Borislav Petkov

In the worst case, we will report 32 CEs before panicking. For that case we either do printk_once as Tony suggested or we ratelimit it. I'll update the patch.

Ok, how about the following, I ratelimit the printk to the default of 10 messages per 5 seconds. I've also got the hardware MCE injection patches ready and will do some testing with them.

Quoted 5 lines

See my previous email ;) I think just putting in a printk_once after the CE call to print_mce() in mce_panic() might be better? At least that way we get the --ascii message for *EVERY* UC which IMO would be nice...

Are you sure? printk_once() is, as its name says, a one-time thing and it is implemented that way - a static bool counter which is once set and that's it. I.e., the "--ascii" message will be printed only once for the system's lifetime.

From my patch (sorry for the cut-and-paste):

Oops ... I may have confused you because what I did was subtle. I really should have explicitly pointed out what I did. Sorry, my bad.

From my patch (sorry for the cut-and-paste):

arch/x86/kernel/cpu/mcheck/mce.c +4 -1

@@ -239,7 +227,10 @@ static void print_mce(struct mce *m)
         * Print out human-readable details about the MCE error,
         * (if the CPU has an implementation for that)
         */
-       atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+       ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+       if (ret != NOTIFY_STOP && (m->status & MCI_STATUS_UC))
+               pr_emerg(HW_ERR "Run the above through 'mcelog --ascii' "
+                        "to decode.\n");
 }
 

This, of course, only outputs during UCs.

and

(quoted diff) +1 -0

@@ -289,6 +280,8 @@ static void mce_panic(char *msg, struct mce *final,
 char *exp)
                        continue;
                if (!(m->status & MCI_STATUS_UC)) {
                        print_mce(m);
+                       printk_once(KERN_EMERG HW_ERR "MCE Corrected

Error(s) "

+                                   "detected.");
                        if (!apei_err)
                                apei_err = apei_write_mce(m);
                }

so we'll print "MCE Corrected Error(s)" _once_ if we go through this path. Since there is no data to decode with mcelog, a nice little one time message is probably the way to go :).

Quoted 27 lines · Prarit Bhargava

Oops ... I may have confused you because what I did was subtle. I really should have explicitly pointed out what I did. Sorry, my bad.

From my patch (sorry for the cut-and-paste):

arch/x86/kernel/cpu/mcheck/mce.c +4 -1

@@ -239,7 +227,10 @@ static void print_mce(struct mce *m)
         * Print out human-readable details about the MCE error,
         * (if the CPU has an implementation for that)
         */
-       atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+       ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
+       if (ret != NOTIFY_STOP && (m->status & MCI_STATUS_UC))
+               pr_emerg(HW_ERR "Run the above through 'mcelog --ascii' "
+                        "to decode.\n");
 }
 

This, of course, only outputs during UCs.

and

(quoted diff) +1 -0

@@ -289,6 +280,8 @@ static void mce_panic(char *msg, struct mce *final,
 char *exp)
                        continue;
                if (!(m->status & MCI_STATUS_UC)) {
                        print_mce(m);
+                       printk_once(KERN_EMERG HW_ERR "MCE Corrected

Error(s) "

Quoted 9 lines

+                                   "detected.");
                        if (!apei_err)
                                apei_err = apei_write_mce(m);
                }

so we'll print "MCE Corrected Error(s)" _once_ if we go through this path. Since there is no data to decode with mcelog, a nice little one time message is probably the way to go :).

Ok, first of all, see the print_mce(m) call above? Yes, we're dumping full CE MCE info in this case because they were unlogged and as such, that info can be decoded.

But this whole point is moot since those errors can be only 32 max _and_ on the _panic_ path. And I don't think this path matters because it is _very_ seldom. I bet you don't hit it on any of your machines.

And we don't want to fix that - we want to fix the case with the occasional CE MCEs which get detected in the polling path but none of their MCA regs get dumped for decoding so the decoding hint there is out of place. And we fixed that at least partially so that it doesn't flood the logs. If you're not fine with the default ratelimit of 10 msgs per 5 seconds we can always raise the ratelimit but tweaking an almost hypothetical case is just not worth it.

Thanks.

Quoted 8 lines

And we don't want to fix that - we want to fix the case with the occasional CE MCEs which get detected in the polling path but none of their MCA regs get dumped for decoding so the decoding hint there is out of place. And we fixed that at least partially so that it doesn't flood the logs. If you're not fine with the default ratelimit of 10 msgs per 5 seconds we can always raise the ratelimit but tweaking an almost hypothetical case is just not worth it.

Okay -- I'm good then.

Quoted 11 lines · Prarit Bhargava

Quoted 9 lines

And we don't want to fix that - we want to fix the case with the occasional CE MCEs which get detected in the polling path but none of their MCA regs get dumped for decoding so the decoding hint there is out of place. And we fixed that at least partially so that it doesn't flood the logs. If you're not fine with the default ratelimit of 10 msgs per 5 seconds we can always raise the ratelimit but tweaking an almost hypothetical case is just not worth it.

Okay -- I'm good then.

Ok, injecting MCEs with this patch looks like this. Nevermind the decoded MCEs, I simply uncommented the "if (ret != NOTIFY_DONE)" line so that I can see the rate limiting. So we still have them there, this is the default setting of 10 calls per 5 secs, we might want to dial it down in the output. After the 10th MCE, it doesn't appear anymore.

Hmm...

Prarit, you said you have a machine which spits a lot of CECCs on boot, does the final version help there, did you have a chance to run it?

Quoted 12 lines · Prarit Bhargava

Quoted 10 lines · Borislav Petkov

Quoted 4 lines · Borislav Petkov

In the worst case, we will report 32 CEs before panicking. For that case we either do printk_once as Tony suggested or we ratelimit it. I'll update the patch.

Ok, how about the following, I ratelimit the printk to the default of 10 messages per 5 seconds. I've also got the hardware MCE injection patches ready and will do some testing with them.

Quoted 4 lines

See my previous email ;) I think just putting in a printk_once after the CE call to print_mce() in mce_panic() might be better? At least that way we get the --ascii message for *EVERY* UC which IMO would be nice...

Are you sure? printk_once() is, as its name says, a one-time thing and it is implemented that way - a static bool counter which is once set and that's it. I.e., the "--ascii" message will be printed only once for the system's lifetime.

The ratelimit-ed thing dumps it a strict number of times. In the end, I don't have a strong opinion on how many times we issue it - I'm fine with it either way.

Maybe some other opinions. Tony?

Quoted 23 lines · Borislav Petkov

Quoted 12 lines · Prarit Bhargava

Quoted 10 lines · Borislav Petkov

Quoted 4 lines · Borislav Petkov

In the worst case, we will report 32 CEs before panicking. For that case we either do printk_once as Tony suggested or we ratelimit it. I'll update the patch.

Ok, how about the following, I ratelimit the printk to the default of 10 messages per 5 seconds. I've also got the hardware MCE injection patches ready and will do some testing with them.

Quoted 4 lines

See my previous email ;) I think just putting in a printk_once after the CE call to print_mce() in mce_panic() might be better? At least that way we get the --ascii message for *EVERY* UC which IMO would be nice...

Are you sure? printk_once() is, as its name says, a one-time thing and it is implemented that way - a static bool counter which is once set and that's it. I.e., the "--ascii" message will be printed only once for the system's lifetime.

Quoted 5 lines

The ratelimit-ed thing dumps it a strict number of times. In the end, I don't have a strong opinion on how many times we issue it - I'm fine with it either way.

Maybe some other opinions. Tony?

In general I think you and Prarit are headed in the right direction.

As for when to throttle messages, differing people will have different opinions as to the right number. For example, some sites may want the threshold low because once they see a CE they will schedule to replace the DIMM. Manufacturing sometimes wants to see all the CEs to know how good/bad the DIMM is.

My suggestion is to pick a default value and have a /sys (or other) way of changing the value. That way if someone has a need to change the value they can. In real life someone will have a legitimate need to change the value.

Is the thresholding on a per DIMM, per Socket, or per system basis? SGI tends to have large systems with lots of DIMMs. Per DIMM or per Socket thresholds tend to scale better.

Quoted 3 lines

Why not? This way you turn reporting of _ALL_ correctable MCEs completely off and some users would actually like to run them through mcelog on Intel.

pr_emerg() is rather overkill for a corrected error - on large systems corrected errors are going to be a routine occurrence (my personal estimation is "one soft error per gigabyte per month" ... which is pretty much the same as "one per terabyte per hour" for the people with the really cool toys.

We are also setting TAINT_MACHINE_CHECK for corrected errors - perhaps this made sense when systems were small and machine checks were rare and scary. But I think we need to start working with the reality that corrected errors are normal events.