Messages in this thread | | | Date | Tue, 17 Apr 2012 20:37:15 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH] x86/mce: Avoid reading every machine check bank register twice. |
| |
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
| |