On Thu, Jul 10, 2014 at 09:12:24PM +0200, Borislav Petkov wrote:
> I'll think about it more tomorrow - my brain is twisted enough for
> today. :-)
Ok, new day, new luck. :-)
So, following yesterday's discussion, our problem is IMHO that shared
banks could be read multiple times before they're finally cleared,
leading to repeated MCE records.
Now, staring at machine_check_poll, the processing is controlled by one
bit - MCI_STATUS_VAL - which decides what happens next.
So how about we change processing around this one bit: we let only one
reader access MSR_IA32_MCx_STATUS(i) and clear it right afterwards by
saving its contents to m.status previously.
Concurrent callers of machine_check_poll will not read the MCI_STATUS
MSR and since they look at the local copy m.status which is 0, they'll
go to the next bank.
And this for the cost of a locked CMPXCHG when we have to inc
poll_reader which should be cheaper than disabling IRQs everytime.
I.e., something like that. Hmm...
---
diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h
b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 09edd0b65fef..5483b507025a 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -19,6 +19,7 @@ struct mce_bank {
unsigned char init; /* initialise bank? */
struct device_attribute attr; /* device attribute */
char attrname[ATTR_LEN]; /* attribute name */
+ atomic_t poll_reader; /* sync for polled
shared banks */
};
int mce_severity(struct mce *a, int tolerant, char **msg);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index bb92f38153b2..443861da86e4 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -609,9 +609,20 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t
*b)
m.addr = 0;
m.bank = i;
m.tsc = 0;
+ m.status = 0;
barrier();
- m.status = mce_rdmsrl(MSR_IA32_MCx_STATUS(i));
+
+ if (atomic_add_unless(&mce_banks[i].poll_reader, 1, 1)) {
+ m.status = mce_rdmsrl(MSR_IA32_MCx_STATUS(i));
+
+ if (m.status & MCI_STATUS_VAL)
+ /* clear status register for this bank */
+ mce_wrmsrl(MSR_IA32_MCx_STATUS(i), 0);
+
+ atomic_dec(&mce_banks[i].poll_reader);
+ }
+
if (!(m.status & MCI_STATUS_VAL))
continue;
@@ -637,17 +648,12 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t
*b)
if (!(flags & MCP_DONTLOG) && !mca_cfg.dont_log_ce)
mce_log(&m);
- /*
- * Clear state for this bank.
- */
- mce_wrmsrl(MSR_IA32_MCx_STATUS(i), 0);
}
/*
* Don't clear MCG_STATUS here because it's only defined for
* exceptions.
*/
-
sync_core();
}
EXPORT_SYMBOL_GPL(machine_check_poll);
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
