| Message ID | 20200103150722.20313-7-jschoenh@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mce: Various fixes and cleanups for MCE handling | expand |
On Fri, Jan 03, 2020 at 04:07:22PM +0100, Jan H. Schönherr wrote: > +retry: > + if (has_notifiers) > + blocking_notifier_chain_unregister(&x86_mce_decoder_chain, > + &mce_default_nb); > + else > + blocking_notifier_chain_cond_register(&x86_mce_decoder_chain, > + &mce_default_nb); I get a compile error here: CC arch/x86/kernel/cpu/mce/core.o arch/x86/kernel/cpu/mce/core.c: In function ‘update_default_notifier_registration’: arch/x86/kernel/cpu/mce/core.c:616:3: error: implicit declaration of function ‘blocking_notifier_chain_cond_register’; did you mean ‘blocking_notifier_chain_unregister’? [-Werror=implicit-function-declaration] blocking_notifier_chain_cond_register(&x86_mce_decoder_chain, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ blocking_notifier_chain_unregister -Tony
On Fri, Jan 03, 2020 at 04:07:22PM +0100, Jan H. Schönherr wrote: > The default MCE handler takes action, when no external MCE handler is > registered. Instead of checking for external handlers within the default > MCE handler, only register the default MCE handler when there are no > external handlers in the first place. > > Signed-off-by: Jan H. Schönherr <jschoenh@amazon.de> > --- > New in v2. This is something that became possible due to patch 4. > I'm not entirely happy with it. > > One the one hand, I'm wondering whether there's a nicer way to handle > (de-)registration races. > Instead of unregistering/registering the default notifier depending on whether there are other notifiers, couldn't you just make the default notifier check to see if it should print. E.g. static int mce_default_notifier(struct notifier_block *nb, unsigned long val, void *data) { struct mce *m = (struct mce *)data; if (m && !atomic_read(&num_notifiers)) __print_mce(m); return NOTIFY_DONE; } > On the other hand, I'm starting to question the whole logic to "only print > the MCE if nothing else in the kernel has a handler registered". Is that > really how it should be? For example, there are handlers that filter for a > specific subset of MCEs. If one of those is registered, we're losing all > information for MCEs that don't match. Maybe put control of this into the hands of the notifiers ... if a notifier thinks that it does something useful with the log that makes the console log redundant, then it could call a function to bump the count to suppress the __print_mce(). Simple filter functions on the chain wouldn't do that. If we go this path the variable should be named something like "suppress_console_mce" rather than num_notifiers. Might also be useful to have some command line option or debugfs knob to force printing for those cases where bad stuff is happening and we don't see what was logged before a crash drops all the bits on the floor. -Tony
On Fri, Jan 03, 2020 at 04:07:22PM +0100, Jan H. Schönherr wrote: > On the other hand, I'm starting to question the whole logic to "only print > the MCE if nothing else in the kernel has a handler registered". Is that > really how it should be? Yes, it should be this way: if there are no consumers, all error information should go to dmesg so that it gets printed at least. > For example, there are handlers that filter for a specific subset of > MCEs. If one of those is registered, we're losing all information for > MCEs that don't match. Probably but I don't think there's an example of an actual system where there are no other MCE consumers registered. Not if its users care about RAS. This default fallback was added for the hypothetical case anyway. > A possible solution to the latter would be to have a "handled" or "printed" > flag within "struct mce" and print the MCE based on that in the default > handler. What do you think? Before we go and fix whatever, we need to define what exactly we're fixing. Is there an actual system you're seeing this on or is this something that would never happen in reality? Because if the latter, I don't really care TBH. As in, there's more important stuff to take care of first.
Hi "Jan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v5.5-rc4 next-20191220]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jan-H-Sch-nherr/x86-mce-Various-fixes-and-cleanups-for-MCE-handling/20200104-183135
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git acfe9d882f586288f663aa73209f40e034003c13
config: x86_64-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/x86/kernel/cpu/mce/core.c: In function 'update_default_notifier_registration':
>> arch/x86/kernel/cpu/mce/core.c:616:3: error: implicit declaration of function 'blocking_notifier_chain_cond_register'; did you mean 'blocking_notifier_chain_unregister'? [-Werror=implicit-function-declaration]
blocking_notifier_chain_cond_register(&x86_mce_decoder_chain,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
blocking_notifier_chain_unregister
cc1: some warnings being treated as errors
vim +616 arch/x86/kernel/cpu/mce/core.c
606
607 static void update_default_notifier_registration(void)
608 {
609 bool has_notifiers = !!atomic_read(&num_notifiers);
610
611 retry:
612 if (has_notifiers)
613 blocking_notifier_chain_unregister(&x86_mce_decoder_chain,
614 &mce_default_nb);
615 else
> 616 blocking_notifier_chain_cond_register(&x86_mce_decoder_chain,
617 &mce_default_nb);
618
619 if (has_notifiers != !!atomic_read(&num_notifiers)) {
620 has_notifiers = !has_notifiers;
621 goto retry;
622 }
623 }
624
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
> -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: Friday, January 3, 2020 5:03 PM > To: Jan H. Schönherr <jschoenh@amazon.de> > Cc: Ghannam, Yazen <Yazen.Ghannam@amd.com>; linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org; Tony Luck > <tony.luck@intel.com>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>; > x86@kernel.org > Subject: Re: [PATCH v2 6/6] x86/mce: Dynamically register default MCE handler > > On Fri, Jan 03, 2020 at 04:07:22PM +0100, Jan H. Schönherr wrote: > > On the other hand, I'm starting to question the whole logic to "only print > > the MCE if nothing else in the kernel has a handler registered". Is that > > really how it should be? > > Yes, it should be this way: if there are no consumers, all error > information should go to dmesg so that it gets printed at least. > > > For example, there are handlers that filter for a specific subset of > > MCEs. If one of those is registered, we're losing all information for > > MCEs that don't match. > > Probably but I don't think there's an example of an actual system where > there are no other MCE consumers registered. Not if its users care about > RAS. This default fallback was added for the hypothetical case anyway. > > > A possible solution to the latter would be to have a "handled" or "printed" > > flag within "struct mce" and print the MCE based on that in the default > > handler. What do you think? > > Before we go and fix whatever, we need to define what exactly we're > fixing. Is there an actual system you're seeing this on or is this > something that would never happen in reality? Because if the latter, I > don't really care TBH. As in, there's more important stuff to take care > of first. > I've encountered an issue where EDAC didn't load (either due to a bug or missing hardware enablement) and the MCE got swallowed by the mcelog notifier. The mcelog utility wasn't in use, so there was no record of the MCE. This can be considered a system configuration issue though that can be resolved with a bit of tweaking. But maybe we can find a solution to prevent something like this? Thanks, Yazen
On Wed, Jan 08, 2020 at 04:24:33AM +0000, Ghannam, Yazen wrote: > I've encountered an issue where EDAC didn't load (either due to a > bug or missing hardware enablement) and the MCE got swallowed by the > mcelog notifier. The mcelog utility wasn't in use, so there was no > record of the MCE. Can you reproduce this using the injector?
> -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: Wednesday, January 8, 2020 5:04 AM > To: Ghannam, Yazen <Yazen.Ghannam@amd.com> > Cc: Jan H. Schönherr <jschoenh@amazon.de>; linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org; Tony Luck > <tony.luck@intel.com>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>; > x86@kernel.org > Subject: Re: [PATCH v2 6/6] x86/mce: Dynamically register default MCE handler > > On Wed, Jan 08, 2020 at 04:24:33AM +0000, Ghannam, Yazen wrote: > > I've encountered an issue where EDAC didn't load (either due to a > > bug or missing hardware enablement) and the MCE got swallowed by the > > mcelog notifier. The mcelog utility wasn't in use, so there was no > > record of the MCE. > > Can you reproduce this using the injector? > Yes, I was just able to do it. Here's what I did. 1) Unload EDAC decoder modules. 2) Inject a corrected MCE using mce-inject. 3) Observe "Machine check events logged" message and no other MCE info. 4) Manually load mcelog and find MCE info. It seems to me that the issue is the mcelog notifier counts toward the number of notifiers, so the default notifier doesn't print anything. Of course, this can be avoided if the EDAC modules are loaded, or if mcelog is disabled in the kernel config, or if rasdaemon is used, etc. Thanks, Yazen
> It seems to me that the issue is the mcelog notifier counts toward the number > of notifiers, so the default notifier doesn't print anything. If we gave a API to the notifiers to say whether to suppress printing, then the dev_mcelog() code could do the suppression only if some process had /dev/mcelog open. So if mcelog(8) wasn't running, you'd still see the console message. -Tony
On 09/01/2020 22.54, Luck, Tony wrote: >> It seems to me that the issue is the mcelog notifier counts toward the number >> of notifiers, so the default notifier doesn't print anything. > > If we gave a API to the notifiers to say whether to suppress printing, then the > dev_mcelog() code could do the suppression only if some process had > /dev/mcelog open. So if mcelog(8) wasn't running, you'd still see the console > message. I briefly looked into that. There is the issue that mcelog code buffers MCEs unconditionally. And we probably don't want to deactivate that, so that MCEs during boot can be queried a bit later via /dev/mcelog. We would get a bit of duplicate logging, if we let mcelog report "supress printing" only if there is an actual consumer. (Or if there was a consumer once, in case there are periodically polling consumers.) Regards Jan -- Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index d8fe5b048ee7..3b6e37c5252f 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -156,36 +156,6 @@ void mce_log(struct mce *m) } EXPORT_SYMBOL_GPL(mce_log); -/* - * We run the default notifier as long as we have no external - * notifiers registered on the chain. - */ -static atomic_t num_notifiers; - -static void mce_register_decode_chain_internal(struct notifier_block *nb) -{ - if (WARN_ON(nb->priority > MCE_PRIO_MCELOG && nb->priority < MCE_PRIO_EDAC)) - return; - - blocking_notifier_chain_register(&x86_mce_decoder_chain, nb); -} - -void mce_register_decode_chain(struct notifier_block *nb) -{ - atomic_inc(&num_notifiers); - - mce_register_decode_chain_internal(nb); -} -EXPORT_SYMBOL_GPL(mce_register_decode_chain); - -void mce_unregister_decode_chain(struct notifier_block *nb) -{ - atomic_dec(&num_notifiers); - - blocking_notifier_chain_unregister(&x86_mce_decoder_chain, nb); -} -EXPORT_SYMBOL_GPL(mce_unregister_decode_chain); - static inline u32 ctl_reg(int bank) { return MSR_IA32_MCx_CTL(bank); @@ -606,18 +576,19 @@ static struct notifier_block mce_uc_nb = { .priority = MCE_PRIO_UC, }; +/* + * We run the default notifier as long as we have no external + * notifiers registered on the chain. + */ +static atomic_t num_notifiers; + static int mce_default_notifier(struct notifier_block *nb, unsigned long val, void *data) { struct mce *m = (struct mce *)data; - if (!m) - return NOTIFY_DONE; - - if (atomic_read(&num_notifiers)) - return NOTIFY_DONE; - - __print_mce(m); + if (m) + __print_mce(m); return NOTIFY_DONE; } @@ -628,6 +599,49 @@ static struct notifier_block mce_default_nb = { .priority = MCE_PRIO_LOWEST, }; +static void update_default_notifier_registration(void) +{ + bool has_notifiers = !!atomic_read(&num_notifiers); + +retry: + if (has_notifiers) + blocking_notifier_chain_unregister(&x86_mce_decoder_chain, + &mce_default_nb); + else + blocking_notifier_chain_cond_register(&x86_mce_decoder_chain, + &mce_default_nb); + + if (has_notifiers != !!atomic_read(&num_notifiers)) { + has_notifiers = !has_notifiers; + goto retry; + } +} + +static void mce_register_decode_chain_internal(struct notifier_block *nb) +{ + if (WARN_ON(nb->priority > MCE_PRIO_MCELOG && + nb->priority < MCE_PRIO_EDAC)) + return; + + blocking_notifier_chain_register(&x86_mce_decoder_chain, nb); +} + +void mce_register_decode_chain(struct notifier_block *nb) +{ + atomic_inc(&num_notifiers); + mce_register_decode_chain_internal(nb); + update_default_notifier_registration(); +} +EXPORT_SYMBOL_GPL(mce_register_decode_chain); + +void mce_unregister_decode_chain(struct notifier_block *nb) +{ + atomic_dec(&num_notifiers); + update_default_notifier_registration(); + blocking_notifier_chain_unregister(&x86_mce_decoder_chain, nb); +} +EXPORT_SYMBOL_GPL(mce_unregister_decode_chain); + /* * Read ADDR and MISC registers. */ @@ -1972,7 +1986,7 @@ int __init mcheck_init(void) mce_register_decode_chain_internal(&first_nb); if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) mce_register_decode_chain_internal(&mce_uc_nb); - mce_register_decode_chain_internal(&mce_default_nb); + update_default_notifier_registration(); mcheck_vendor_init_severity(); INIT_WORK(&mce_work, mce_gen_pool_process);
The default MCE handler takes action, when no external MCE handler is registered. Instead of checking for external handlers within the default MCE handler, only register the default MCE handler when there are no external handlers in the first place. Signed-off-by: Jan H. Schönherr <jschoenh@amazon.de> --- New in v2. This is something that became possible due to patch 4. I'm not entirely happy with it. One the one hand, I'm wondering whether there's a nicer way to handle (de-)registration races. On the other hand, I'm starting to question the whole logic to "only print the MCE if nothing else in the kernel has a handler registered". Is that really how it should be? For example, there are handlers that filter for a specific subset of MCEs. If one of those is registered, we're losing all information for MCEs that don't match. A possible solution to the latter would be to have a "handled" or "printed" flag within "struct mce" and print the MCE based on that in the default handler. What do you think? --- arch/x86/kernel/cpu/mce/core.c | 90 ++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 38 deletions(-)