mce recovery for Sandy Bridge server

Here's a nine-part patch series to implement "AR=1" recovery that will be available on high-end Sandy Bridge server processors. In this case the process detects an uncorrectable memory error while doing an instruction of data fetch that is about to be consumed. This is in contrast to the recoverable errors on Nehalem and Westmere that were out of immediate execution context (patrol scrubber and cache line write-back).

The code is based on work done by Andi last year and published in the "mce/action-required" branch of his mce git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-mce-2.6.git
Thus he gets author credit on 6 out of 9 patches (but I'll take the blame for all of them).

The first eight patches are mostly cleanups and minor new bits that are needed by part 9 where the interesting stuff happens.

For the "in context" case, we must not return from the machine check handler (in the data fetch case we'd re-execute the fetch and take another machine check, in the instruction fetch case we actually don't have a precise IP to return to). We use the TIF_MCE_NOTIFY task flag bit to ensure that we don't return to the user context - but we also need to keep track of the memory address where the fault occurred. The h/w only gives us the physical address which we must keep track of ... to do so we have added "mce_error_pfn" to the task structure - this feels odd, but it is an attribute of the task (e.g. this task may be migrated to another processor before we get to look at TIF_MCE_NOTIFY and head to do_notify_resume() to process it).

Andi's recovery code can also handle a few cases where the error is detected while running kernel code (when copying data to/from a user process) - but the TIF_MCE_NOTIFY method doesn't actually ever get to this code (since the entry_64.S code only checks TIF_MCE_NOTIFY on return to userspace). I'd appreciate any ideas on how to handle this. Perhaps we could do good things when CONFIG_PREEMPT=y (it seems probable that any error in a non-preemtible section of kernel code is going to be fatal).

-Tony

 arch/x86/include/asm/mce.h                |    3 +-
 arch/x86/kernel/cpu/mcheck/mce-severity.c |   37 +++-
 arch/x86/kernel/cpu/mcheck/mce.c          |  286 ++++++++++++++++++++++++-----
 arch/x86/kernel/signal.c                  |    2 +-
 include/linux/init_task.h                 |    7 +
 include/linux/sched.h                     |    3 +
 mm/memory-failure.c                       |   28 ++--
 7 files changed, 300 insertions(+), 66 deletions(-)

Andi Kleen (6):
MCE: Always retrieve mce rip before calling no_way_out
MCE: Move ADDR/MISC reading code into common function
MCE: Mask out address mask bits below address granuality
HWPOISON: Handle hwpoison in current process
MCE: Pass registers to work handlers
MCE: Add Action-Required support

Tony Luck (3):
mce: fixes for mce severity table
mce: save most severe error information
mce: run through processors with more severe problems first

From: Tony Luck <[email protected]>

The "Spurious not enabled" entry is redundant. The "Not enabled" entry earlier in the table will cover this case.

The "Action required; unknown MCACOD" entry shouldn't specify MCACOD in the .mask field. Current code will only match for mcacod==0 rather than all AR=1 entries.

---
 arch/x86/kernel/cpu/mcheck/mce-severity.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

index 1e8d66c..352d16a 100644
@@ -69,8 +69,6 @@ static struct severity {
 	MCGMASK(MCG_STATUS_RIPV, 0, PANIC, "In kernel and no restart IP",
 		KERNEL),
 	BITCLR(MCI_STATUS_UC, KEEP, "Corrected error", NOSER),
-	MASK(MCI_STATUS_OVER|MCI_STATUS_UC|MCI_STATUS_EN, MCI_STATUS_UC, SOME,
-	     "Spurious not enabled", SER),
 
 	/* ignore OVER for UCNA */
 	MASK(MCI_UC_SAR, MCI_STATUS_UC, KEEP,

@@ -82,7 +80,7 @@ static struct severity {
 	/* AR add known MCACODs here */
 	MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_STATUS_OVER|MCI_UC_SAR, PANIC,
 	     "Action required with lost events", SER),
-	MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCACOD, MCI_UC_SAR, PANIC,
+	MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR, PANIC,
 	     "Action required; unknown MCACOD", SER),
 
 	/* known AO MCACODs: */

1.7.3.1

From: Tony Luck <[email protected]>

monarch clears all of the per cpu "mces_seen", so we must keep a copy to use after mce_end()

---
 arch/x86/kernel/cpu/mcheck/mce.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

index 3385ea2..ed1542a 100644
@@ -1046,6 +1046,9 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 		}
 	}
 
+	/* Save our worst error locally, monarch will clear mces_seen */
+	m = *final;
+
 	if (!no_way_out)
 		mce_clear_state(toclear);
 

@@ -1064,7 +1067,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	 * support MCE broadcasting or it has been disabled.
 	 */
 	if (no_way_out && tolerant < 3)
-		mce_panic("Fatal machine check on current CPU", final, msg);
+		mce_panic("Fatal machine check on current CPU", &m, msg);
 
 	/*
 	 * If the error seems to be unrecoverable, something should be

1.7.3.1

From: Andi Kleen <[email protected]>

Some of the rules in the decision engine need a valid rip, so set it up before calling it.

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index ed1542a..ee3b14e 100644
@@ -957,6 +957,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	final = &__get_cpu_var(mces_seen);
 	*final = m;
 
+	mce_get_rip(&m, regs);
 	no_way_out = mce_no_way_out(&m, &msg);
 
 	barrier();

@@ -1037,7 +1038,6 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 		if (severity == MCE_AO_SEVERITY && mce_usable_address(&m))
 			mce_ring_add(m.addr >> PAGE_SHIFT);
 
-		mce_get_rip(&m, regs);
 		mce_log(&m);
 
 		if (severity > worst) {

1.7.3.1

From: Andi Kleen <[email protected]>

Used for next patch.

---
 arch/x86/kernel/cpu/mcheck/mce.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

index ee3b14e..f4924dc 100644
@@ -525,6 +525,17 @@ static void mce_report_event(struct pt_regs *regs)
 #endif
 }
 
+/*
+ * Read ADDR and MISC registers.
+ */
+static void mce_read_aux(struct mce *m, int i)
+{
+	if (m->status & MCI_STATUS_MISCV)
+		m->misc = mce_rdmsrl(MSR_IA32_MCx_MISC(i));
+	if (m->status & MCI_STATUS_ADDRV)
+		m->addr = mce_rdmsrl(MSR_IA32_MCx_ADDR(i));
+}
+
 DEFINE_PER_CPU(unsigned, mce_poll_count);
 
 /*

@@ -576,10 +587,7 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		    (m.status & (mce_ser ? MCI_STATUS_S : MCI_STATUS_UC)))
 			continue;
 
-		if (m.status & MCI_STATUS_MISCV)
-			m.misc = mce_rdmsrl(MSR_IA32_MCx_MISC(i));
-		if (m.status & MCI_STATUS_ADDRV)
-			m.addr = mce_rdmsrl(MSR_IA32_MCx_ADDR(i));
+		mce_read_aux(&m, i);
 
 		if (!(flags & MCP_TIMESTAMP))
 			m.tsc = 0;

@@ -1023,10 +1031,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 		if (severity == MCE_AR_SEVERITY)
 			kill_it = 1;
 
-		if (m.status & MCI_STATUS_MISCV)
-			m.misc = mce_rdmsrl(MSR_IA32_MCx_MISC(i));
-		if (m.status & MCI_STATUS_ADDRV)
-			m.addr = mce_rdmsrl(MSR_IA32_MCx_ADDR(i));
+		mce_read_aux(&m, i);
 
 		/*
 		 * Action optional error. Queue address for later processing.

1.7.3.1

From: Andi Kleen <[email protected]>

SER enabled systems report the address granuality for each reported address in a machine check. But the bits below the granuality are undefined. Mask them out before logging the machine check.

---
 arch/x86/kernel/cpu/mcheck/mce.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

index f4924dc..a9a0e25 100644
@@ -532,8 +532,18 @@ static void mce_read_aux(struct mce *m, int i)
 {
 	if (m->status & MCI_STATUS_MISCV)
 		m->misc = mce_rdmsrl(MSR_IA32_MCx_MISC(i));
-	if (m->status & MCI_STATUS_ADDRV)
+	if (m->status & MCI_STATUS_ADDRV) {
 		m->addr = mce_rdmsrl(MSR_IA32_MCx_ADDR(i));
+
+		/*
+		 * Mask the reported address by the reported granuality.
+		 */
+		if (mce_ser && (m->status & MCI_STATUS_MISCV)) {
+			u8 shift = m->misc & 0x1f;
+			m->addr >>= shift;
+			m->addr <<= shift;
+		}
+	}
 }
 
 DEFINE_PER_CPU(unsigned, mce_poll_count);

1.7.3.1

From: Andi Kleen <[email protected]>

When hardware poison handles the current process use a forced signal with _AR severity.

---
 mm/memory-failure.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

index 2b9a5ee..a203113 100644
@@ -184,8 +184,7 @@ int hwpoison_filter(struct page *p)
 EXPORT_SYMBOL_GPL(hwpoison_filter);
 
 /*
- * Send all the processes who have the page mapped an ``action optional''
- * signal.
+ * Send all the processes who have the page mapped a SIGBUS.
  */
 static int kill_proc_ao(struct task_struct *t, unsigned long addr, int trapno,
 			unsigned long pfn, struct page *page)

@@ -194,23 +193,28 @@ static int kill_proc_ao(struct task_struct *t, unsigned long addr, int trapno,
 	int ret;
 
 	printk(KERN_ERR
-		"MCE %#lx: Killing %s:%d early due to hardware memory corruption\n",
-		pfn, t->comm, t->pid);
+		"MCE %#lx: Killing %s:%d due to hardware memory corruption\n",
+	       pfn, t->comm, t->pid);
 	si.si_signo = SIGBUS;
 	si.si_errno = 0;
-	si.si_code = BUS_MCEERR_AO;
 	si.si_addr = (void *)addr;
 #ifdef __ARCH_SI_TRAPNO
 	si.si_trapno = trapno;
 #endif
 	si.si_addr_lsb = compound_trans_order(compound_head(page)) + PAGE_SHIFT;
-	/*
-	 * Don't use force here, it's convenient if the signal
-	 * can be temporarily blocked.
-	 * This could cause a loop when the user sets SIGBUS
-	 * to SIG_IGN, but hopefully no one will do that?
-	 */
-	ret = send_sig_info(SIGBUS, &si, t);  /* synchronous? */
+	if (t == current) {
+		si.si_code = BUS_MCEERR_AR;
+		ret = force_sig_info(SIGBUS, &si, t);
+	} else {
+		/*
+		 * Don't use force here, it's convenient if the signal
+		 * can be temporarily blocked.
+		 * This could cause a loop when the user sets SIGBUS
+		 * to SIG_IGN, but hopefully noone will do that?
+		 */
+		si.si_code = BUS_MCEERR_AO;
+		ret = send_sig_info(SIGBUS, &si, t);
+	}
 	if (ret < 0)
 		printk(KERN_INFO "MCE: Error sending signal to %s:%d: %d\n",
 		       t->comm, t->pid, ret);

1.7.3.1

From: Andi Kleen <[email protected]>

Pass the register context to the task work handler. Needed for the next patch

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

index eb16e94..66c41e0 100644
@@ -195,7 +195,8 @@ enum mcp_flags {
 void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
 
 int mce_notify_irq(void);
-void mce_notify_process(void);
+struct pt_regs;
+void mce_notify_process(struct pt_regs *regs);
 
 DECLARE_PER_CPU(struct mce, injectm);
 extern struct file_operations mce_chrdev_ops;

index a9a0e25..6b2fa20 100644
@@ -1123,7 +1123,7 @@ void __attribute__((weak)) memory_failure(unsigned long pfn, int vector)
  * This is merely a fast path to expedite processing in some common
  * cases.
  */
-void mce_notify_process(void)
+void mce_notify_process(struct pt_regs *regs)
 {
 	unsigned long pfn;
 	mce_notify_irq();

@@ -1133,7 +1133,7 @@ void mce_notify_process(void)
 
 static void mce_process_work(struct work_struct *dummy)
 {
-	mce_notify_process();
+	mce_notify_process(NULL);
 }
 
 #ifdef CONFIG_X86_MCE_INTEL

index 4fd173c..3e6d421 100644
@@ -841,7 +841,7 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
 #ifdef CONFIG_X86_MCE
 	/* notify userspace of pending MCEs */
 	if (thread_info_flags & _TIF_MCE_NOTIFY)
-		mce_notify_process();
+		mce_notify_process(regs);
 #endif /* CONFIG_X86_64 && CONFIG_X86_MCE */
 
 	/* deal with pending signal delivery */

1.7.3.1

From: Tony Luck <[email protected]>

Instead of letting cpus run through the MC bank scanning code in the order that they turned up in the handler, we arrange to deal with those that have more severe problems (mcgstatus.ripv=0) first. This will make life simpler in the case that banks are shared between processors, since the cpu with the problem will see it and clear it, leaving the other cpu(s) that share the bank with nothing to do.

---
 arch/x86/kernel/cpu/mcheck/mce.c |   87 +++++++++++++++++++++++++++-----------
 1 files changed, 62 insertions(+), 25 deletions(-)

index 6b2fa20..7da4a75 100644
@@ -646,7 +646,7 @@ static int mce_no_way_out(struct mce *m, char **msg)
  * Variable to establish order between CPUs while scanning.
  * Each CPU spins initially until executing is equal its number.
  */
-static atomic_t mce_executing;
+static atomic_t mce_executing = ATOMIC_INIT(-1);
 
 /*
  * Defines order of CPUs on entry. First CPU becomes Monarch.

@@ -762,13 +762,40 @@ static void mce_reign(void)
 static atomic_t global_nwo;
 
 /*
+ * Keep separate bitmaps for cpus that have the option return from
+ * machine check handler (MCG_STATUS.RIPV == 1) and those for that
+ * cannot.
+ */
+static cpumask_t	can_return;
+static cpumask_t	cant_return;
+
+static int	monarch;
+
+/*
+ * next cpu choosing first from cant_return, and then from can_return
+ */
+int mce_nextcpu(int this)
+{
+	int next;
+
+	if (this == -1 || cpumask_test_cpu(this, &cant_return)) {
+		next = cpumask_next(this, &cant_return);
+		if (next >= nr_cpu_ids)
+			next = cpumask_next(-1, &can_return);
+		return next;
+	}
+
+	return cpumask_next(this, &can_return);
+}
+
+/*
  * Start of Monarch synchronization. This waits until all CPUs have
  * entered the exception handler and then determines if any of them
  * saw a fatal event that requires panic. Then it executes them
- * in the entry order.
+ * one at a time.
  * TBD double check parallel CPU hotunplug
  */
-static int mce_start(int *no_way_out)
+static int mce_start(int *no_way_out, int noreturn)
 {
 	int order;
 	int cpus = num_online_cpus();

@@ -784,6 +811,11 @@ static int mce_start(int *no_way_out)
 	smp_wmb();
 	order = atomic_inc_return(&mce_callin);
 
+	if (noreturn)
+		cpumask_set_cpu(smp_processor_id(), &cant_return);
+	else
+		cpumask_set_cpu(smp_processor_id(), &can_return);
+
 	/*
 	 * Wait for everyone.
 	 */

@@ -802,23 +834,26 @@ static int mce_start(int *no_way_out)
 
 	if (order == 1) {
 		/*
-		 * Monarch: Starts executing now, the others wait.
+		 * Choose a cpu to be monarch.  It will run first
 		 */
-		atomic_set(&mce_executing, 1);
-	} else {
-		/*
-		 * Subject: Now start the scanning loop one by one in
-		 * the original callin order.
-		 * This way when there are any shared banks it will be
-		 * only seen by one CPU before cleared, avoiding duplicates.
-		 */
-		while (atomic_read(&mce_executing) < order) {
-			if (mce_timed_out(&timeout)) {
-				atomic_set(&global_nwo, 0);
-				return -1;
-			}
-			ndelay(SPINUNIT);
+		monarch = mce_nextcpu(-1);
+		atomic_set(&mce_executing, monarch);
+	}
+	/*
+	 * Subject: Now start the scanning loop one by one in
+	 * choosing first the "cant_return" cpus and the the others.
+	 * This way when there are any shared banks it will be
+	 * only seen by one CPU before cleared, avoiding duplicates.
+	 * Letting the "cant_return" folks go first means that for
+	 * "action required" errors, the cpu that hit the problem
+	 * will find its error in the shared bank first.
+	 */
+	while (atomic_read(&mce_executing) != smp_processor_id()) {
+		if (mce_timed_out(&timeout)) {
+			atomic_set(&global_nwo, 0);
+			return -1;
 		}
+		ndelay(SPINUNIT);
 	}
 
 	/*

@@ -846,9 +881,9 @@ static int mce_end(int order)
 	/*
 	 * Allow others to run.
 	 */
-	atomic_inc(&mce_executing);
+	atomic_set(&mce_executing, mce_nextcpu(smp_processor_id()));
 
-	if (order == 1) {
+	if (smp_processor_id() == monarch) {
 		/* CHECKME: Can this race with a parallel hotplug? */
 		int cpus = num_online_cpus();
 

@@ -856,7 +891,7 @@ static int mce_end(int order)
 		 * Monarch: Wait for everyone to go through their scanning
 		 * loops.
 		 */
-		while (atomic_read(&mce_executing) <= cpus) {
+		while (atomic_read(&mce_executing) != nr_cpu_ids) {
 			if (mce_timed_out(&timeout))
 				goto reset;
 			ndelay(SPINUNIT);

@@ -869,7 +904,7 @@ static int mce_end(int order)
 		/*
 		 * Subject: Wait for Monarch to finish.
 		 */
-		while (atomic_read(&mce_executing) != 0) {
+		while (atomic_read(&mce_executing) != -1) {
 			if (mce_timed_out(&timeout))
 				goto reset;
 			ndelay(SPINUNIT);

@@ -887,12 +922,14 @@ static int mce_end(int order)
 reset:
 	atomic_set(&global_nwo, 0);
 	atomic_set(&mce_callin, 0);
+	cpumask_clear(&can_return);
+	cpumask_clear(&cant_return);
 	barrier();
 
 	/*
 	 * Let others run again.
 	 */
-	atomic_set(&mce_executing, 0);
+	atomic_set(&mce_executing, -1);
 	return ret;
 }
 

@@ -991,7 +1028,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	 * This way we don't report duplicated events on shared banks
 	 * because the first one to see it will clear it.
 	 */
-	order = mce_start(&no_way_out);
+	order = mce_start(&no_way_out, kill_it);
 	for (i = 0; i < banks; i++) {
 		__clear_bit(i, toclear);
 		if (!mce_banks[i].ctl)

@@ -2193,7 +2230,7 @@ static void mce_reset(void)
 {
 	cpu_missing = 0;
 	atomic_set(&mce_fake_paniced, 0);
-	atomic_set(&mce_executing, 0);
+	atomic_set(&mce_executing, -1);
 	atomic_set(&mce_callin, 0);
 	atomic_set(&global_nwo, 0);
 }

1.7.3.1

From: Andi Kleen <[email protected]>

Implement core MCA recovery. This is used for errors that happen in the current execution context.

The kernel has to first pass the error information to a function running on the current process stack. This is done using a new work flag and then executing the code after the exception through do_notify_resume.

Then hwpoison is allowed to sleep and can try to recover it.

To pass the information about the error around we need to use a field in the current process. The old ways to handle this (per cpu buffer) don't work because a CPU could be switched before reaching the handler code.

For kernel recovery we only handle errors happening during copy_*_user() exception tables and inject EFAULT. When the tolerance level is sufficiently high also a unsafe oops like do_exit() killing, which has some deadlock potential.

FIXME: fix 386 handling of mce notify bit in entry_32.S after mce

---
 arch/x86/kernel/cpu/mcheck/mce-severity.c |   35 ++++++-
 arch/x86/kernel/cpu/mcheck/mce.c          |  157 +++++++++++++++++++++++++++--
 include/linux/init_task.h                 |    7 ++
 include/linux/sched.h                     |    3 +
 4 files changed, 189 insertions(+), 13 deletions(-)

index 352d16a..fe8a28c 100644
@@ -13,6 +13,7 @@
 #include <linux/seq_file.h>
 #include <linux/init.h>
 #include <linux/debugfs.h>
+#include <linux/module.h>
 #include <asm/mce.h>
 
 #include "mce-internal.h"

@@ -54,6 +55,9 @@ static struct severity {
 	{ .mcgmask = x, .mcgres = res, SEV(s), .msg = m, ## r }
 #define MASK(x, y, s, m, r...) \
 	{ .mask = x, .result = y, SEV(s), .msg = m, ## r }
+#define ARMASK(x, y, s, m, r...) \
+	{ .mcgmask = MCG_STATUS_RIPV, .mcgres = 0, \
+	  .mask = x, .result = y, SEV(s), .msg = m, ## r }
 #define MCI_UC_S (MCI_STATUS_UC|MCI_STATUS_S)
 #define MCI_UC_SAR (MCI_STATUS_UC|MCI_STATUS_S|MCI_STATUS_AR)
 #define MCACOD 0xffff

@@ -67,7 +71,7 @@ static struct severity {
 	MCGMASK(MCG_STATUS_RIPV|MCG_STATUS_EIPV, 0, PANIC,
 		"Neither restart nor error IP"),
 	MCGMASK(MCG_STATUS_RIPV, 0, PANIC, "In kernel and no restart IP",
-		KERNEL),
+		KERNEL, NOSER),
 	BITCLR(MCI_STATUS_UC, KEEP, "Corrected error", NOSER),
 
 	/* ignore OVER for UCNA */

@@ -77,10 +81,16 @@ static struct severity {
 	     "Illegal combination (UCNA with AR=1)", SER),
 	MASK(MCI_STATUS_S, 0, KEEP, "Non signalled machine check", SER),
 
-	/* AR add known MCACODs here */
 	MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_STATUS_OVER|MCI_UC_SAR, PANIC,
 	     "Action required with lost events", SER),
-	MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR, PANIC,
+
+	/* known AR MCACODs: */
+	ARMASK(MCI_UC_SAR|MCI_STATUS_OVER|0xffff, MCI_UC_SAR|0x134, AR,
+	     "Action required: data load error", SER),
+	ARMASK(MCI_UC_SAR|MCI_STATUS_OVER|0xffff, MCI_UC_SAR|0x150, AR,
+	     "Action required: instruction fetch error", SER),
+
+	ARMASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR, PANIC,
 	     "Action required; unknown MCACOD", SER),
 
 	/* known AO MCACODs: */

@@ -89,6 +99,7 @@ static struct severity {
 	MASK(MCI_UC_SAR|MCI_STATUS_OVER|MCACOD, MCI_UC_S|0x17a, AO,
 	     "Action optional: last level cache writeback error", SER),
 
+
 	MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_S, SOME,
 	     "Action optional unknown MCACOD", SER),
 	MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_S|MCI_STATUS_OVER, SOME,

@@ -110,6 +121,17 @@ static int error_context(struct mce *m)
 	return IN_KERNEL;
 }
 
+static int kernel_ar_recoverable(struct mce *m, int tolerant)
+{
+	if (tolerant >= 2)
+		return MCE_AR_SEVERITY;
+	if (!(m->mcgstatus & MCG_STATUS_EIPV) || !m->ip)
+		return MCE_PANIC_SEVERITY;
+	if (search_exception_tables(m->ip))
+		return MCE_AR_SEVERITY;
+	return MCE_PANIC_SEVERITY;
+}
+
 int mce_severity(struct mce *a, int tolerant, char **msg)
 {
 	enum context ctx = error_context(a);

@@ -129,9 +151,12 @@ int mce_severity(struct mce *a, int tolerant, char **msg)
 		if (msg)
 			*msg = s->msg;
 		s->covered = 1;
-		if (s->sev >= MCE_UC_SEVERITY && ctx == IN_KERNEL) {
-			if (panic_on_oops || tolerant < 1)
+		if (ctx == IN_KERNEL) {
+			if (s->sev >= MCE_UC_SEVERITY &&
+				(panic_on_oops || tolerant < 1))
 				return MCE_PANIC_SEVERITY;
+			if (s->sev == MCE_AR_SEVERITY)
+				return kernel_ar_recoverable(a, tolerant);
 		}
 		return s->sev;
 	}

index caa151f..16ab936 100644
@@ -124,6 +124,12 @@ extern struct cred init_cred;
 # define INIT_PERF_EVENTS(tsk)
 #endif
 
+#ifdef CONFIG_X86_MCE
+#define INIT_MCE_ERROR_PFN .mce_error_pfn = -1L,
+#else
+#define INIT_MCE_ERROR_PFN
+#endif
+
 /*
  *  INIT_TASK is used to set up the first task table, touch at
  * your own risk!. Base=0, limit=0x1fffff (=2MB)

@@ -192,6 +198,7 @@ extern struct cred init_cred;
 	INIT_FTRACE_GRAPH						\
 	INIT_TRACE_RECURSION						\
 	INIT_TASK_RCU_PREEMPT(tsk)					\
+	INIT_MCE_ERROR_PFN						\
 }
 
 

index 781abd1..a72f3aa 100644
@@ -1540,6 +1540,9 @@ struct task_struct {
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 	atomic_t ptrace_bp_refcnt;
 #endif
+#ifdef CONFIG_X86_MCE
+	unsigned long mce_error_pfn;
+#endif
 };
 
 /* Future-safe accessor for struct task_struct's cpus_allowed. */

1.7.3.1

So, the generic comment i have on this is similar to what i wrote to Huang Ying already: i'd really like to see progress on the proper RAS / event approach instead of seeing further iterations of the very limited and isolated severity levels and mcelog approach ...

Also, more integration with EDAC would be required: there is no reason why the framework and user/admin appearance of hardware error handling of CPU and memory chipset level hardware should be different. In fact the physical distinction is largely moot since memory controllers moved into the CPU ...

Well, did Andi tell you that me and Thomas NAK-ed those bits and what reasons we outlined? See:

http://lkml.org/lkml/2010/2/16/309

We are more than half a year down the road and i do not see much progress on those important conceptual points in this RFC patchset either. The technical points we made in our original NAK was left largely unaddressed AFAICS.

The two main technical points made there still hold today:

- Please work on integrating RAS/EDAC with MCE. Both Mauro and Boris has made progress there, but frankly, i only saw Andi ignoring/stonewalling them.

- Please work on using a proper event framework for new hardware events, we really do not want to extend the mcelog mess, and we want to help efforts like unified system event logging and the new RAS daemon.

Now, some of that might be fun and non-trivial infrastructure work - so i'm not asking you to implement the world and everything on day 1. But seeing this non-progress on the Intel MCE side why should i consider lifting the NAK?

There is absolutely *no reason* for the MCE code to hook in to the kernel at such a low level and muck with task struct!

The error PFN is a useful event field - use it as a TRACE_EVENT() field. Processing events in non-NMI context is something you can do already as well, see how perf_event_wakeup() delays a ring-buffer wakeup to post-NMI IRQ context.

Creating a callback there would be a good place to do the TIF_MCE work and also to extract any events that got queued by other NMIs. Note that more events might be queued by further NMIs while we are processing the MCE path - while with the task->mce_error_pfn hack we are limited to a single pending event only and subsequent NMIs will overwrite this value!

A happy side effect is that the TIF_MCE_NOTIFY hack could go away as well.

Also note that if it's done via events then injection support will naturally extend to this capability as well and you can test it by injecting arbitrary error-PFNs ...

So my broad take on this is that historically the MCE code started out as a limited set of very low level hacks, both on the CPU support side, on the logging side and the user ABI side. For years it was only supporting AMD CPUs in a serious fashion. This was borderline acceptable because frankly back then MCE did not really matter much: boxes died faster than anyone could do anything intelligent about it.

But today it matters more. There are more intelligent errors emitted by the hardware and less fatal hardware behavior. The boundary between 'hard errors', 'soft errors' and 'system events' has become fuzzier: so we really want to have a generic notion for system events that matter to performance and reliability - instead of these isolated islands of weirdly coded functionality that the Intel MCE code is today. We want to be able to log all of these event classes, in one unified framework, and define policy on them.

So we *really* want to promote this code to a higher level of abstraction. Everyone would benefit from doing that: Intel hardware error handling features would be enabled much more richly and i suspect they would also be *used* in a much more meaningful way - driving the hw cycle as well.

Thanks,

Ingo

Absolutely agreed. The RAS architecture should look like this, IMHO:

I. Event collection: #MC handler and pollers, no queueing or buffering crap.

II. Pluggable and extensible filters which are
* per vendor
* configurable from userspace
* easily extensible
* decide whether action should be taken in the kernel or error is non-critical and should go to RAS daemon

III. Error handling callback(s)
* also extensible
* also per vendor
* also configurable from userspace

Advantages:
* reuse perf code - no need for ad-hoc new buffers and lockless thingies when we have it all already

* easy code and even hw testing with perf inject or ras inject
** this gives us also the different injection methods per vendor in an unified way instead of interfaces in /sys or debugfs or mcelog or ...

* keep code design sane instead of letting it needlessly fiddle with other parts of the kernel

* ...

Now I should better go and put my patches where my mouth is :).

Regards/Gruss,
    Boris.

Other points noted - I'll go look at the previous discussion threads you gave me links to.

I do want to comment on this point:

I wasn't very happy with task->mce_error_pfn either - but being overwritten is not one of its flaws. The task that stumbled on the error must not be run until the error is dealt with - any other NMIs for other errors must be happening to other tasks (who have their own task->mce_error_pfn).

We need some way to stop the task that found the error dead in its tracks - if it tripped over a data error, then running it will just trip over the same error again. If it had a memory error during an instruction fetch we have no place to return to.

So can we talk about this part for a while before returning to the "how to report this" discussion?

So here's the situation - we are in the NMI handler when we find from looking at the machine check bank registers that we have a recoverable error. We know the physical address, and we know the task (which might have been in user or kernel context). I can package that information into a perf/event ... but then how can I mark the current task as not-fit-for-execution?

-Tony

Maybe something like

set_current_state(TASK_UNINTERRUPTIBLE);

finish work in NMI context

do remaining work in process context like sending appropriate signals etc; finally:

set_task_state(tsk, TASK_RUNNING)

?

Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Dragging PeterZ to this thread, since we are now talking about scheduler.

That looks pretty easy - are their any weird side effects that I should be worried about? My perf/event can't really include the "task" pointer (that sounds way too internal) - but I can provide the process id, so the "RAS daemon" that sees this event can look up the task to do that final set_task_state(tsk, TASK_RUNNING).

Does this work in the threaded case? In the case where the task was in kernel context (but in a CONFIG_PREEMT=y kernel at some point where preemption is allowed)?

-Tony

Actually, I was thinking more in the direction of doing this in a kernel thread or workqueue without going back to the RAS daemon. Then you would only need to save the task_struct ptr.

Well, IIUC and depending on the error, if it is severe enough, you would want to run the remaining work right after the NMI handler finishes without going to userspace.

Hmm..

Regards/Gruss,
Boris.

Right, so you can't do things like that from NMI context, but what perf can do is raise a self-IPI and continue from IRQ context (question for the HW folks, can there be cycles between the NMI iret and IRQ assert from whatever context was before the NMI hit?)

basically do what SIGSTOP does and put the task in TASK_STOPPED state, wake your handler thread and set TIF_NEED_RESCHED. Then the handler thread will be scheduled depending on your handler's sched policy.

Of course there can be - the code where the NMI hit may have interrupts disabled.

Linus

D'0h yes of course!..

Then I can't immediately see a way to stop a thread dead in its tracks. Userspace yes, but not kernel-space.

It's a best-effort thing: run proper kernel code (hardirq context) as soon as possible - but obviously not sooner than that! :-)

Thanks,

Ingo

But the case when I'd want to do the "stop this task" thing is when I think that I can recover - for memory errors detected while in kernel code I expect this will only ever be a few special cases:
1) copy to/from user
2) copy page (for copy-on-write fault)
3) ...
and in these cases we don't have interrupts disabled. In fact I have difficulty imagining a scenario where the kernel trips over a memory error in interrupt disabled code that would ever be recoverable.

So my NMI handler can look at the saved pt_regs to see whether it blasted its way into some interrupt disabled code and call that fatal - if it came in while interrupts were enabled, then it could use Peter's self-IPI thingy.

-Tony

How about separating stuffs in:
step1) Add support for AR in user space :
- send sigbus to affected processes, poison affected memory
- panic if error is in kernel
step2) Add support for AR in kernel
- some new notify/handle mechanism etc.

It seems too big jump for me.

Thanks,
H.Seto

Perf itself has a copy_from_user_nmi which is used to collect stack traces. But those non-preemptible user accesses can be moved out of your special cases.

Joerg

If that task is running right now you might not have much of a chance but to be prepared for a repeat NMI the moment NMIs are enabled again after the IRET.

So repeat errors are not just

But yeah, this is not the worst ugliness of it indeed.

Well, the primary thing TIF_MCE_NOTIFY does is a roundabout way to iterate through repeat calls to memory_failure(), with all pfns that got buffered so far.

We already have a generic facility to do such things at
return-to-userspace: _TIF_USER_RETURN_NOTIFY.

Btw., the SIGKILL logic is probably overcomplicated: when it's clear that user-space can not recover why not do a do_exit() and be done with it? As long as it's called from a syscall level codepath and no locks/resources are held do_exit() can be called.

Well, you'd queue up a user return notifier, process the ring-buffer at that end (as a ring-buffer consumer), call policy action like memory_failure() which does its MM specific checks to see whether the task is recoverable or not.

The goal is that we wouldnt lose any existing hwpoison functionality over what hwpoison does today - we'd *gain* functionality:

- through the use of enumerated events we would give access to *other* users of the same event source as well, not just hwpoison.

- we'd also eventually remove the mcelog ring-buffer mechanism itself.

For this i'd suggest to take a look at Frederic's recent patches on lkml which decouple the events code some more:

Subject: [PATCH v2] hw_breakpoint: Let the user choose not to build it (and perf too)

Those could be compounded with more decoupling of perf events: for example right now 'struct perf_event' is pretty big, because it carries hw PMU details as well. But those details are not needed for non-PMU events so splitting those from struct perf_event would streamline this code some more.

Likewise the kernel/events/core.c ring-buffer bits could be split out into kernel/events/ring-buffer.c - i think Frederic might be working on this one already.

More advanced steps in the MCE area would be to replace the 'severity' logic (which is really a poor-man's filter) with event filters and panic the box (or do other immediate action) if the filter matches.

This again would not lose any functionality that the severity bits give us today, we'd gain functionality:

- the conditions in filter expressions are pretty flexible so we could do more than the current stack of severity bitmask ops. For example a system could be configured to ignore the first N non-fatal messages but panic the box if more than 10 errors were generated. If there's a "message_seq_nr" field available in the TRACE_EVENT() then this would be a simple "message_seq_nr >= 10" filter condition. With the current severity code this would have to be coded in, the ABI extended, etc. etc.

Although initially it would probably want to match what the severity filters do today and install all the default policy.

Thanks,

Ingo

Agreed - we can go step by step with different recovery cases. Deciding which to implement is a classic benefit vs. effort trade off.

For each case determine some ball-park numbers for the percentage of memory that will be in the state you wish to recover (hence user-mode scores very highly because most people buy machines to run applications, though there are some exceptions like file servers).

Determine the effort and invasiveness of a solution to recover - here it is clear that a way to handle arbitrary kernel memory corruption is never going to fly - but there is hope for some simple cases like copy to/from user (that already are specially tagged so that user level page faults can be processed).

-Tony

2011/5/25 Ingo Molnar <[email protected]>:

There is no SIGKILL - we use SIGBUS because it generally isn't clear to the kernel whether the error is recoverable (the kernel can tell whether it is *transparently* recoverable - e.g. by replacing a corrupt memory page with a new copy read from disk in the case that the page is mapped from a file and still marked as clean) - but if the kernel can't recover, we want to give the application a shot at doing so. So we send a SIGBUS with a payload specifying the virtual address and amount of data that has been lost.

One database vendor has already used this mechanism in a demo of application level recovery - a second is looking at doing so, and a third was internally divided about whether the engineering cost of doing this was justified given the rate of 2+bit memory errors.

[We do need a tweak here - it isn't helpful to have the application drop a core file in the SIG_DFL case - so we really ought to stop it from doing so]

Generally you'd want to avoid rules based on absolute counts like this, if you simply panic when you get to an event count of 10, then any system that runs for long enough will eventually accrue this many errors and die. Much better to use "events per time-window" (or a leaky bucket algorithm that slowly "forgets" about old errors). You might also want to keep separate counts per component (e.g. DIMM stick) because 10 errors from one DIMM stick may well indicate a problem with that DIMM, but 10 errors from different DIMMs is more likely an indication that your power supply is glitchy.

I'll have to think about whether some parts of what is being done by the existing severity code could be moved out to filters - I'm not certain that they can - the code uses that table to parse whats in the machine check banks as described in volume 3A, chapter 15 of the SDM to determine just what is going on. The severity codes refer to each bank (and each logical cpu nominally has its own set of banks - some banks are actually shared between hyperthreads on the same core, or cores on the same socket). The meanings are:

MCE_NO_SEVERITY = no error logged in this bank

MCE_KEEP_SEVERITY = something here, but is not useful in our current context, leave it alone. The "S" bit in the MCi_STATUS register is used to mark whether an entry should be processed by CMCI/poll of the banks, or by the NMI machine check event hanlder (this resolves races when a machine check is delivered while handling a CMCI)

MCE_SOME_SEVERITY = a real error, low severity (e.g. h/w has already corrected it)

MCE_AO_SEVERITY = an uncorrected error has been found, but it need not be handled
right away (e.g. patrol scrubber found a 2-bit error in memory that is not currently being
accessed by any processor).

MCE_UC_SEVERITY - on pre-nehalem cpus uncorrected errors are never recoverable, so
the AO and AR values are not used

MCE_AR_SEVERITY - an uncorrected error in current execution context - something must be done, if OS can't figure out what, then this error is fatal.

MCE_PANIC_SEVERITY - instant death, no saving throw (log to NVRAM if you have it)

So I think that we still need this triage - to tell us which sort of perf/event to
generate (corrected vs. uncorrected, memory vs. something else, ...), and whether
we need to take some action in the kernel immediately.

Probably all the event filtering can do is count and analyse the stream of corrected
and recovered errors to look for patterns for some pre-emptive action - but the bulk of the complex logic for this should be in the user level "RASdaemon" that is consuming the perf/events.

-Tony

there is one here:

mm/memory-failure.c::kill_procs_ao()

                                force_sig(SIGKILL, tk->tsk);

Which can be reached from memory_failure() in certain (justified) circumstances.

Thanks,

Ingo

2011/5/25 Ingo Molnar <[email protected]>:

This looked really promising as a way to drop one use of TIF_MCE_NOTIFY, but it doesn't currently quite do what is needed for my new case.

What I need is a way to grab the current task just before it returns to user space - what this code appears to do is to catch the current *processor* just before
it sees a flagged process trying to return to user space.

These aren't quite the same ... if I use "user_return_notifier_register()" in my machine check handler, what might happen is that entry_64.S paranoid_userspace may see _TIF_NEED_RESCHED, and call schedule. Now my "i don't want this to run" process could be picked up by a different cpu that doesn't have the notifier registered.

The big clue was

     head = &get_cpu_var(return_notifier_list);

in fire_user_return_notifiers()

But I wonder if I'm misreading the code - I'm not quite certain what the kvm code is trying to do when using this, but it looks to me that it might also suffer from the resched and migrate to another cpu possibility.

-Tony

2011/5/25 Tony Luck <[email protected]>:

I didn't notice propagate_user_return_notify() ... which runs during context switch to move TIF_USER_RETURN_NOTIFY from the old process to the next one. This deals with the resched problem.

So this really is an "execute this function when the next task tries to return to user mode" without any regard to *which* task is being piggy-backed. It might be the one for which we first set TIF_USER_RETURN_NOTIFY, or it might be some totally different task many context switches later (if we happen to run a bunch of kernel daemons that don't return to user mode).

This could work for the current usage of TIF_MCE_NOTIFY (which looks like it is just trying to get some task to push the error logs along their path to /dev/mcelog, and process any "action optional" faults that have recorded page numbers of memory that needs to be looked at, but for which there is no rush.

But it doesn't work for the "action required" case where I need to stop the task that hit the error from running, and figure out the virtual address that it is using to map the physical address that the h/w reported as having an error. To get this behavior I'd need the list of functions to call have its head in the task structure (so the list is attached to a specific task). But I'd need another TIF bit (unless I can re-purpose TIF_MCE_NOTIFY). It would get rid of the task->mce_error_pfn (since I'd pass parameters in the structure that is the container of the one that links onto the list).

Would such a change be worth prototyping?

-Tony