kgdb, x86: Pull up NMI notifier handler priority

kgdb needs IPI to be sent and handled before perf or anything else NMI, otherwise kgdb hangs with bootup self-tests (found on P4 HT SMP machine). Raise its priority so that we're called first in a notifier chain.

Quoted 4 lines · Cyrill Gorcunov

kgdb needs IPI to be sent and handled before perf or anything else NMI, otherwise kgdb hangs with bootup self-tests (found on P4 HT SMP machine). Raise its priority so that we're called first in a notifier chain.

This is only because P4 perf swallows all the nmis. If that is the case you are arguing to make the perf nmi at the bottom of the priority list, which is probably not where it should be due to its volume.

I am stuck debugging P4 problems again for RHEL-6 and I noticed a small change that is needed (didn't help my problem though) but it looked like an oversight that might help your case.

Cheers,
Don

arch/x86/kernel/cpu/perf_event_p4.c +0 -0

index 3769ac8..d945314 100644

kgdb needs IPI to be sent and handled before perf or anything else NMI, otherwise kgdb hangs with bootup self-tests (found on P4 HT SMP machine). Raise its priority so that we're called first in a notifier chain.

Reported-by Don Zickus <[email protected]>

Tested-by Lin Ming <[email protected]>

CC Jason Wessel <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---

Don, Jason, take a look please.

 arch/x86/kernel/kgdb.c |    7 +++++--

 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/kgdb.c
=====================================================================

arch/x86/kernel/kgdb.c +0 -0

@@ -592,9 +592,12 @@ static struct notifier_block kgdb_notifi
        .notifier_call  = kgdb_notify,
 

Quoted 23 lines · Cyrill Gorcunov

kgdb needs IPI to be sent and handled before perf or anything else NMI, otherwise kgdb hangs with bootup self-tests (found on P4 HT SMP machine). Raise its priority so that we're called first in a notifier chain.

Reported-by Don Zickus <[email protected]>

Tested-by Lin Ming <[email protected]>

CC Jason Wessel <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---

Don, Jason, take a look please.

 arch/x86/kernel/kgdb.c |    7 +++++--

 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/kgdb.c
=====================================================================

arch/x86/kernel/kgdb.c +0 -0

@@ -592,9 +592,12 @@ static struct notifier_block kgdb_notifi
        .notifier_call  = kgdb_notify,
 

arch/x86/kernel/kgdb.c static struct notifier_block kgdb_notifi · line 601

Quoted 10 lines

arch/x86/kernel/kgdb.c +5 -2

        /*
-        * Lowest-prio notifier priority, we want to be notified last:
+        * We might need to send an IPI and
+        * do cpu roundup before anything else
+        * in notifier chain so high priority
+        * is needed.
         */
-       .priority       = NMI_LOCAL_LOW_PRIOR,
+       .priority       = NMI_LOCAL_HIGH_PRIOR,
  };

CC kgdb maillist.

I quote Jason's early email to explain why debugger tends to set the low level of die_notifier.

"The original thinking was that if you are using a low level debugger that you would want to stop on such a event or breakpoint because there is nothing else handling it and your system is about to print an oops message."

For keeping above purpose, I have a "ugly" proposal that splitting kgdb die handling to two parts.

1: The first part with HIGH priority and just handle NMI event,
 if the NMI event is not belong kgdb, it return NOTIFY_DONE and pass to others.

2: The second part with low priority to handling others.

Due to above handling logic could let kgdb source get complexly, I couldn't make sure
it is a suitable method here, if it is OK, I can send a formal patch. :-)

$git diff
-----------
arch/x86/kernel/kgdb.c |   67 +++++++++++++++++++++++++++++++++++------------

 1 files changed, 50 insertions(+), 17 deletions(-)

arch/x86/kernel/kgdb.c +0 -17

index dba0b36..afd18db 100644
@@ -516,23 +516,6 @@ static int __kgdb_notify(struct die_args *args,
 unsigned long cmd)
        struct pt_regs *regs = args->regs;
 
        switch (cmd) {
-       case DIE_NMI:
-               if (atomic_read(&kgdb_active) != -1) {
-                       /* KGDB CPU roundup */
-                       kgdb_nmicallback(raw_smp_processor_id(), regs);
-                       was_in_debug_nmi[raw_smp_processor_id()] = 1;
-                       touch_nmi_watchdog();
-                       return NOTIFY_STOP;
-               }
-               return NOTIFY_DONE;
-
-       case DIE_NMIUNKNOWN:
-               if (was_in_debug_nmi[raw_smp_processor_id()]) {
-                       was_in_debug_nmi[raw_smp_processor_id()] = 0;
-                       return NOTIFY_STOP;
-               }
-               return NOTIFY_DONE;
-
        case DIE_DEBUG:
                if (atomic_read(&kgdb_cpu_doing_single_step) != -1) {

                       if (user_mode(regs))

arch/x86/kernel/kgdb.c +50 -1

@@ -588,6 +571,52 @@ kgdb_notify(struct notifier_block *self, unsigned
 long cmd, void *ptr)
        return ret;
  }
 
+static int
+kgdb_nmi_notify(struct notifier_block *self, unsigned long cmd, void *ptr)
+{
+       unsigned long flags;
+       struct pt_regs *regs = ((struct die_args*)ptr)->regs;
+       int ret = NOTIFY_DONE;
+
+       local_irq_save(flags);
+
+       switch (cmd) {
+       case DIE_NMI:
+               if (atomic_read(&kgdb_active) != -1) {
+                       /* KGDB CPU roundup */
+                       kgdb_nmicallback(raw_smp_processor_id(), regs);
+                       was_in_debug_nmi[raw_smp_processor_id()] = 1;
+                       touch_nmi_watchdog();
+                       ret = NOTIFY_STOP;
+               }
+               break;
+
+       case DIE_NMIUNKNOWN:
+               if (was_in_debug_nmi[raw_smp_processor_id()]) {
+                       was_in_debug_nmi[raw_smp_processor_id()] = 0;
+                       ret = NOTIFY_STOP;
+               }
+               break;
+       default:
+               break;
+       }
+
+       local_irq_restore(flags);
+       return ret;
+}
+
+static struct notifier_block kgdb_nmi_notifier = {
+       .notifier_call  = kgdb_nmi_notify,
+
+       /*
+        * We might need to send an IPI and
+        * do cpu roundup before anything else
+        * in notifier chain so high priority
+        * is needed.
+        */
+       .priority       = NMI_LOCAL_HIGH_PRIOR,
+};
+
  static struct notifier_block kgdb_notifier = {
        .notifier_call  = kgdb_notify,

@@ -605,6 +634,9 @@ static struct notifier_block kgdb_notifier = {
  */
  int kgdb_arch_init(void)
  {
+       int err = register_die_notifier(&kgdb_nmi_notifier);
+       if (err)
+               return err;
        return register_die_notifier(&kgdb_notifier);
  }
 

@@ -673,6 +705,7 @@ void kgdb_arch_exit(void)
                        breakinfo[i].pev = NULL;
                }
        }
+       unregister_die_notifier(&kgdb_nmi_notifier);
        unregister_die_notifier(&kgdb_notifier);
  }
 
--

1.6.0.4

If Jason is ok with such splitting -- I dont mind either ;)

Quoted 6 lines · Dongdong Deng

Quoted 5 lines · Cyrill Gorcunov

kgdb needs IPI to be sent and handled before perf or anything else NMI, otherwise kgdb hangs with bootup self-tests (found on P4 HT SMP machine). Raise its priority so that we're called first in a notifier chain.

The net result. I'll sign-off on the kgdb change and add a TODO item to wait for the jump patching to enter the kernel.

Cyrill, I am assuming this is something we want to aim to merge into the 2.6.39 as a regression fix? I'll try to get a version of Deng Dongdong's patch into linux-next as soon as possible in the mean time.

Cheers,
Jason.

Quoted 9 lines · Cyrill Gorcunov

If Jason is ok with such splitting -- I dont mind either ;)

Quoted 6 lines · Dongdong Deng

Quoted 5 lines · Cyrill Gorcunov

kgdb needs IPI to be sent and handled before perf or anything else NMI, otherwise kgdb hangs with bootup self-tests (found on P4 HT SMP machine). Raise its priority so that we're called first in a notifier chain.

I talked with Cyrill outside the mailing list since he pinged me and I will summarize here.

My initial thought about the patch Deng Dongdong posted was that it was really ugly to have kgdb registered in the notifier chain twice.  I would be willing to live with this for now if we agree that when jump labels are merged to the kernel that we can make use of that instead.

The jump labels would allow us to invoke the debugger directly when the debugger is active much like we do when CONFIG_KGDB_LOW_LEVEL_TRAP is set.  In fact the code that is ifdef'ed with CONFIG_KGDB_LOW_LEVEL_TRAP can make use of the same jump label as the NMI entry and no longer be #ifdef'ed when jump labels come to pass.

The very discussion of the patch raised the question of "why not always have the debugger be first?"  The answer for that lies in that some code needs to run before the debugger to keep the system running assuming you are planning on restarting it after entering the debugger.   The generic die notifier is used for lots of circumstances and the priority the debugger cares about only matter for a select few exception types.

The kmmio, mce-inject, and crash_nmi_nb (from reboot.c) are good examples of in tree code that should run with a higher priority than the debugger because the debugger doesn't know what to do with these code paths, so it sits last in line hoping someone else will deal with the exception else enter the debugger.  For the trap paths the debugger needs to be first in line to deal with the case where where a breakpoint is in a notifier to avoid non-recoverable recursive faults. For NMI it appears we need to run before the perf code or the perf code will eat an nmi event intended for kgdb and result in a dead locked system.

The net result.   I'll sign-off on the kgdb change and add a TODO item to wait for the jump patching to enter the kernel.

Cyrill, I am assuming this is something we want to aim to merge into the 2.6.39 as a regression fix?  I'll try to get a version of Deng Dongdong's patch into linux-next as soon as possible in the mean time.

Quoted 5 lines

The net result. I'll sign-off on the kgdb change and add a TODO item to wait for the jump patching to enter the kernel.

Cyrill, I am assuming this is something we want to aim to merge into the 2.6.39 as a regression fix? I'll try to get a version of Deng Dongdong's patch into linux-next as soon as possible in the mean time.

Hi Jason,

Do I need to send a patch to you? :-)

BR,
Dongdong