Messages in this thread | | | Date | Thu, 08 Sep 2011 11:05:44 +0800 | From | Minskey Guo <> | Subject | Re: [PATCH 5/5] mce: recover from "action required" errors reported in data path in usermode |
| |
On 09/07/2011 02:05 PM, Chen Gong wrote: > 于 2011/9/1 6:26, Luck, Tony 写道: >> From: Tony Luck<tony.luck@intel.com> >> >> Two new entries in the mce severity table - one notes that data errors >> observed by innocent bystanders (who happen to share a machine check >> bank with the cpu experiencing the error) should be left alone by using >> the "KEEP" severity. >> >> Then inline in the do_machine_check() handler we process the user-mode >> data error that was marked at MCE_AR_SEVERITY. Even though we are in >> "machine check context" it is almost safe to do so. We have already >> released all the other cpus from rendezvous and we know that the cpu >> with the error was executing user code - so it cannot have interrupts >> locked out, or hold any locks. I.e. this is almost equivalent to a >> page fault. Only difference (and risk) is that on x86_64 we are still >> on the machine check stack - so if another machine check arrives, we >> are toast (we didn't clear MCG_STATUS - yet, so cpu will reset rather >> than taking a nested machine check on the same stack). >> >> Signed-off-by: Tony Luck<tony.luck@intel.com> >> --- >> >> Using the "KEEP" state avoids the complexity of my earlier solution >> that sorted the cpus by severity and ran the more serious ones first. >> >> arch/x86/kernel/cpu/mcheck/mce-severity.c | 14 ++++++++++- >> arch/x86/kernel/cpu/mcheck/mce.c | 35 >> ++++++++++++++++++++-------- >> 2 files changed, 38 insertions(+), 11 deletions(-) >> >> diff --git a/arch/x86/kernel/cpu/mcheck/mce-severity.c >> b/arch/x86/kernel/cpu/mcheck/mce-severity.c >> index 7395d5f..c4d8b24 100644 >> --- a/arch/x86/kernel/cpu/mcheck/mce-severity.c >> +++ b/arch/x86/kernel/cpu/mcheck/mce-severity.c >> @@ -54,6 +54,7 @@ static struct severity { >> #define MASK(x, y) .mask = x, .result = y >> #define MCI_UC_S (MCI_STATUS_UC|MCI_STATUS_S) >> #define MCI_UC_SAR (MCI_STATUS_UC|MCI_STATUS_S|MCI_STATUS_AR) >> +#define MCI_ADDR (MCI_STATUS_ADDRV|MCI_STATUS_MISCV) >> #define MCACOD 0xffff >> >> MCESEV( >> @@ -102,11 +103,22 @@ static struct severity { >> SER, BITCLR(MCI_STATUS_S) >> ), >> >> - /* AR add known MCACODs here */ >> MCESEV( >> PANIC, "Action required with lost events", >> SER, BITSET(MCI_STATUS_OVER|MCI_UC_SAR) >> ), >> + >> + /* known AR MCACODs: */ >> + MCESEV( >> + KEEP, "HT thread notices Action required: data load error", >> + SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, >> MCI_UC_SAR|MCI_ADDR|0x0134), >> + MCGMASK(MCG_STATUS_EIPV, 0) >> + ), >> + MCESEV( >> + AR, "Action required: data load error", >> + SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, >> MCI_UC_SAR|MCI_ADDR|0x0134), >> + USER >> + ), > > I don't think *AR* makes sense here because the following codes have a > assumption that it means *user space* condition. If so, in the future > a new *AR* severity for kernel usage is created, we can't distinguish > which one can call "memory_failure" as below. At least, it should have > a suffix such as AR_USER/AR_KERN: > > enum severity_level { > MCE_NO_SEVERITY, > MCE_KEEP_SEVERITY, > MCE_SOME_SEVERITY, > MCE_AO_SEVERITY, > MCE_UC_SEVERITY, > MCE_AR_USER_SEVERITY, > MCE_AR_KERN_SEVERITY, > MCE_PANIC_SEVERITY, > }; > > >> MCESEV( >> PANIC, "Action required: unknown MCACOD", >> SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR) >> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c >> b/arch/x86/kernel/cpu/mcheck/mce.c >> index 135e12d..2c59a34 100644 >> --- a/arch/x86/kernel/cpu/mcheck/mce.c >> +++ b/arch/x86/kernel/cpu/mcheck/mce.c >> @@ -996,12 +996,6 @@ void do_machine_check(struct pt_regs *regs, long >> error_code) >> continue; >> } >> >> - /* >> - * Kill on action required. >> - */ >> - if (severity == MCE_AR_SEVERITY) >> - kill_it = 1; >> - >> mce_read_aux(&m, i); >> >> /* >> @@ -1022,6 +1016,8 @@ void do_machine_check(struct pt_regs *regs, >> long error_code) >> } >> } >> >> + m = *final; >> + >> if (!no_way_out) >> mce_clear_state(toclear); >> >> @@ -1040,7 +1036,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 >> @@ -1049,11 +1045,24 @@ void do_machine_check(struct pt_regs *regs, >> long error_code) >> * high, don't try to do anything at all. >> */ >> >> - if (kill_it&& tolerant< 3) >> + if (worst != MCE_AR_SEVERITY&& kill_it&& tolerant< 3) >> force_sig(SIGBUS, current); >> >> if (worst> 0) >> mce_report_event(regs); >> + >> + if (worst == MCE_AR_SEVERITY) { >> + unsigned long pfn = m.addr>> PAGE_SHIFT; >> + >> + pr_err("Uncorrected hardware memory error in user-access at >> %llx", >> + m.addr); > > print in the MCE handler maybe makes a deadlock ? say, when other CPUs > are printing something, suddently they received MCE broadcast from > Monarch CPU, when Monarch CPU runs above codes, a deadlock happens ? > Please fix me if I miss something :-) > >> + if (__memory_failure(pfn, MCE_VECTOR, 0)< 0) { >> + pr_err("Memory error not recovered"); >> + force_sig(SIGBUS, current); >> + } else >> + pr_err("Memory error recovered"); >> + } > > as you mentioned in the comment, the biggest concern is that when > __memory_failure runs too long, if another MCE happens at the same > time, (assuming this MCE is happened on its sibling CPU which has the > same banks), the 2nd MCE will crash the system. Why not delaying the > process in a safer context, such as using user_return_notifer ? >
besides, I somewhat suspect that calling __memory_failure() in do_machine_check() will cause deadlock.
__memory_failure() handling calls some routines, such as is_free_buddy_page(), which needs to acquire the spin lock, zone->lock. How can we guarantee that other CPUs haven't acquired the lock when receiving #mc broadcast and entering #mc handlers ?
Moreover, there are too many printk in __memory_failure() which can cause deadlock.
-minskey
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |