x86: mce: Bugfixes, cleanups and a new CMCI poll version

Split timer init function into the init and the start part, so the start part can replace the open coded version in CPU_DOWN_FAILED.

Signed-off-by Thomas Gleixner <[email protected]>

arch/x86/kernel/cpu/mcheck/mce.c +2 -3

@@ -1554,23 +1554,28 @@ static void __mcheck_cpu_init_vendor(str
 	}
 }
 
-static void __mcheck_cpu_init_timer(void)
+static void mce_start_timer(unsigned int cpu, struct timer_list *t)
 {
-	struct timer_list *t = &__get_cpu_var(mce_timer);
 	unsigned long iv = check_interval * HZ;
 
-	setup_timer(t, mce_timer_fn, smp_processor_id());
+	__this_cpu_write(mce_next_interval, iv);

No point in having double cases if we can simply mask the FROZEN bit out.

Signed-off-by Thomas Gleixner <[email protected]>

Index: tip/arch/x86/kernel/cpu/mcheck/mce_intel.c
===================================================================

arch/x86/kernel/cpu/mcheck/mce_intel.c +2 -0

@@ -15,6 +15,8 @@
 #include <asm/msr.h>
 #include <asm/mce.h>
 
+#include "mce-internal.h"
+
 /*
  * Support for Intel Correct Machine Check Interrupts. This allows
  * the CPU to raise an interrupt when a corrected machine check happened.

arch/x86/kernel/cpu/mcheck/mce_intel.c +60 -0

+
+static DEFINE_PER_CPU(unsigned long, cmci_time_stamp);
+static DEFINE_PER_CPU(unsigned int, cmci_storm_cnt);
+static DEFINE_PER_CPU(unsigned int, cmci_storm_state);
+
+enum {
+	CMCI_STORM_NONE,
+	CMCI_STORM_ACTIVE,
+	CMCI_STORM_SUBSIDED,
+};
+
+static atomic_t cmci_storm_on_cpus;
 
 static int cmci_supported(int *banks)
 {

@@ -53,6 +70,73 @@ static int cmci_supported(int *banks)
 	return !!(cap & MCG_CMCI_P);
 }
 
+void mce_intel_cmci_poll(void)
+{
+	if (__this_cpu_read(cmci_storm_state) == CMCI_STORM_NONE)
+		return;
+	machine_check_poll(MCP_TIMESTAMP, &__get_cpu_var(mce_banks_owned));
+}
+
+unsigned long mce_intel_adjust_timer(unsigned long interval)
+{
+	if (interval < CMCI_POLL_INTERVAL)
+		return interval;
+
+	switch (__this_cpu_read(cmci_storm_state)) {
+	case CMCI_STORM_ACTIVE:
+		/*
+		 * We switch back to interrupt mode once the poll timer has
+		 * silenced itself. That means no events recorded and the
+		 * timer interval is back to our poll interval.
+		 */
+		__this_cpu_write(cmci_storm_state, CMCI_STORM_SUBSIDED);
+		atomic_dec(&cmci_storm_on_cpus);
+
+	case CMCI_STORM_SUBSIDED:
+		/*
+		 * We wait for all cpus to go back to SUBSIDED
+		 * state. When that happens we switch back to
+		 * interrupt mode.
+		 */
+		if (!atomic_read(&cmci_storm_on_cpus)) {
+			__this_cpu_write(cmci_storm_state, CMCI_STORM_NONE);
+			cmci_reenable();
+			cmci_recheck();
+		}
+		return CMCI_POLL_INTERVAL;
+	default:
+		/*
+		 * We have shiny wheather, let the poll do whatever it
+		 * thinks.
+		 */
+		return interval;
+	}
+}
+
+static bool cmci_storm_detect(void)
+{
+	unsigned int cnt = __this_cpu_read(cmci_storm_cnt);
+	unsigned long ts = __this_cpu_read(cmci_time_stamp);
+	unsigned long now = jiffies;

arch/x86/kernel/cpu/mcheck/mce_intel.c +11 -0

+		return false;
+
+	cmci_clear();
+	__this_cpu_write(cmci_storm_state, CMCI_STORM_ACTIVE);
+	atomic_inc(&cmci_storm_on_cpus);
+	mce_timer_kick(CMCI_POLL_INTERVAL);
+	return true;
+}
+
 /*
  * The interrupt handler. This is called on every event.
  * Just call the poller directly to log any events.

@@ -61,6 +145,8 @@ static int cmci_supported(int *banks)
  */
 static void intel_threshold_interrupt(void)
 {
+	if (cmci_storm_detect())
+		return;
 	machine_check_poll(MCP_TIMESTAMP, &__get_cpu_var(mce_banks_owned));
 	mce_notify_irq();
 }

Sorry for the resend, I guess I need more sleep or vacation or both :(

The following series fixes a few interesting bugs (found by review in context of the CMCI poll effort) and a cleanup to the timer/hotplug code followed by a consolidated version of the CMCI poll implementation. This series is based on

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git

which contains the bugfix for the dropped timer interval init.

Thanks,

tglx

But during the CPU online/offline test I found an issue. After *STORM* mode is entered, it can't come back from *STORM* mode to normal interrupt mode. At least there exists such an issue: when *STORM* is entered, in the meanwhile, one CPU is offline during this period, which means *cmci_storm_on_cpus* can't decrease to 0 because there is one bit stuck on this offlined CPU.

Stupid question: do you even want to allow offlining of CPUs while the storm is raging? IOW, wouldn't it be better to disallow hotplug when the storm happens so that all online CPUs, which is all CPUs on the system, can work out the storm conditions faster so that the system gets back to normal faster...

But during the CPU online/offline test I found an issue. After *STORM* mode is entered, it can't come back from *STORM* mode to normal interrupt mode. At least there exists such an issue: when *STORM* is entered, in the meanwhile, one CPU is offline during this period, which means *cmci_storm_on_cpus* can't decrease to 0 because there is one bit stuck on this offlined CPU. So we should detect such situation and decrease on *cmci_storm_on_cpus* at proper time.

BTW, even I online the *CPU* in above situation, the normal CMCI still doesn't come back, strange.

I still have another question: When we handle following case:
mce_cpu_callback(struct notifier_block *

		mce_device_remove(cpu);
		break;
	case CPU_DOWN_PREPARE:

(quoted diff) +0 -1

-		del_timer_sync(t);

		smp_call_function_single(cpu, mce_disable_cpu, &action, 1);

(quoted diff) +1 -0

+ 		del_timer_sync(t);

		break;

Where we add this timer back? I can't find it in "case CPU_ONLINE".

Quoted 8 lines · Chen Gong

But during the CPU online/offline test I found an issue. After *STORM* mode is entered, it can't come back from *STORM* mode to normal interrupt mode. At least there exists such an issue: when *STORM* is entered, in the meanwhile, one CPU is offline during this period, which means *cmci_storm_on_cpus* can't decrease to 0 because there is one bit stuck on this offlined CPU. So we should detect such situation and decrease on *cmci_storm_on_cpus* at proper time.

Yes, we need to reset the storm state as well I think.

Quoted 2 lines

BTW, even I online the *CPU* in above situation, the normal CMCI still doesn't come back, strange.

That's weird.

Quoted 11 lines

I still have another question: When we handle following case:
mce_cpu_callback(struct notifier_block *

		mce_device_remove(cpu);
		break;
	case CPU_DOWN_PREPARE:

(quoted diff) +0 -1

-		del_timer_sync(t);

		smp_call_function_single(cpu, mce_disable_cpu, &action, 1);

(quoted diff) +1 -0

+ 		del_timer_sync(t);

		break;

Quoted 1 line

Where we add this timer back? I can't find it in "case CPU_ONLINE".

The timer gets added back via mcheck_cpu_init(), which is called on the newly onlined cpu from smp_callin().

Thanks,

tglx

Quoted 8 lines · Chen Gong

But during the CPU online/offline test I found an issue. After *STORM* mode is entered, it can't come back from *STORM* mode to normal interrupt mode. At least there exists such an issue: when *STORM* is entered, in the meanwhile, one CPU is offline during this period, which means *cmci_storm_on_cpus* can't decrease to 0 because there is one bit stuck on this offlined CPU. So we should detect such situation and decrease on *cmci_storm_on_cpus* at proper time.

Yes, we need to reset the storm state as well I think.

Quoted 2 lines

BTW, even I online the *CPU* in above situation, the normal CMCI still doesn't come back, strange.

That's weird.

Quoted 11 lines

I still have another question: When we handle following case:
mce_cpu_callback(struct notifier_block *

		mce_device_remove(cpu);
		break;
	case CPU_DOWN_PREPARE:

(quoted diff) +0 -1

-		del_timer_sync(t);

		smp_call_function_single(cpu, mce_disable_cpu, &action, 1);

(quoted diff) +1 -0

+ 		del_timer_sync(t);

		break;

Quoted 1 line

Where we add this timer back? I can't find it in "case CPU_ONLINE".

The timer gets added back via mcheck_cpu_init(), which is called on the newly onlined cpu from smp_callin().

Thanks,

tglx

this patch is based on tip tree and previous 5 patches.

Quoted 1 line · Chen Gong

this patch is based on tip tree and previous 5 patches.

You really don't need all this complexity to handle that. The main thing is that you clear the storm state and adjust the storm counter when the cpu goes offline (in case the state is ACTIVE).

When it comes online again then you can simply let it restart cmci. If the storm on this cpu (or node) still exists then it will notice and everything falls in place.

Quoted 10 lines · Thomas Gleixner

Quoted 1 line · Chen Gong

this patch is based on tip tree and previous 5 patches.

You really don't need all this complexity to handle that. The main thing is that you clear the storm state and adjust the storm counter when the cpu goes offline (in case the state is ACTIVE).

When it comes online again then you can simply let it restart cmci. If the storm on this cpu (or node) still exists then it will notice and everything falls in place.

I ever tested some different scenarios, if storm on this cpu still exists, it triggers the CMCI and broadcast it on the sibling CPU, which means the counter *cmci_storm_on_cpus* will increase beyond the upper limit. E.g. on a 2 sockets SandyBridge-EP system (one socket has 8 cores and 16 threads), inject one error on one socket, you can watch *cmci_storm_on_cpus* = 16 becuase of CMCI broadcast, during this time, offline and online one CPU on this socket, firstly *cmci_storm_on_cpus* = 15 because of offline and ACTIVE status, and then *cmci_storm_on_cpus* = 31 in that CMCI is actived because of online.That's why I have to disable CMCI during whole online/offline until CMCI storm is subsided. Frankly, the logic is a little bit complex so that I write many comments to avoid I forget it after some time :-)

So according to your explanation that would cause the cmci vector to be broadcasted to CPU0 as well. Now that would cause the counter to get a bogus increment, right ?

So instead of hacking insane crap into the code, we have simply to do the obvious Right Thing:

Index: linux-2.6/arch/x86/kernel/cpu/mcheck/mce_intel.c
===================================================================

arch/x86/kernel/cpu/mcheck/mce_intel.c +3 -0

@@ -119,6 +119,9 @@ static bool cmci_storm_detect(void)
  	unsigned long ts = __this_cpu_read(cmci_time_stamp);
  	unsigned long now = jiffies;
 
+	if (__this_cpu_read(cmci_storm_state) != CMCI_STORM_NONE)
+		return true;
+
  	if (time_before_eq(now, ts + CMCI_STORM_INTERVAL)) {
  		cnt++;
  	} else {

That will prevent damage under all circumstances, cpu hotplug included.

But that's too simple and comprehensible I fear.

arch/x86/kernel/cpu/mcheck/mce_intel.c static DEFINE_RAW_SPINLOCK(cmci_discover_lock); · line 43

Quoted 8 lines · Chen Gong

index 92d8b5c..0493525 100644

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -0

@@ -40,6 +40,7 @@ static DEFINE_RAW_SPINLOCK(cmci_discover_lock);
  static DEFINE_PER_CPU(unsigned long, cmci_time_stamp);
  static DEFINE_PER_CPU(unsigned int, cmci_storm_cnt);
  static DEFINE_PER_CPU(unsigned int, cmci_storm_state);
+static DEFINE_PER_CPU(unsigned long, cmci_storm_hcpu_status);

Why do you insist on having another status variable, which does actually nothing than obfuscate the code?

Look at the usage sites:

arch/x86/kernel/cpu/mcheck/mce_intel.c unsigned long mce_intel_adjust_timer(unsigned long interval) · line 110

Quoted 2 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -0

  		__this_cpu_write(cmci_storm_state, CMCI_STORM_SUBSIDED);
+		__this_cpu_write(cmci_storm_hcpu_status, CMCI_STORM_HCPU_NONE);

Quoted 13 lines · Thomas Gleixner

arch/x86/kernel/cpu/mcheck/mce_intel.c static DEFINE_RAW_SPINLOCK(cmci_discover_lock); · line 43

Quoted 8 lines · Chen Gong

index 92d8b5c..0493525 100644

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -0

@@ -40,6 +40,7 @@ static DEFINE_RAW_SPINLOCK(cmci_discover_lock);
  static DEFINE_PER_CPU(unsigned long, cmci_time_stamp);
  static DEFINE_PER_CPU(unsigned int, cmci_storm_cnt);
  static DEFINE_PER_CPU(unsigned int, cmci_storm_state);
+static DEFINE_PER_CPU(unsigned long, cmci_storm_hcpu_status);

Why do you insist on having another status variable, which does actually nothing than obfuscate the code?

Quoted 5 lines

Look at the usage sites:

arch/x86/kernel/cpu/mcheck/mce_intel.c static DEFINE_RAW_SPINLOCK(cmci_discover_lock); · line 46

Quoted 3 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -0

  		__this_cpu_write(cmci_storm_state, CMCI_STORM_SUBSIDED);
+		__this_cpu_write(cmci_storm_hcpu_status,
 CMCI_STORM_HCPU_NONE);

Because here I can't substitute CMCI_STORM_HCPU_NONE with CMCI_STORM_SUBSIDED. If so, the status is chaos.

Quoted 14 lines · Chen Gong

Quoted 13 lines · Thomas Gleixner

arch/x86/kernel/cpu/mcheck/mce_intel.c static DEFINE_RAW_SPINLOCK(cmci_discover_lock); · line 43

Quoted 8 lines · Chen Gong

index 92d8b5c..0493525 100644

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -0

@@ -40,6 +40,7 @@ static DEFINE_RAW_SPINLOCK(cmci_discover_lock);
   static DEFINE_PER_CPU(unsigned long, cmci_time_stamp);
   static DEFINE_PER_CPU(unsigned int, cmci_storm_cnt);
   static DEFINE_PER_CPU(unsigned int, cmci_storm_state);
+static DEFINE_PER_CPU(unsigned long, cmci_storm_hcpu_status);

Why do you insist on having another status variable, which does actually nothing than obfuscate the code?

Quoted 8 lines

Quoted 5 lines

Look at the usage sites:

arch/x86/kernel/cpu/mcheck/mce_intel.c static DEFINE_RAW_SPINLOCK(cmci_discover_lock); · line 46

Quoted 3 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -0

   		__this_cpu_write(cmci_storm_state, CMCI_STORM_SUBSIDED);
+		__this_cpu_write(cmci_storm_hcpu_status,
 CMCI_STORM_HCPU_NONE);

Because here I can't substitute CMCI_STORM_HCPU_NONE with CMCI_STORM_SUBSIDED. If so, the status is chaos.

Have you actually read what I wrote later? You do not neeed that extra state, period.

v3->v1
Thanks very much for Thomas' suggestion to simply the whole logic.

Signed-off-by Chen Gong <[email protected]>

---
  arch/x86/kernel/cpu/mcheck/mce-internal.h |    2 ++
  arch/x86/kernel/cpu/mcheck/mce.c          |    1 +
  arch/x86/kernel/cpu/mcheck/mce_intel.c    |   11 +++++++++++
  3 files changed, 14 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce-internal.h +2 -0

index 2cd73ce..6a05c1d 100644
@@ -31,9 +31,11 @@ extern struct mce_bank *mce_banks;
  #ifdef CONFIG_X86_MCE_INTEL
  unsigned long mce_intel_adjust_timer(unsigned long interval);
  void mce_intel_cmci_poll(void);
+void mce_intel_hcpu_update(unsigned long cpu);
  #else
  # define mce_intel_adjust_timer mce_adjust_timer_default
  static inline void mce_intel_cmci_poll(void) { }
+static inline void mce_intel_hcpu_update(unsigned long cpu) { }
  #endif
 
  void mce_timer_kick(unsigned long interval);

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index e3f8b94..5e22d99 100644
@@ -2306,6 +2306,7 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
  		if (threshold_cpu_callback)
  			threshold_cpu_callback(action, cpu);
  		mce_device_remove(cpu);
+		mce_intel_hcpu_update(cpu);
  		break;
  	case CPU_DOWN_PREPARE:
  		smp_call_function_single(cpu, mce_disable_cpu, &action, 1);

arch/x86/kernel/cpu/mcheck/mce_intel.c +8 -0

index 92d8b5c..693bc7d 100644
@@ -77,6 +77,14 @@ void mce_intel_cmci_poll(void)
  	machine_check_poll(MCP_TIMESTAMP, &__get_cpu_var(mce_banks_owned));
  }
 
+void mce_intel_hcpu_update(unsigned long cpu)
+{
+	if (per_cpu(cmci_storm_state, cpu) == CMCI_STORM_ACTIVE)
+		atomic_dec(&cmci_storm_on_cpus);
+
+	per_cpu(cmci_storm_state, cpu) = CMCI_STORM_NONE;
+}
+
  unsigned long mce_intel_adjust_timer(unsigned long interval)
  {
  	if (interval < CMCI_POLL_INTERVAL)

Quoted 63 lines · Chen Gong

v3->v1
Thanks very much for Thomas' suggestion to simply the whole logic.

Signed-off-by Chen Gong <[email protected]>

---
  arch/x86/kernel/cpu/mcheck/mce-internal.h |    2 ++
  arch/x86/kernel/cpu/mcheck/mce.c          |    1 +
  arch/x86/kernel/cpu/mcheck/mce_intel.c    |   11 +++++++++++
  3 files changed, 14 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce-internal.h +2 -0

index 2cd73ce..6a05c1d 100644
@@ -31,9 +31,11 @@ extern struct mce_bank *mce_banks;
  #ifdef CONFIG_X86_MCE_INTEL
  unsigned long mce_intel_adjust_timer(unsigned long interval);
  void mce_intel_cmci_poll(void);
+void mce_intel_hcpu_update(unsigned long cpu);
  #else
  # define mce_intel_adjust_timer mce_adjust_timer_default
  static inline void mce_intel_cmci_poll(void) { }
+static inline void mce_intel_hcpu_update(unsigned long cpu) { }
  #endif
 
  void mce_timer_kick(unsigned long interval);

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index e3f8b94..5e22d99 100644
@@ -2306,6 +2306,7 @@ mce_cpu_callback(struct notifier_block *nfb,
 unsigned long action, void *hcpu)
          if (threshold_cpu_callback)
              threshold_cpu_callback(action, cpu);
          mce_device_remove(cpu);
+        mce_intel_hcpu_update(cpu);
          break;
      case CPU_DOWN_PREPARE:

          smp_call_function_single(cpu, mce_disable_cpu, &action, 1);

arch/x86/kernel/cpu/mcheck/mce_intel.c +8 -0

index 92d8b5c..693bc7d 100644
@@ -77,6 +77,14 @@ void mce_intel_cmci_poll(void)
      machine_check_poll(MCP_TIMESTAMP, &__get_cpu_var(mce_banks_owned));
  }
 
+void mce_intel_hcpu_update(unsigned long cpu)
+{
+    if (per_cpu(cmci_storm_state, cpu) == CMCI_STORM_ACTIVE)
+        atomic_dec(&cmci_storm_on_cpus);
+
+    per_cpu(cmci_storm_state, cpu) = CMCI_STORM_NONE;
+}
+
  unsigned long mce_intel_adjust_timer(unsigned long interval)
  {
      if (interval < CMCI_POLL_INTERVAL)

arch/x86/kernel/cpu/mcheck/mce_intel.c static bool cmci_storm_detect(void) · line 135

Quoted 9 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +3 -0

@@ -119,6 +127,9 @@ static bool cmci_storm_detect(void)
      unsigned long ts = __this_cpu_read(cmci_time_stamp);
      unsigned long now = jiffies;
 
+    if (__this_cpu_read(cmci_storm_state) != CMCI_STORM_NONE)
+        return true;
+
      if (time_before_eq(now, ts + CMCI_STORM_INTERVAL)) {
          cnt++;
      } else {

Hi, Thomas

How is going on for this patch and whole patch series? I don't know if you have updated it or not.

Are you still care about this thread any more? Any plan to update it? Hope to get your feedback ASAP.

Date: Mon, 16 Jul 2012 10:22:16 +0200 (CEST)
From: Thomas Gleixner <[email protected]>
To: Chen Gong <[email protected]>
cc: [email protected], [email protected], [email protected],
[email protected], [email protected]
Subject: Re: [PATCH V3] tmp patch to fix hotplug issue in CMCI storm
User-Agent: Alpine 2.02 (LFD 1266 2009-07-14)

Quoted 3 lines · Chen Gong

Are you still care about this thread any more? Any plan to update it? Hope to get your feedback ASAP.

Can you please collect the latest series and send it to lkml, Tony and Boris. I think it's ok as is now.

Thanks,

tglx