| Message ID | 20181026003729.8420-1-vishal.l.verma@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5d96c9342c23ee1d084802dcf064caa67ecaa45b |
| Headers | show |
| Series | [v3,1/2] nfit, mce: only handle uncorrectable machine checks | expand |
Hi, Vishal, Vishal Verma <vishal.l.verma@intel.com> writes: > The mce handler for 'nfit' devices is called for memory errors on a > Non-Volatile DIMM, and adds the error location to a 'badblocks' list. > This list is used by the various NVDIMM drivers to avoid consuming known > poison locations during IO. > > The mce handler gets called for both corrected and uncorrectable errors. Sorry for necroposting. I thought the point of the CEC was to make sure that the other registered decoders only ever saw uncorrected errors. How do we end up getting called with a correctable error? Thanks, Jeff > Until now, both kinds of errors have been added to the badblocks list. > However, corrected memory errors indicate that the problem has already > been fixed by hardware, and the resulting interrupt is merely a > notification to Linux. As far as future accesses to that location are > concerned, it is perfectly fine to use, and thus doesn't need to be > included in the above badblocks list. > > Add a check in the nfit mce handler to filter out corrected mce events, > and only process uncorrectable errors. > > Reported-by: Omar Avelar <omar.avelar@intel.com> > Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error") > Cc: stable@vger.kernel.org > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Borislav Petkov <bp@alien8.de> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > arch/x86/include/asm/mce.h | 1 + > arch/x86/kernel/cpu/mcheck/mce.c | 3 ++- > drivers/acpi/nfit/mce.c | 4 ++-- > 3 files changed, 5 insertions(+), 3 deletions(-) > > v3: Unchanged from v2 > > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h > index 3a17107594c8..3111b3cee2ee 100644 > --- a/arch/x86/include/asm/mce.h > +++ b/arch/x86/include/asm/mce.h > @@ -216,6 +216,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s > > int mce_available(struct cpuinfo_x86 *c); > bool mce_is_memory_error(struct mce *m); > +bool mce_is_correctable(struct mce *m); > > DECLARE_PER_CPU(unsigned, mce_exception_count); > DECLARE_PER_CPU(unsigned, mce_poll_count); > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c > index 953b3ce92dcc..27015948bc41 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce.c > +++ b/arch/x86/kernel/cpu/mcheck/mce.c > @@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m) > } > EXPORT_SYMBOL_GPL(mce_is_memory_error); > > -static bool mce_is_correctable(struct mce *m) > +bool mce_is_correctable(struct mce *m) > { > if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED) > return false; > @@ -544,6 +544,7 @@ static bool mce_is_correctable(struct mce *m) > > return true; > } > +EXPORT_SYMBOL_GPL(mce_is_correctable); > > static bool cec_add_mce(struct mce *m) > { > diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c > index e9626bf6ca29..7a51707f87e9 100644 > --- a/drivers/acpi/nfit/mce.c > +++ b/drivers/acpi/nfit/mce.c > @@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val, > struct acpi_nfit_desc *acpi_desc; > struct nfit_spa *nfit_spa; > > - /* We only care about memory errors */ > - if (!mce_is_memory_error(mce)) > + /* We only care about uncorrectable memory errors */ > + if (!mce_is_memory_error(mce) || mce_is_correctable(mce)) > return NOTIFY_DONE; > > /*
Drop stable@ On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote: > Sorry for necroposting. I thought the point of the CEC was to make sure > that the other registered decoders only ever saw uncorrected errors. Ha, good point! You mean drivers/ras/cec.c, right? If so, then I don't think we've ever talked about connecting CEC with NVDIMM and whether that would make sense. Lemme add Dan. > How do we end up getting called with a correctable error? Good question. We shouldn't. So we need to figure out here how exactly should those things interact and whether NFIT should get all errors reported or it should put all the correctable errors through the decay thing, see the comment at the top of drivers/ras/cec.c Thx for pointing that out Jeff.
Borislav Petkov <bp@alien8.de> writes: > Drop stable@ > > On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote: >> Sorry for necroposting. I thought the point of the CEC was to make sure >> that the other registered decoders only ever saw uncorrected errors. > > Ha, good point! You mean drivers/ras/cec.c, right? Yes. > If so, then I don't think we've ever talked about connecting CEC with > NVDIMM and whether that would make sense. Lemme add Dan. I don't think there's a difference between MCEs for NVDIMMs and normal DRAM. I'll let Dan confirm or deny that. >> How do we end up getting called with a correctable error? > > Good question. We shouldn't. > > So we need to figure out here how exactly should those things interact > and whether NFIT should get all errors reported or it should put all the > correctable errors through the decay thing, see the comment at the top > of drivers/ras/cec.c > > Thx for pointing that out Jeff. Sure, thanks for the quick reply, Boris! Also, thanks for your detailed commit messages. They really help with understanding the code changes. Cheers, Jeff
On Wed, Feb 20, 2019 at 02:26:45PM -0500, Jeff Moyer wrote: > Sure, thanks for the quick reply, Boris! Also, thanks for your detailed > commit messages. They really help with understanding the code changes. Ha! From now on I'll be showing that to *anyone* who complains when I ask of them to make their commit messages understandable! :-) Thanks!
On Wed, Feb 20, 2019 at 11:26 AM Jeff Moyer <jmoyer@redhat.com> wrote: > > Borislav Petkov <bp@alien8.de> writes: > > > Drop stable@ > > > > On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote: > >> Sorry for necroposting. I thought the point of the CEC was to make sure > >> that the other registered decoders only ever saw uncorrected errors. > > > > Ha, good point! You mean drivers/ras/cec.c, right? > > Yes. > > > If so, then I don't think we've ever talked about connecting CEC with > > NVDIMM and whether that would make sense. Lemme add Dan. > > I don't think there's a difference between MCEs for NVDIMMs and normal > DRAM. I'll let Dan confirm or deny that. There is a difference. NVDIMMs have local tracking of discovered poison, methods to scan for latent poison, and methods to clear. A CEC connection, iiuc, would seem an awkward fit. Awkward because what CEC enables is meant to be implemented natively in the hardware, and CEC seems to have no concept of the fact that errors can be repaired.
On Wed, Feb 20, 2019 at 11:40:10AM -0800, Dan Williams wrote: > There is a difference. NVDIMMs have local tracking of discovered > poison, methods to scan for latent poison, and methods to clear. A CEC > connection, iiuc, would seem an awkward fit. Awkward because what CEC > enables is meant to be implemented natively in the hardware, and CEC > seems to have no concept of the fact that errors can be repaired. CEC is a leaky bucket of sorts which does call memory_failure_queue() in the end. So we poison only those errors which report the same address over and over again. Correctable errors are by definition already repaired, i.e., corrected so there's no need to do anything. The way stuff is plumbed now is, all correctable errors go to the CEC so NFIT doesn't see them, if CEC is enabled. But the patch Jeff quoted already changed NFIT to ignore correctable errors so I guess we don't have to do anything. And this is still needed for the case where CEC is not enabled. I'd say.
Dan Williams <dan.j.williams@intel.com> writes: > On Wed, Feb 20, 2019 at 11:26 AM Jeff Moyer <jmoyer@redhat.com> wrote: >> >> Borislav Petkov <bp@alien8.de> writes: >> >> > Drop stable@ >> > >> > On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote: >> >> Sorry for necroposting. I thought the point of the CEC was to make sure >> >> that the other registered decoders only ever saw uncorrected errors. >> > >> > Ha, good point! You mean drivers/ras/cec.c, right? >> >> Yes. >> >> > If so, then I don't think we've ever talked about connecting CEC with >> > NVDIMM and whether that would make sense. Lemme add Dan. >> >> I don't think there's a difference between MCEs for NVDIMMs and normal >> DRAM. I'll let Dan confirm or deny that. > > There is a difference. NVDIMMs have local tracking of discovered > poison, methods to scan for latent poison, and methods to clear. What I meant was that you couldn't tell the difference between an MCE generated by accessing DRAM vs one generated by accessing an NVDIMM (aside from checking the address). > A CEC connection, iiuc, would seem an awkward fit. Awkward because > what CEC enables is meant to be implemented natively in the hardware, > and CEC seems to have no concept of the fact that errors can be > repaired. As far as I can tell, the Correctable Errors Collector just eats correctable errors so that the rest of the registered decoders don't have to worry about receiving them. It sounds like you're suggesting that NVDIMMs won't spew correctable errors. If that's the case (I don't think it is), then there's no need at all for these patches. Anyway, given that the correctable errors collector can be turned off in the kernel config, and assuming that we still can get correctable errors from NVDIMMs (I think we can, since I believe the caching hierarchy can generate them as well), we definitely need to continue to check for correctable errors in the nfit mce decoder. That's something I had overlooked. Cheers, Jeff
On Thu, Feb 21, 2019 at 11:11:27AM -0500, Jeff Moyer wrote: > Anyway, given that the correctable errors collector can be turned off in > the kernel config, Also, on the cmdline, for whatever reason: ras=cec_disable
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index 3a17107594c8..3111b3cee2ee 100644 --- a/arch/x86/include/asm/mce.h +++ b/arch/x86/include/asm/mce.h @@ -216,6 +216,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s int mce_available(struct cpuinfo_x86 *c); bool mce_is_memory_error(struct mce *m); +bool mce_is_correctable(struct mce *m); DECLARE_PER_CPU(unsigned, mce_exception_count); DECLARE_PER_CPU(unsigned, mce_poll_count); diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c index 953b3ce92dcc..27015948bc41 100644 --- a/arch/x86/kernel/cpu/mcheck/mce.c +++ b/arch/x86/kernel/cpu/mcheck/mce.c @@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m) } EXPORT_SYMBOL_GPL(mce_is_memory_error); -static bool mce_is_correctable(struct mce *m) +bool mce_is_correctable(struct mce *m) { if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED) return false; @@ -544,6 +544,7 @@ static bool mce_is_correctable(struct mce *m) return true; } +EXPORT_SYMBOL_GPL(mce_is_correctable); static bool cec_add_mce(struct mce *m) { diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c index e9626bf6ca29..7a51707f87e9 100644 --- a/drivers/acpi/nfit/mce.c +++ b/drivers/acpi/nfit/mce.c @@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val, struct acpi_nfit_desc *acpi_desc; struct nfit_spa *nfit_spa; - /* We only care about memory errors */ - if (!mce_is_memory_error(mce)) + /* We only care about uncorrectable memory errors */ + if (!mce_is_memory_error(mce) || mce_is_correctable(mce)) return NOTIFY_DONE; /*
The mce handler for 'nfit' devices is called for memory errors on a Non-Volatile DIMM, and adds the error location to a 'badblocks' list. This list is used by the various NVDIMM drivers to avoid consuming known poison locations during IO. The mce handler gets called for both corrected and uncorrectable errors. Until now, both kinds of errors have been added to the badblocks list. However, corrected memory errors indicate that the problem has already been fixed by hardware, and the resulting interrupt is merely a notification to Linux. As far as future accesses to that location are concerned, it is perfectly fine to use, and thus doesn't need to be included in the above badblocks list. Add a check in the nfit mce handler to filter out corrected mce events, and only process uncorrectable errors. Reported-by: Omar Avelar <omar.avelar@intel.com> Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error") Cc: stable@vger.kernel.org Cc: Dan Williams <dan.j.williams@intel.com> Cc: Tony Luck <tony.luck@intel.com> Cc: Borislav Petkov <bp@alien8.de> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- arch/x86/include/asm/mce.h | 1 + arch/x86/kernel/cpu/mcheck/mce.c | 3 ++- drivers/acpi/nfit/mce.c | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) v3: Unchanged from v2