x86: auto poll/interrupt mode switch for CMC to stop CMC storm

This idea is inspired from IA64 implementation. It is like NAPI for network stack. When CMCI is too many to handle, this interrupt can be disabled and then poll mode will take over the events handle. When no more events happen in the system, CMC interrupt can be enabled automatically.

/*
* CPU/chipset specific EDAC code can register a notifier call here to print
* MCE errors in a human-readable form.

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

@@ -582,6 +603,37 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 {
 	struct mce m;
 	int i;

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

+			spin_unlock_irqrestore(&cmc_poll_lock, flag);
+			printk(KERN_WARNING "WARNING: Switching to polling "\
+				"CMC handler; error records may be lost\n");
 

@@ -1253,8 +1320,19 @@ static void mce_start_timer(unsigned long data)
 	n = &__get_cpu_var(mce_next_interval);
 	if (mce_notify_irq())
 		*n = max(*n/2, HZ/100);
-	else
+	else {
 		*n = min(*n*2, (int)round_jiffies_relative(check_interval*HZ));
+		/* if no CMC event, switch out of polling mode */

What's the point of doing this work? Why can't we just do that on the CPU which got hit by the MCE storm and leave the others alone? They either detect it themself or are just not affected.

What's wrong with doing that strictly per cpu and avoid the whole global state horror?

Quoted 3 lines

What's the point of doing this work? Why can't we just do that on the CPU which got hit by the MCE storm and leave the others alone? They either detect it themself or are just not affected.

CMCI gets broadcast to all threads on a socket. So if one cpu has a problem, many cpus have a problem :-( Some machine check banks are local to a thread/core, so we need to make sure that the CMCI gets taken by someone who can actually see the bank with the problem. The others are collateral damage - but this means there is even more reason to do something about a CMCI storm as the effects are not localized.

Quoted 2 lines

What's wrong with doing that strictly per cpu and avoid the whole global state horror?

Is that less of a horror? We'd have some cpus polling and some taking CMCI (in somewhat arbitrary and ever changing combinations). I'm not sure which is less bad.

If that's the case, then I really can't understand the 5 CMCIs per second threshold for defining the storm and switching to poll mode. I'd rather expect 5 of them in a row.

Quoted 3 lines

If that's the case, then I really can't understand the 5 CMCIs per second threshold for defining the storm and switching to poll mode. I'd rather expect 5 of them in a row.

We don't have a lot of science to back up the "5" number (and can change it to conform to any better numbers if someone has some real data).

My general approximation for DRAM corrected error rates is "one per gigabyte per month, plus or minus two orders of
magnitude". So if I saw 1600 errors per month on a 16GB
workstation, I'd think that was a high rate - but still plausible from natural causes (especially if the machine was some place 5000 feet above sea level with a lot less atmosphere to block neutrons). That only amounts to a couple of errors per hour. So five in a second is certainly a storm!

Looking at this from another perspective ... how many CMCIs can we take per second before we start having a noticeable impact on system performance. RT answer may be quite a small number, generic throughput computing answer might be several hundred per second.

The situation we are trying to avoid is a stuck bit on some very frequently accessed piece of memory generating a solid stream of CMCI that make the system unusable. In this case the question is for how long do we let the storm rage before we turn of CMCI to get some real work done.

Once we are in polling mode, we do lose data on the location of some corrected errors. But I don't think that this is too serious. If there are few errors, we want to know about them all. If there are so many that we have difficulty counting them all - then sampling from a subset will give us reasonable data most of the time (the exception being the case where we have one error source that is 100,000 times as noisy as some other sources that we'd still like to keep tabs on ... we'll need a *lot* of samples to see the quieter error sources amongst the noise).

So I think there are justifications for numbers in the 2..1000 range. We could punt it to the user by making it configurable/tunable ... but I think we already have too many tunables that end-users don't have enough information to really set in meaningful ways to meet their actual needs - so I'd prefer to see some "good enough" number that meets the needs, rather than yet another /sys/... file that people can tweak.

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi, Boris, when I write these codes I don't care if it is specific for Intel or AMD.

I just noticed it should be general for x86 platform and all related codes are general too, which in mce.c, so I think it should be fine to place the codes in mce.c.

Quoted 2 lines · Chen Gong

Hi, Boris, when I write these codes I don't care if it is specific for Intel or AMD.

Well, but I do care so that when you leave and start doing something else, people after you can still read and maintain that code.

Quoted 3 lines

I just noticed it should be general for x86 platform and all related codes are general too, which in mce.c, so I think it should be fine to place the codes in mce.c.

Are you kidding me? Only Intel has CMCI.

Now, if some other vendor needs correctable errors interrupt rate throttling, they can carve it out, make it generic, and move it to mce.c.

Otherwise, it belongs in mce_intel.c. For the same reason AMD error thresholding code belongs to mce_amd.c.

Jeez.

Quoted 11 lines · Borislav Petkov

Quoted 2 lines · Chen Gong

Hi, Boris, when I write these codes I don't care if it is specific for Intel or AMD.

Well, but I do care so that when you leave and start doing something else, people after you can still read and maintain that code.

Quoted 3 lines

I just noticed it should be general for x86 platform and all related codes are general too, which in mce.c, so I think it should be fine to place the codes in mce.c.

Are you kidding me? Only Intel has CMCI.

Quoted 8 lines

Now, if some other vendor needs correctable errors interrupt rate throttling, they can carve it out, make it generic, and move it to mce.c.

Otherwise, it belongs in mce_intel.c. For the same reason AMD error thresholding code belongs to mce_amd.c.

Jeez.

Sorry, I'm really not familiar with AMD's CPU. But I still consider these codes should be in
current place. Because the original poll timer logic is there, and my patch is just the
extension for poll timer. Even if moving these codes to Intel specific file, it should be
another patch to move whole logic including poll timer/CMCI handler to Intel specific
file, do you agree?

Quoted 2 lines · Chen Gong

Hi, Boris, when I write these codes I don't care if it is specific for Intel or AMD.

Well, but I do care so that when you leave and start doing something else, people after you can still read and maintain that code.

Quoted 3 lines

I just noticed it should be general for x86 platform and all related codes are general too, which in mce.c, so I think it should be fine to place the codes in mce.c.

Are you kidding me? Only Intel has CMCI.

Now, if some other vendor needs correctable errors interrupt rate throttling, they can carve it out, make it generic, and move it to mce.c.

Otherwise, it belongs in mce_intel.c. For the same reason AMD error thresholding code belongs to mce_amd.c.

Aside of that machine_check_poll is called from other places as well. So looking at mce_timer_start() which is surprisingly the timer callback:

The poll timer rate is self adjusting to intervals down to HZ/100. So when you get into a state where the timer rate becomes lower than HZ/5 we'll trigger that CMCI storm in software and queue work even on machines which do not support CMCI or have it disabled. Brilliant, isn't it?

So that rate check belongs into intel_treshold_interrupt() and wants a intel specific callback in mce_start_timer() to undo it.

Aside of that machine_check_poll is called from other places as well. So looking at mce_timer_start() which is surprisingly the timer callback:

The poll timer rate is self adjusting to intervals down to HZ/100. So when you get into a state where the timer rate becomes lower than HZ/5 we'll trigger that CMCI storm in software and queue work even on machines which do not support CMCI or have it disabled. Brilliant, isn't it?

So that rate check belongs into intel_treshold_interrupt() and wants a intel specific callback in mce_start_timer() to undo it.

Quoted 3 lines

If that's the case, then I really can't understand the 5 CMCIs per second threshold for defining the storm and switching to poll mode. I'd rather expect 5 of them in a row.

We don't have a lot of science to back up the "5" number (and can change it to conform to any better numbers if someone has some real data).

needs - so I'd prefer to see some "good enough" number that meets the needs, rather than yet another /sys/... file that people can tweak.

So can you please explain how this is better than having this strict per cpu and avoid all the mess which comes with that patch? The approach of letting global state be modified in a random manner is just doomed.

Quoted 4 lines

So can you please explain how this is better than having this strict per cpu and avoid all the mess which comes with that patch? The approach of letting global state be modified in a random manner is just doomed.

Well doomed sounds bad :-) ... and I think I now agree that we should get rid of global state and have polling vs. CMCI mode be per-cpu. It means that it will take fractionally longer to react to a storm, but on the plus side we'll naturally set storm mode on just the cpus that are seeing it on a multi-socket system without having to check topology data ... which should be better for the case where a noisy source of CMCI is plaguing one socket, while other sockets have some much lower rate of CMCI that we'd still like to log.

This idea is inspired from IA64 implementation. It is like NAPI for network stack. When CMCI is too many to handle, this interrupt can be disabled and then poll mode will take over the events handle. When no more events happen in the system, CMC interrupt can be enabled automatically.

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

---
 arch/x86/kernel/cpu/mcheck/mce.c |   83 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 2 deletions(-)

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

index d086a09..6334f0d 100644
@@ -92,6 +92,7 @@ static char			*mce_helper_argv[2] = { mce_helper, NULL };
 
 static DECLARE_WAIT_QUEUE_HEAD(mce_chrdev_wait);
 
+static DEFINE_PER_CPU(struct timer_list, mce_timer);
 static DEFINE_PER_CPU(struct mce, mces_seen);
 static int			cpu_missing;
 

@@ -100,8 +101,28 @@ DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = {
 	[0 ... BITS_TO_LONGS(MAX_NR_BANKS)-1] = ~0UL
 };
 
+#define CMC_POLL_INTERVAL	(1 * 30)
+#define CMC_STORM		5
+static DEFINE_PER_CPU(int, cmci_storm_warning);
+static DEFINE_PER_CPU(unsigned long, first_cmci_jiffie);
+static DEFINE_SPINLOCK(cmc_poll_lock);
+
+/*
+ * This variable tells whether we are in cmci-storm-happened mode.
+ * Start with this in the wrong state so we won't play w/ timers
+ * before the system is ready.
+ */
+static int cmci_storm_detected = 1;
+
 static DEFINE_PER_CPU(struct work_struct, mce_work);
 
+static void mce_disable_cmci(void *data);
+static void mce_enable_ce(void *all);
+static void cmc_disable_keventd(struct work_struct *dummy);
+static void cmc_enable_keventd(struct work_struct *dummy);
+
+static DECLARE_WORK(cmc_disable_work, cmc_disable_keventd);
+static DECLARE_WORK(cmc_enable_work, cmc_enable_keventd);
 /*
  * CPU/chipset specific EDAC code can register a notifier call here to print
  * MCE errors in a human-readable form.

@@ -582,6 +603,37 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 {
 	struct mce m;
 	int i;
+	unsigned long flag;
+
+	spin_lock_irqsave(&cmc_poll_lock, flag);
+	if (cmci_storm_detected == 0) {
+		unsigned long now = jiffies;
+		int *count = &__get_cpu_var(cmci_storm_warning);
+		unsigned long *history = &__get_cpu_var(first_cmci_jiffie);
+
+		if (time_before_eq(now, *history + HZ))
+			(*count)++;
+		else {
+			*count = 0;
+			*history = now;
+		}
+
+		if (*count >= CMC_STORM) {
+			cmci_storm_detected = 1;
+			/* If we're being hit with CMC interrupts, we won't
+			 * ever execute the schedule_work() below.  Need to
+			 * disable CMC interrupts on this processor now.
+			 */
+			mce_disable_cmci(NULL);
+			if (!work_pending(&cmc_disable_work))
+				schedule_work(&cmc_disable_work);
+			spin_unlock_irqrestore(&cmc_poll_lock, flag);
+			printk(KERN_WARNING "WARNING: Switching to polling "\
+				"CMC handler; error records may be lost\n");
+			goto out;
+		}
+	}
+	spin_unlock_irqrestore(&cmc_poll_lock, flag);
 
 	percpu_inc(mce_poll_count);
 

@@ -628,6 +680,7 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		mce_wrmsrl(MSR_IA32_MCx_STATUS(i), 0);
 	}
 
+out:
 	/*
 	 * Don't clear MCG_STATUS here because it's only defined for
 	 * exceptions.

@@ -1199,6 +1252,20 @@ static void mce_process_work(struct work_struct *dummy)
 		memory_failure(pfn, MCE_VECTOR, 0);
 }
 
+static void cmc_disable_keventd(struct work_struct *dummy)
+{
+	struct timer_list *t = __this_cpu_ptr(&mce_timer);
+
+	on_each_cpu(mce_disable_cmci, NULL, 0);
+	mod_timer(t, jiffies + CMC_POLL_INTERVAL * HZ);
+}
+
+static void cmc_enable_keventd(struct work_struct *dummy)
+{
+	/* don't re-initiate timer */
+	on_each_cpu(mce_enable_ce, NULL, 0);
+}
+
 #ifdef CONFIG_X86_MCE_INTEL
 /***
  * mce_log_therm_throt_event - Logs the thermal throttling event to mcelog

@@ -1232,12 +1299,12 @@ void mce_log_therm_throt_event(__u64 status)
 static int check_interval = 5 * 60; /* 5 minutes */
 
 static DEFINE_PER_CPU(int, mce_next_interval); /* in jiffies */
-static DEFINE_PER_CPU(struct timer_list, mce_timer);
 
 static void mce_start_timer(unsigned long data)
 {
 	struct timer_list *t = &per_cpu(mce_timer, data);
 	int *n;
+	unsigned long flags;
 
 	WARN_ON(smp_processor_id() != data);
 

@@ -1253,8 +1320,19 @@ static void mce_start_timer(unsigned long data)
 	n = &__get_cpu_var(mce_next_interval);
 	if (mce_notify_irq())
 		*n = max(*n/2, HZ/100);
-	else
+	else {
 		*n = min(*n*2, (int)round_jiffies_relative(check_interval*HZ));
+		/* if no CMC event, switch out of polling mode */
+		spin_lock_irqsave(&cmc_poll_lock, flags);
+		if (cmci_storm_detected == 1) {
+			printk(KERN_WARNING "Returning to interrupt driven "\
+					"CMC handler\n");
+			if (!work_pending(&cmc_enable_work))
+				schedule_work(&cmc_enable_work);
+			cmci_storm_detected = 0;
+		}
+		spin_unlock_irqrestore(&cmc_poll_lock, flags);
+	}
 
 	t->expires = jiffies + *n;
 	add_timer_on(t, smp_processor_id());