lkml.org 
[lkml]   [2010]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH -v2 4/7] x86, NMI, Rewrite NMI handler
    On 26.09.10 20:57:03, Huang Ying wrote:
    > The original NMI handler is quite outdated in many aspects. This patch
    > try to fix it.
    >
    > The order to process the NMI sources are changed as follow:
    >
    > notify_die(DIE_NMI_IPI);
    > notify_die(DIE_NMI);
    > /* process io port 0x61 */
    > nmi_watchdog_touch();
    > notify_die(DIE_NMIUNKNOWN);
    > unknown_nmi();
    >
    > DIE_NMI_IPI is used to process CPU specific NMI sources, such as perf
    > event, oprofile, crash IPI, etc. While DIE_NMI is used to process
    > non-CPU-specific NMI sources, such as APEI (ACPI Platform Error
    > Interface) GHES (Generic Hardware Error Source), etc. Non-CPU-specific
    > NMI sources can be processed on any CPU,
    >
    > DIE_NMI_IPI must be processed before DIE_NMI. For example, perf event
    > trigger a NMI on CPU 1, at the same time, APEI GHES trigger another
    > NMI on CPU 0. If DIE_NMI is processed before DIE_NMI_IPI, it is
    > possible that APEI GHES is processed on CPU 1, while unknown NMI is
    > gotten on CPU 0.

    I think macro names DIE_NMI_IPI and DIE_NMI should be swapped as
    e.g. the perf nmi is actually local and non-IPI.

    We might consider to rework the IPI thing completly, but may be in a
    follow-on patch.

    >
    > In this new order of processing, performance sensitive NMI sources
    > such as oprofile or perf event will have better performance because
    > the time consuming IO port reading is done after them.
    >
    > Only one NMI is eaten for each NMI handler call, even for PCI SERR and
    > IOCHK NMIs. Because one NMI should be raised for each of them, eating
    > too many NMI will cause unnecessary unknown NMI.
    >
    > The die value used in NMI sources are fixed accordingly.
    >
    > The NMI handler in the patch is designed by Andi Kleen.
    >
    >
    > v2:
    >
    > - Split process NMI reason (0x61) on non-BSP into another patch
    >
    > Signed-off-by: Huang Ying <ying.huang@intel.com>
    > ---
    > arch/x86/kernel/cpu/perf_event.c | 1
    > arch/x86/kernel/traps.c | 80 +++++++++++++++++++-------------------
    > arch/x86/oprofile/nmi_int.c | 1
    > arch/x86/oprofile/nmi_timer_int.c | 2
    > drivers/char/ipmi/ipmi_watchdog.c | 2
    > drivers/watchdog/hpwdt.c | 2
    > 6 files changed, 43 insertions(+), 45 deletions(-)
    >
    > --- a/arch/x86/kernel/cpu/perf_event.c
    > +++ b/arch/x86/kernel/cpu/perf_event.c
    > @@ -1247,7 +1247,6 @@ perf_event_nmi_handler(struct notifier_b
    > return NOTIFY_DONE;
    >
    > switch (cmd) {
    > - case DIE_NMI:
    > case DIE_NMI_IPI:

    See my comment above. Same is true for oprofile and some other
    handlers below. It isn't an IPI and should be case DIE_NMI: instead.

    > break;
    > case DIE_NMIUNKNOWN:
    > --- a/arch/x86/kernel/traps.c
    > +++ b/arch/x86/kernel/traps.c
    > @@ -354,9 +354,6 @@ io_check_error(unsigned char reason, str
    > static notrace __kprobes void
    > unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
    > {
    > - if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, 2, SIGINT) ==
    > - NOTIFY_STOP)
    > - return;
    > #ifdef CONFIG_MCA
    > /*
    > * Might actually be able to figure out what the guilty party
    > @@ -385,51 +382,54 @@ static notrace __kprobes void default_do
    >
    > cpu = smp_processor_id();

    This should go to if (!cpu) and maybe we drop variable cpu completly.

    >
    > - /* Only the BSP gets external NMIs from the system. */
    > - if (!cpu)
    > - reason = get_nmi_reason();
    > + /*
    > + * CPU-specific NMI must be processed before non-CPU-specific
    > + * NMI, otherwise we may lose it, because the CPU-specific
    > + * NMI can not be detected/processed on other CPUs.
    > + */
    >
    > - if (!(reason & NMI_REASON_MASK)) {
    > - if (notify_die(DIE_NMI_IPI, "nmi_ipi", regs, reason, 2, SIGINT)
    > - == NOTIFY_STOP)
    > - return;
    > + /*
    > + * CPU-specific NMI: send to specific CPU or NMI sources must
    > + * be processed on specific CPU
    > + */
    > + if (notify_die(DIE_NMI_IPI, "nmi_ipi", regs, 0, 2, SIGINT)
    > + == NOTIFY_STOP)
    > + return;
    >
    > -#ifdef CONFIG_X86_LOCAL_APIC

    Are you sure we may drop this option?

    > - if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
    > - == NOTIFY_STOP)
    > - return;
    > + /* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
    > + if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
    > + return;

    As said, IPI and non-IPI are mixed up.

    >
    > -#ifndef CONFIG_LOCKUP_DETECTOR
    > - /*
    > - * Ok, so this is none of the documented NMI sources,
    > - * so it must be the NMI watchdog.
    > - */
    > - if (nmi_watchdog_tick(regs, reason))
    > - return;
    > - if (!do_nmi_callback(regs, cpu))
    > -#endif /* !CONFIG_LOCKUP_DETECTOR */
    > - unknown_nmi_error(reason, regs);
    > -#else
    > - unknown_nmi_error(reason, regs);
    > + if (!cpu) {
    > + reason = get_nmi_reason();
    > + if (reason & NMI_REASON_MASK) {
    > + if (reason & NMI_REASON_SERR)
    > + pci_serr_error(reason, regs);
    > + else if (reason & NMI_REASON_IOCHK)
    > + io_check_error(reason, regs);
    > +#ifdef CONFIG_X86_32
    > + /*
    > + * Reassert NMI in case it became active
    > + * meanwhile as it's edge-triggered:
    > + */
    > + reassert_nmi();
    > #endif
    > + return;
    > + }
    > + }
    >
    > +#if defined(CONFIG_X86_LOCAL_APIC) && !defined(CONFIG_LOCKUP_DETECTOR)
    > + if (nmi_watchdog_tick(regs, reason))
    > return;
    > - }
    > - if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT) == NOTIFY_STOP)
    > + if (do_nmi_callback(regs, smp_processor_id()))
    > return;
    > -
    > - /* AK: following checks seem to be broken on modern chipsets. FIXME */
    > - if (reason & NMI_REASON_SERR)
    > - pci_serr_error(reason, regs);
    > - if (reason & NMI_REASON_IOCHK)
    > - io_check_error(reason, regs);
    > -#ifdef CONFIG_X86_32
    > - /*
    > - * Reassert NMI in case it became active meanwhile
    > - * as it's edge-triggered:
    > - */
    > - reassert_nmi();
    > #endif
    > +
    > + if (notify_die(DIE_NMIUNKNOWN, "nmi_unknown", regs, reason, 2, SIGINT)
    > + == NOTIFY_STOP)
    > + return;
    > +
    > + unknown_nmi_error(reason, regs);
    > }
    >
    > dotraplinkage notrace __kprobes void
    > --- a/arch/x86/oprofile/nmi_int.c
    > +++ b/arch/x86/oprofile/nmi_int.c
    > @@ -64,7 +64,6 @@ static int profile_exceptions_notify(str
    > int ret = NOTIFY_DONE;
    >
    > switch (val) {
    > - case DIE_NMI:
    > case DIE_NMI_IPI:
    > if (ctr_running)
    > model->check_ctrs(args->regs, &__get_cpu_var(cpu_msrs));
    > --- a/arch/x86/oprofile/nmi_timer_int.c
    > +++ b/arch/x86/oprofile/nmi_timer_int.c
    > @@ -25,7 +25,7 @@ static int profile_timer_exceptions_noti
    > int ret = NOTIFY_DONE;
    >
    > switch (val) {
    > - case DIE_NMI:
    > + case DIE_NMI_IPI:
    > oprofile_add_sample(args->regs, 0);
    > ret = NOTIFY_STOP;
    > break;
    > --- a/drivers/char/ipmi/ipmi_watchdog.c
    > +++ b/drivers/char/ipmi/ipmi_watchdog.c
    > @@ -1080,7 +1080,7 @@ ipmi_nmi(struct notifier_block *self, un
    > {
    > struct die_args *args = data;
    >
    > - if (val != DIE_NMI)
    > + if (val != DIE_NMIUNKNOWN)

    As this function handles an nmi and not an unknown nmi, we should keep
    DIE_NMI here and better modify priorities in ipmi_nmi_handler.

    > return NOTIFY_OK;
    >
    > /* Hack, if it's a memory or I/O error, ignore it. */
    > --- a/drivers/watchdog/hpwdt.c
    > +++ b/drivers/watchdog/hpwdt.c
    > @@ -469,7 +469,7 @@ static int hpwdt_pretimeout(struct notif
    > unsigned long rom_pl;
    > static int die_nmi_called;
    >
    > - if (ulReason != DIE_NMI && ulReason != DIE_NMI_IPI)
    > + if (ulReason != DIE_NMIUNKNOWN)

    Same here, but dropping DIE_NMI_IPI.

    -Robert

    > goto out;
    >
    > if (!hpwdt_nmi_decoding)
    > --
    > 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/
    >

    --
    Advanced Micro Devices, Inc.
    Operating System Research Center


    \
     
     \ /
      Last update: 2010-09-27 20:11    [W:0.037 / U:89.400 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site