[PATCH] x86/mce: Avoid reading every machine check bank register twice.

[PATCH] x86/mce: Avoid reading every machine check bank register twice.

[Luck, Tony]

Reading machine check bank registers is slow. There is a trend of
increasing the number of banks, and the number of cores. The main section
of do_machine_check() is a serialized section where each cpu in turn
checks every bank. Even on a little two socket SandyBridge-EP system
that multiplies out as:

	2 sockets * 8 cores * 2 hyperthreads * 20 banks = 640 MSRs

We already scan the banks in parallel in mce_no_way_out() to see if there
is a fatal error anywhere in the system. If we build a cache of VALID
bits during this scan, we can avoid uselessly re-reading banks that have
no data. Note that this cache is only a hint. If the valid bit is set in a
shared bank, all cpus that share that bank will see it during the parallel
scan, but the first to find it in the sequential scan will (usually) clear
the bank.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
[I'll push via RAS tree later - after fixing the problems you'll no doubt find :-)]

 arch/x86/kernel/cpu/mcheck/mce.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index d086a09..6f25a7c 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -641,14 +641,19 @@ EXPORT_SYMBOL_GPL(machine_check_poll);
  * Do a quick check if any of the events requires a panic.
  * This decides if we keep the events around or clear them.
  */
-static int mce_no_way_out(struct mce *m, char **msg)
+static int mce_no_way_out(struct mce *m, char **msg, unsigned long *hintp)
 {
 	int i;
 
 	for (i = 0; i < banks; i++) {
 		m->status = mce_rdmsrl(MSR_IA32_MCx_STATUS(i));
-		if (mce_severity(m, tolerant, msg) >= MCE_PANIC_SEVERITY)
+		if (m->status & MCI_STATUS_VAL)
+			__set_bit(i, hintp);
+		if (mce_severity(m, tolerant, msg) >= MCE_PANIC_SEVERITY) {
+			while (++i < banks)
+				__set_bit(i, hintp);
 			return 1;
+		}
 	}
 	return 0;
 }
@@ -1011,6 +1016,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	 */
 	int kill_it = 0;
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
+	DECLARE_BITMAP(hint, MAX_NR_BANKS);
 	char *msg = "Unknown";
 
 	atomic_inc(&mce_entry);
@@ -1025,7 +1031,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	final = &__get_cpu_var(mces_seen);
 	*final = m;
 
-	no_way_out = mce_no_way_out(&m, &msg);
+	memset(hint, 0, sizeof(hint));
+	no_way_out = mce_no_way_out(&m, &msg, hint);
 
 	barrier();
 
@@ -1045,6 +1052,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	order = mce_start(&no_way_out);
 	for (i = 0; i < banks; i++) {
 		__clear_bit(i, toclear);
+		if (!test_bit(i, hint))
+			continue;
 		if (!mce_banks[i].ctl)
 			continue;
 
-- 
1.7.9.rc2.1.g69204

Re: [PATCH] x86/mce: Avoid reading every machine check bank register twice.

[Borislav Petkov]

On Tue, Apr 17, 2012 at 11:04:01AM -0700, Luck, Tony wrote:
> Reading machine check bank registers is slow. There is a trend of
> increasing the number of banks, and the number of cores. The main section
> of do_machine_check() is a serialized section where each cpu in turn
> checks every bank. Even on a little two socket SandyBridge-EP system
> that multiplies out as:
> 
> 	2 sockets * 8 cores * 2 hyperthreads * 20 banks = 640 MSRs
> 
> We already scan the banks in parallel in mce_no_way_out() to see if there
> is a fatal error anywhere in the system. If we build a cache of VALID
> bits during this scan, we can avoid uselessly re-reading banks that have
> no data. Note that this cache is only a hint. If the valid bit is set in a
> shared bank, all cpus that share that bank will see it during the parallel
> scan, but the first to find it in the sequential scan will (usually) clear
> the bank.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> [I'll push via RAS tree later - after fixing the problems you'll no doubt find :-)]

Why, is Waldo hidden somewhere in this patch for us to find?

:-)

> 
>  arch/x86/kernel/cpu/mcheck/mce.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index d086a09..6f25a7c 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -641,14 +641,19 @@ EXPORT_SYMBOL_GPL(machine_check_poll);
>   * Do a quick check if any of the events requires a panic.
>   * This decides if we keep the events around or clear them.
>   */
> -static int mce_no_way_out(struct mce *m, char **msg)
> +static int mce_no_way_out(struct mce *m, char **msg, unsigned long *hintp)
>  {
>  	int i;
>  
>  	for (i = 0; i < banks; i++) {
>  		m->status = mce_rdmsrl(MSR_IA32_MCx_STATUS(i));
> -		if (mce_severity(m, tolerant, msg) >= MCE_PANIC_SEVERITY)
> +		if (m->status & MCI_STATUS_VAL)
> +			__set_bit(i, hintp);
> +		if (mce_severity(m, tolerant, msg) >= MCE_PANIC_SEVERITY) {
> +			while (++i < banks)
> +				__set_bit(i, hintp);

I'm guessing this is there so that whenever we have a serious MCE
worth of panic, we want to look at the remaining banks no matter their
validity?

Why not continue through the banks, filling up the hint bitmap but make
a note to yourself here that we'll be returning 1 when finished?

>  			return 1;
> +		}
>  	}
>  	return 0;
>  }
> @@ -1011,6 +1016,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
>  	 */
>  	int kill_it = 0;
>  	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
> +	DECLARE_BITMAP(hint, MAX_NR_BANKS);

Maybe call it 'toread' like the toclear above so that we know what it
means? Or even better, valid_banks or something...?

>  	char *msg = "Unknown";
>  
>  	atomic_inc(&mce_entry);
> @@ -1025,7 +1031,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
>  	final = &__get_cpu_var(mces_seen);
>  	*final = m;
>  
> -	no_way_out = mce_no_way_out(&m, &msg);
> +	memset(hint, 0, sizeof(hint));
> +	no_way_out = mce_no_way_out(&m, &msg, hint);
>  
>  	barrier();
>  
> @@ -1045,6 +1052,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
>  	order = mce_start(&no_way_out);
>  	for (i = 0; i < banks; i++) {
>  		__clear_bit(i, toclear);
> +		if (!test_bit(i, hint))
> +			continue;
>  		if (!mce_banks[i].ctl)
>  			continue;
>  
> -- 
> 1.7.9.rc2.1.g69204
> 
> 

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] x86/mce: Avoid reading every machine check bank register twice.

[Tony Luck]

On Tue, Apr 17, 2012 at 11:37 AM, Borislav Petkov <bp@amd64.org> wrote:
> Why, is Waldo hidden somewhere in this patch for us to find?
I leave a couple of easy things for people to nit-pick ... it makes feel
useful when they find stuff to complain about :-)

>> +                     while (++i < banks)
>> +                             __set_bit(i, hintp);
>
> I'm guessing this is there so that whenever we have a serious MCE
> worth of panic, we want to look at the remaining banks no matter their
> validity?
Correct. I didn't want to change the functionality of do_machine_fault()
[even though it doesn't currently handle finding multiple errors very
well - we ought to allow it to see everything]
>
> Why not continue through the banks, filling up the hint bitmap but make
> a note to yourself here that we'll be returning 1 when finished?

That's probably a better idea. We don't gain much by breaking
the loop early - might as well let all the cpus do the full scan of
all the banks in parallel.

>>       DECLARE_BITMAP(toclear, MAX_NR_BANKS);
>> +     DECLARE_BITMAP(hint, MAX_NR_BANKS);
>
> Maybe call it 'toread' like the toclear above so that we know what it
> means? Or even better, valid_banks or something...?

See - you found Waldo (I suck at choosing good names). valid_banks
has a good sound to it.

Thanks - I will respin a v2 soon.

-Tony