Add a hook into machine_check_poll() to keep track of per-CPU, per-bank
corrected error logs.
Maintain a bitmap history for each bank showing whether the bank
logged an corrected error or not each time it is polled.
In normal operation the interval between polls of this banks
determines how far to shift the history. The 64 bit width corresponds
to about one second.
When a storm is observed the Rate of interrupts is reduced by setting
a large threshold value for this bank in IA32_MCi_CTL2. This bank is
added to the bitmap of banks for this CPU to poll. The polling rate
is increased to once per second.
During a storm each bit in the history indicates the status of the
bank each time it is polled. Thus the history covers just over a minute.
Declare a storm for that bank if the number of corrected interrupts
seen in that history is above some threshold (5 in this RFC code for
ease of testing, likely move to 15 for compatibility with previous
storm detection).
A storm on a bank ends if enough consecutive polls of the bank show
no corrected errors (currently 30, may also change). That resets the
threshold in IA32_MCi_CTL2 back to 1, removes the bank from the bitmap
for polling, and changes the polling rate back to the default.
If a CPU with banks in storm mode is taken offline, the new CPU
that inherits ownership of those banks takes over management of
storm(s) in the inherited bank(s).
Signed-off-by: Tony Luck <[email protected]>
---
arch/x86/kernel/cpu/mce/core.c | 26 ++++--
arch/x86/kernel/cpu/mce/intel.c | 124 ++++++++++++++++++++++++++++-
arch/x86/kernel/cpu/mce/internal.h | 4 +-
3 files changed, 143 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 4f9abb66520d..1f3e7c074182 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -714,6 +714,8 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
barrier();
m.status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));
+ mce_intel_storm_tracker(i, m.status);
+
/* If this entry is not valid, ignore it */
if (!(m.status & MCI_STATUS_VAL))
continue;
@@ -1509,6 +1511,7 @@ static unsigned long check_interval = INITIAL_CHECK_INTERVAL;
static DEFINE_PER_CPU(unsigned long, mce_next_interval); /* in jiffies */
static DEFINE_PER_CPU(struct timer_list, mce_timer);
+static DEFINE_PER_CPU(bool, storm_poll_mode);
static void __start_timer(struct timer_list *t, unsigned long interval)
{
@@ -1544,22 +1547,29 @@ static void mce_timer_fn(struct timer_list *t)
else
iv = min(iv * 2, round_jiffies_relative(check_interval * HZ));
- __this_cpu_write(mce_next_interval, iv);
- __start_timer(t, iv);
+ if (__this_cpu_read(storm_poll_mode)) {
+ __start_timer(t, HZ);
+ } else {
+ __this_cpu_write(mce_next_interval, iv);
+ __start_timer(t, iv);
+ }
}
/*
- * Ensure that the timer is firing in @interval from now.
+ * When a storm starts on any bank on this CPU, switch to polling
+ * once per second. When the storm ends, revert to the default
+ * polling interval.
*/
-void mce_timer_kick(unsigned long interval)
+void mce_timer_kick(bool storm)
{
struct timer_list *t = this_cpu_ptr(&mce_timer);
- unsigned long iv = __this_cpu_read(mce_next_interval);
- __start_timer(t, interval);
+ __this_cpu_write(storm_poll_mode, storm);
- if (interval < iv)
- __this_cpu_write(mce_next_interval, interval);
+ if (storm)
+ __start_timer(t, HZ);
+ else
+ __this_cpu_write(mce_next_interval, check_interval * HZ);
}
/* Must not be called in IRQ context where del_timer_sync() can deadlock */
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index cee9d989f791..2ed5634ec277 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -47,8 +47,48 @@ static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
*/
static DEFINE_RAW_SPINLOCK(cmci_discover_lock);
+/*
+ * CMCI storm tracking state
+ */
+static DEFINE_PER_CPU(int, stormy_bank_count);
+static DEFINE_PER_CPU(u64 [MAX_NR_BANKS], bank_history);
+static DEFINE_PER_CPU(bool [MAX_NR_BANKS], bank_storm);
+static DEFINE_PER_CPU(unsigned long [MAX_NR_BANKS], bank_time_stamp);
+static int cmci_threshold[MAX_NR_BANKS];
+
#define CMCI_THRESHOLD 1
+/*
+ * High threshold to limit CMCI rate during storms. Max supported is
+ * 0x7FFF. Use this slightly smaller value so it has a distinctive
+ * signature when some asks "Why am I not seeing all corrected errors?"
+ */
+#define CMCI_STORM_THRESHOLD 0x7FED
+
+/*
+ * How many errors within the history buffer mark the start of a storm
+ */
+#define STORM_BEGIN 5
+
+/*
+ * How many polls of machine check bank without an error before declaring
+ * the storm is over
+ */
+#define STORM_END 30
+
+/*
+ * If there is no poll data for a bank for this amount of time, just
+ * discard the history.
+ */
+#define STORM_INTERVAL (1 * HZ)
+
+/*
+ * When there is no storm each "bit" in the history represents
+ * this many jiffies. When there is a storm every poll() takes
+ * one history bit.
+ */
+#define HZBITS (HZ / 64)
+
static int cmci_supported(int *banks)
{
u64 cap;
@@ -103,6 +143,70 @@ static bool lmce_supported(void)
return tmp & FEAT_CTL_LMCE_ENABLED;
}
+/*
+ * Set a new CMCI threshold value. Preserve the state of the
+ * MCI_CTL2_CMCI_EN bit in case this happens during a
+ * cmci_rediscover() operation.
+ */
+static void cmci_set_threshold(int bank, int thresh)
+{
+ unsigned long flags;
+ u64 val;
+
+ raw_spin_lock_irqsave(&cmci_discover_lock, flags);
+ rdmsrl(MSR_IA32_MCx_CTL2(bank), val);
+ val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
+ wrmsrl(MSR_IA32_MCx_CTL2(bank), val | thresh);
+ raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
+}
+
+static void cmci_storm_begin(int bank)
+{
+ __set_bit(bank, this_cpu_ptr(mce_poll_banks));
+ this_cpu_write(bank_storm[bank], true);
+ if (this_cpu_inc_return(stormy_bank_count) == 1)
+ mce_timer_kick(true);
+}
+
+static void cmci_storm_end(int bank)
+{
+ __clear_bit(bank, this_cpu_ptr(mce_poll_banks));
+ this_cpu_write(bank_history[bank], 0ull);
+ this_cpu_write(bank_storm[bank], false);
+ if (this_cpu_dec_return(stormy_bank_count) == 0)
+ mce_timer_kick(false);
+}
+
+void mce_intel_storm_tracker(int bank, u64 status)
+{
+ unsigned long now = jiffies, delta;
+ unsigned int shift;
+ u64 history;
+
+ delta = now - this_cpu_read(bank_time_stamp[bank]);
+ shift = this_cpu_read(bank_storm[bank]) ? 1 : (delta + HZBITS) / HZBITS;
+ history = (shift < 64) ? this_cpu_read(bank_history[bank]) << shift : 0;
+ this_cpu_write(bank_time_stamp[bank], now);
+
+ if ((status & (MCI_STATUS_VAL | MCI_STATUS_UC)) == MCI_STATUS_VAL)
+ history |= 1;
+ this_cpu_write(bank_history[bank], history);
+
+ if (this_cpu_read(bank_storm[bank])) {
+ if (history & GENMASK_ULL(STORM_END - 1, 0))
+ return;
+ pr_notice("CPU%d BANK%d CMCI storm subsided\n", smp_processor_id(), bank);
+ cmci_set_threshold(bank, cmci_threshold[bank]);
+ cmci_storm_end(bank);
+ } else {
+ if (hweight64(history) < STORM_BEGIN)
+ return;
+ pr_notice("CPU%d BANK%d CMCI storm detected\n", smp_processor_id(), bank);
+ cmci_set_threshold(bank, CMCI_STORM_THRESHOLD);
+ cmci_storm_begin(bank);
+ }
+}
+
/*
* The interrupt handler. This is called on every event.
* Just call the poller directly to log any events.
@@ -147,6 +251,9 @@ static void cmci_discover(int banks)
continue;
}
+ if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD)
+ goto storm;
+
if (!mca_cfg.bios_cmci_threshold) {
val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
val |= CMCI_THRESHOLD;
@@ -159,7 +266,7 @@ static void cmci_discover(int banks)
bios_zero_thresh = 1;
val |= CMCI_THRESHOLD;
}
-
+storm:
val |= MCI_CTL2_CMCI_EN;
wrmsrl(MSR_IA32_MCx_CTL2(i), val);
rdmsrl(MSR_IA32_MCx_CTL2(i), val);
@@ -167,7 +274,14 @@ static void cmci_discover(int banks)
/* Did the enable bit stick? -- the bank supports CMCI */
if (val & MCI_CTL2_CMCI_EN) {
set_bit(i, owned);
- __clear_bit(i, this_cpu_ptr(mce_poll_banks));
+ if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD) {
+ pr_notice("CPU%d BANK%d CMCI inherited storm\n", smp_processor_id(), i);
+ this_cpu_write(bank_history[i], ~0ull);
+ this_cpu_write(bank_time_stamp[i], jiffies);
+ cmci_storm_begin(i);
+ } else {
+ __clear_bit(i, this_cpu_ptr(mce_poll_banks));
+ }
/*
* We are able to set thresholds for some banks that
* had a threshold of 0. This means the BIOS has not
@@ -177,6 +291,10 @@ static void cmci_discover(int banks)
if (mca_cfg.bios_cmci_threshold && bios_zero_thresh &&
(val & MCI_CTL2_CMCI_THRESHOLD_MASK))
bios_wrong_thresh = 1;
+
+ /* Save default threshold for each bank */
+ if (cmci_threshold[i] == 0)
+ cmci_threshold[i] = val & MCI_CTL2_CMCI_THRESHOLD_MASK;
} else {
WARN_ON(!test_bit(i, this_cpu_ptr(mce_poll_banks)));
}
@@ -218,6 +336,8 @@ static void __cmci_disable_bank(int bank)
val &= ~MCI_CTL2_CMCI_EN;
wrmsrl(MSR_IA32_MCx_CTL2(bank), val);
__clear_bit(bank, this_cpu_ptr(mce_banks_owned));
+ if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD)
+ cmci_storm_end(bank);
}
/*
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index f01d6cbeb809..6c7480bce977 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -41,12 +41,14 @@ struct dentry *mce_get_debugfs_dir(void);
extern mce_banks_t mce_banks_ce_disabled;
#ifdef CONFIG_X86_MCE_INTEL
+void mce_intel_storm_tracker(int bank, u64 status);
void cmci_disable_bank(int bank);
void intel_init_cmci(void);
void intel_init_lmce(void);
void intel_clear_lmce(void);
bool intel_filter_mce(struct mce *m);
#else
+static inline void mce_intel_storm_tracker(int bank, u64 status) { }
static inline void cmci_disable_bank(int bank) { }
static inline void intel_init_cmci(void) { }
static inline void intel_init_lmce(void) { }
@@ -54,7 +56,7 @@ static inline void intel_clear_lmce(void) { }
static inline bool intel_filter_mce(struct mce *m) { return false; }
#endif
-void mce_timer_kick(unsigned long interval);
+void mce_timer_kick(bool storm);
#ifdef CONFIG_ACPI_APEI
int apei_write_mce(struct mce *m);
--
2.35.1
Hi,
After reviewing Intel's new implementation to handle storms and
comparing it against the patch here:
https://lore.kernel.org/all/[email protected]/ <https://lore.kernel.org/all/[email protected]/>
AMD's current implementation, already does per CPU disabling of
interrupts and the storm handling is done for corrected errors only.
However, we are not doing per bank approach to handle storms. So,
its a good idea to incorporate this in our code too.
It looked to me that most of the Intel's current implementation can
be shared except in few places where we use different registers
(MCx_MISC) for setting threshold limits to fire interrupts.
Other questions/comments below..
On 2/17/22 11:36 AM, Luck, Tony wrote:
> Add a hook into machine_check_poll() to keep track of per-CPU, per-bank
> corrected error logs.
>
> Maintain a bitmap history for each bank showing whether the bank
> logged an corrected error or not each time it is polled.
>
> In normal operation the interval between polls of this banks
> determines how far to shift the history. The 64 bit width corresponds
> to about one second.
I did not quite get this paragraph and the associated sentence below
..."Thus the history covers just over a minute".
I'm thinking these timing calculations relate to the statements above?
+ delta = now - this_cpu_read(bank_time_stamp[bank]);
+ shift = this_cpu_read(bank_storm[bank]) ? 1 : (delta + HZBITS) / HZBITS;
+ history = (shift < 64) ? this_cpu_read(bank_history[bank]) << shift : 0;
+ this_cpu_write(bank_time_stamp[bank], now);
Can you please elaborate?
>
> When a storm is observed the Rate of interrupts is reduced by setting
> a large threshold value for this bank in IA32_MCi_CTL2. This bank is
> added to the bitmap of banks for this CPU to poll. The polling rate
> is increased to once per second.
> During a storm each bit in the history indicates the status of the
> bank each time it is polled. Thus the history covers just over a minute.
>
> Declare a storm for that bank if the number of corrected interrupts
> seen in that history is above some threshold (5 in this RFC code for
> ease of testing, likely move to 15 for compatibility with previous
> storm detection).
>
> A storm on a bank ends if enough consecutive polls of the bank show
> no corrected errors (currently 30, may also change). That resets the
> threshold in IA32_MCi_CTL2 back to 1, removes the bank from the bitmap
> for polling, and changes the polling rate back to the default.
>
> If a CPU with banks in storm mode is taken offline, the new CPU
> that inherits ownership of those banks takes over management of
> storm(s) in the inherited bank(s).
>
> Signed-off-by: Tony Luck <[email protected]>
> ---
> arch/x86/kernel/cpu/mce/core.c | 26 ++++--
> arch/x86/kernel/cpu/mce/intel.c | 124 ++++++++++++++++++++++++++++-
> arch/x86/kernel/cpu/mce/internal.h | 4 +-
> 3 files changed, 143 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 4f9abb66520d..1f3e7c074182 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -714,6 +714,8 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
> barrier();
> m.status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));
>
> + mce_intel_storm_tracker(i, m.status);
> +
> /* If this entry is not valid, ignore it */
> if (!(m.status & MCI_STATUS_VAL))
> continue;
> @@ -1509,6 +1511,7 @@ static unsigned long check_interval = INITIAL_CHECK_INTERVAL;
>
> static DEFINE_PER_CPU(unsigned long, mce_next_interval); /* in jiffies */
> static DEFINE_PER_CPU(struct timer_list, mce_timer);
> +static DEFINE_PER_CPU(bool, storm_poll_mode);
>
> static void __start_timer(struct timer_list *t, unsigned long interval)
> {
> @@ -1544,22 +1547,29 @@ static void mce_timer_fn(struct timer_list *t)
> else
> iv = min(iv * 2, round_jiffies_relative(check_interval * HZ));
>
> - __this_cpu_write(mce_next_interval, iv);
> - __start_timer(t, iv);
> + if (__this_cpu_read(storm_poll_mode)) {
> + __start_timer(t, HZ);
So, this is where the timer is fired to poll once per second on
a storm?
> + } else {
> + __this_cpu_write(mce_next_interval, iv);
> + __start_timer(t, iv);
> + }
> }
>
> /*
> - * Ensure that the timer is firing in @interval from now.
> + * When a storm starts on any bank on this CPU, switch to polling
> + * once per second. When the storm ends, revert to the default
> + * polling interval.
> */
> -void mce_timer_kick(unsigned long interval)
> +void mce_timer_kick(bool storm)
> {
> struct timer_list *t = this_cpu_ptr(&mce_timer);
> - unsigned long iv = __this_cpu_read(mce_next_interval);
>
> - __start_timer(t, interval);
> + __this_cpu_write(storm_poll_mode, storm);
>
> - if (interval < iv)
> - __this_cpu_write(mce_next_interval, interval);
> + if (storm)
> + __start_timer(t, HZ);
.. and here?
> + else
> + __this_cpu_write(mce_next_interval, check_interval * HZ);
> }
>
> /* Must not be called in IRQ context where del_timer_sync() can deadlock */
> diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
> index cee9d989f791..2ed5634ec277 100644
> --- a/arch/x86/kernel/cpu/mce/intel.c
> +++ b/arch/x86/kernel/cpu/mce/intel.c
> @@ -47,8 +47,48 @@ static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
> */
> static DEFINE_RAW_SPINLOCK(cmci_discover_lock);
>
> +/*
> + * CMCI storm tracking state
> + */
> +static DEFINE_PER_CPU(int, stormy_bank_count);
> +static DEFINE_PER_CPU(u64 [MAX_NR_BANKS], bank_history);
> +static DEFINE_PER_CPU(bool [MAX_NR_BANKS], bank_storm);
So, we maintain two bitmaps.
bank_history: per bank bitmap which contains history of whether
the bank logged corrected error or not each time it is polled.
bank_storm: bitmap of banks in a storm.
Am I right?
> +static DEFINE_PER_CPU(unsigned long [MAX_NR_BANKS], bank_time_stamp);
> +static int cmci_threshold[MAX_NR_BANKS];
> +
> #define CMCI_THRESHOLD 1
>
> +/*
> + * High threshold to limit CMCI rate during storms. Max supported is
> + * 0x7FFF. Use this slightly smaller value so it has a distinctive
> + * signature when some asks "Why am I not seeing all corrected errors?"
> + */
> +#define CMCI_STORM_THRESHOLD 0x7FED
> +
> +/*
> + * How many errors within the history buffer mark the start of a storm
> + */
> +#define STORM_BEGIN 5
> +
> +/*
> + * How many polls of machine check bank without an error before declaring
> + * the storm is over
> + */
> +#define STORM_END 30
> +
> +/*
> + * If there is no poll data for a bank for this amount of time, just
> + * discard the history.
> + */
> +#define STORM_INTERVAL (1 * HZ)
Looks like STORM_INTERVAL isn't been used anywhere..
> +
> +/*
> + * When there is no storm each "bit" in the history represents
> + * this many jiffies. When there is a storm every poll() takes
> + * one history bit.
> + */
> +#define HZBITS (HZ / 64)
> +
> static int cmci_supported(int *banks)
> {
> u64 cap;
> @@ -103,6 +143,70 @@ static bool lmce_supported(void)
> return tmp & FEAT_CTL_LMCE_ENABLED;
> }
>
> +/*
> + * Set a new CMCI threshold value. Preserve the state of the
> + * MCI_CTL2_CMCI_EN bit in case this happens during a
> + * cmci_rediscover() operation.
> + */
> +static void cmci_set_threshold(int bank, int thresh)
> +{
> + unsigned long flags;
> + u64 val;
> +
> + raw_spin_lock_irqsave(&cmci_discover_lock, flags);
Why not local_irq_save(flags) instead?
> + rdmsrl(MSR_IA32_MCx_CTL2(bank), val);
> + val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
> + wrmsrl(MSR_IA32_MCx_CTL2(bank), val | thresh);
> + raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
> +}
> +
> +static void cmci_storm_begin(int bank)
> +{
> + __set_bit(bank, this_cpu_ptr(mce_poll_banks));
> + this_cpu_write(bank_storm[bank], true);
> + if (this_cpu_inc_return(stormy_bank_count) == 1)
What is the significance of stormy_bank_count?
> + mce_timer_kick(true);
> +}
> +
> +static void cmci_storm_end(int bank)
> +{
> + __clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> + this_cpu_write(bank_history[bank], 0ull);
> + this_cpu_write(bank_storm[bank], false);
> + if (this_cpu_dec_return(stormy_bank_count) == 0)
> + mce_timer_kick(false);
> +}
> +
> +void mce_intel_storm_tracker(int bank, u64 status)
> +{
> + unsigned long now = jiffies, delta;
> + unsigned int shift;
> + u64 history;
> +
> + delta = now - this_cpu_read(bank_time_stamp[bank]);
> + shift = this_cpu_read(bank_storm[bank]) ? 1 : (delta + HZBITS) / HZBITS;
> + history = (shift < 64) ? this_cpu_read(bank_history[bank]) << shift : 0;
> + this_cpu_write(bank_time_stamp[bank], now);
> +
> + if ((status & (MCI_STATUS_VAL | MCI_STATUS_UC)) == MCI_STATUS_VAL)
> + history |= 1;
> + this_cpu_write(bank_history[bank], history);
> +
> + if (this_cpu_read(bank_storm[bank])) {
> + if (history & GENMASK_ULL(STORM_END - 1, 0))
> + return;
> + pr_notice("CPU%d BANK%d CMCI storm subsided\n", smp_processor_id(), bank);
> + cmci_set_threshold(bank, cmci_threshold[bank]);
> + cmci_storm_end(bank);
> + } else {
> + if (hweight64(history) < STORM_BEGIN)
> + return;
> + pr_notice("CPU%d BANK%d CMCI storm detected\n", smp_processor_id(), bank);
> + cmci_set_threshold(bank, CMCI_STORM_THRESHOLD);
> + cmci_storm_begin(bank);
> + }
> +}
> +
> /*
> * The interrupt handler. This is called on every event.
> * Just call the poller directly to log any events.
> @@ -147,6 +251,9 @@ static void cmci_discover(int banks)
> continue;
> }
>
> + if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD)
> + goto storm;
> +
> if (!mca_cfg.bios_cmci_threshold) {
> val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
> val |= CMCI_THRESHOLD;
> @@ -159,7 +266,7 @@ static void cmci_discover(int banks)
> bios_zero_thresh = 1;
> val |= CMCI_THRESHOLD;
> }
> -
> +storm:
> val |= MCI_CTL2_CMCI_EN;
> wrmsrl(MSR_IA32_MCx_CTL2(i), val);
> rdmsrl(MSR_IA32_MCx_CTL2(i), val);
> @@ -167,7 +274,14 @@ static void cmci_discover(int banks)
> /* Did the enable bit stick? -- the bank supports CMCI */
> if (val & MCI_CTL2_CMCI_EN) {
> set_bit(i, owned);
> - __clear_bit(i, this_cpu_ptr(mce_poll_banks));
> + if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD) {
Why val is checked twice? Before goto storm and after?
> + pr_notice("CPU%d BANK%d CMCI inherited storm\n", smp_processor_id(), i);
> + this_cpu_write(bank_history[i], ~0ull);
> + this_cpu_write(bank_time_stamp[i], jiffies);
> + cmci_storm_begin(i);
> + } else {
> + __clear_bit(i, this_cpu_ptr(mce_poll_banks));
> + }
> /*
> * We are able to set thresholds for some banks that
> * had a threshold of 0. This means the BIOS has not
> @@ -177,6 +291,10 @@ static void cmci_discover(int banks)
> if (mca_cfg.bios_cmci_threshold && bios_zero_thresh &&
> (val & MCI_CTL2_CMCI_THRESHOLD_MASK))
> bios_wrong_thresh = 1;
> +
> + /* Save default threshold for each bank */
> + if (cmci_threshold[i] == 0)
> + cmci_threshold[i] = val & MCI_CTL2_CMCI_THRESHOLD_MASK;
So, this is the default threshold value for interrupts to fire after storm
subsides?
Is this defaulted to 1? As the commit message reads..
.."That resets the threshold in IA32_MCi_CTL2 back to 1"..
> } else {
> WARN_ON(!test_bit(i, this_cpu_ptr(mce_poll_banks)));
> }
> @@ -218,6 +336,8 @@ static void __cmci_disable_bank(int bank)
> val &= ~MCI_CTL2_CMCI_EN;
> wrmsrl(MSR_IA32_MCx_CTL2(bank), val);
> __clear_bit(bank, this_cpu_ptr(mce_banks_owned));
> + if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD)
> + cmci_storm_end(bank);
> }
>
> /*
> diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
> index f01d6cbeb809..6c7480bce977 100644
> --- a/arch/x86/kernel/cpu/mce/internal.h
> +++ b/arch/x86/kernel/cpu/mce/internal.h
> @@ -41,12 +41,14 @@ struct dentry *mce_get_debugfs_dir(void);
> extern mce_banks_t mce_banks_ce_disabled;
>
> #ifdef CONFIG_X86_MCE_INTEL
> +void mce_intel_storm_tracker(int bank, u64 status);
> void cmci_disable_bank(int bank);
> void intel_init_cmci(void);
> void intel_init_lmce(void);
> void intel_clear_lmce(void);
> bool intel_filter_mce(struct mce *m);
> #else
> +static inline void mce_intel_storm_tracker(int bank, u64 status) { }
> static inline void cmci_disable_bank(int bank) { }
> static inline void intel_init_cmci(void) { }
> static inline void intel_init_lmce(void) { }
> @@ -54,7 +56,7 @@ static inline void intel_clear_lmce(void) { }
> static inline bool intel_filter_mce(struct mce *m) { return false; }
> #endif
>
> -void mce_timer_kick(unsigned long interval);
> +void mce_timer_kick(bool storm);
>
> #ifdef CONFIG_ACPI_APEI
> int apei_write_mce(struct mce *m);
Seems to me that most of the code can be shared except in few places.
Should I come up with a shared code by keeping Tony's patch as reference
and incorporating AMD's changes in them?
Thanks,
Smita
On Thu, Feb 17, 2022 at 09:36:50AM -0800, Luck, Tony wrote:
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 4f9abb66520d..1f3e7c074182 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -714,6 +714,8 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
> barrier();
> m.status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));
>
> + mce_intel_storm_tracker(i, m.status);
Why is this called before the VALID bit check?
Because you want to still run the tracker on each polling - not only
when it sees a valid error?
> diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
> index cee9d989f791..2ed5634ec277 100644
> --- a/arch/x86/kernel/cpu/mce/intel.c
> +++ b/arch/x86/kernel/cpu/mce/intel.c
> @@ -47,8 +47,48 @@ static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
> */
> static DEFINE_RAW_SPINLOCK(cmci_discover_lock);
>
> +/*
> + * CMCI storm tracking state
> + */
Those could use some comments explaining what is tracking what:
> +static DEFINE_PER_CPU(int, stormy_bank_count);
> +static DEFINE_PER_CPU(u64 [MAX_NR_BANKS], bank_history);
> +static DEFINE_PER_CPU(bool [MAX_NR_BANKS], bank_storm);
AFAICT, this says whether a bank is in storm mode?
> +static DEFINE_PER_CPU(unsigned long [MAX_NR_BANKS], bank_time_stamp);
This looks like it collects the jiffies when the bank was looked at in
the storm tracker.
> +static int cmci_threshold[MAX_NR_BANKS];
> +
> #define CMCI_THRESHOLD 1
>
> +/*
> + * High threshold to limit CMCI rate during storms. Max supported is
> + * 0x7FFF. Use this slightly smaller value so it has a distinctive
> + * signature when some asks "Why am I not seeing all corrected errors?"
> + */
> +#define CMCI_STORM_THRESHOLD 0x7FED
Why is a "threshold" in hex?
> +
> +/*
> + * How many errors within the history buffer mark the start of a storm
> + */
> +#define STORM_BEGIN 5
That looks like a STORM_BEGIN_THRESHOLD to me.
> +
> +/*
> + * How many polls of machine check bank without an error before declaring
> + * the storm is over
> + */
> +#define STORM_END 30
Similarly:
STORM_END_POLL_THRESHOLD
> +
> +/*
> + * If there is no poll data for a bank for this amount of time, just
> + * discard the history.
> + */
> +#define STORM_INTERVAL (1 * HZ)
That looks unused.
> +static void cmci_storm_begin(int bank)
> +{
> + __set_bit(bank, this_cpu_ptr(mce_poll_banks));
> + this_cpu_write(bank_storm[bank], true);
> + if (this_cpu_inc_return(stormy_bank_count) == 1)
s/ == 1//
> + mce_timer_kick(true);
> +}
> +
> +static void cmci_storm_end(int bank)
> +{
> + __clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> + this_cpu_write(bank_history[bank], 0ull);
> + this_cpu_write(bank_storm[bank], false);
> + if (this_cpu_dec_return(stormy_bank_count) == 0)
if (!...
> + mce_timer_kick(false);
> +}
> +
> +void mce_intel_storm_tracker(int bank, u64 status)
Function name needs a verb.
> +{
> + unsigned long now = jiffies, delta;
> + unsigned int shift;
> + u64 history;
> +
> + delta = now - this_cpu_read(bank_time_stamp[bank]);
> + shift = this_cpu_read(bank_storm[bank]) ? 1 : (delta + HZBITS) / HZBITS;
Do
shift = 1;
on function entry to simplify this assignment.
Also, I'm having trouble with this shift calculation. The laptop here has
HZ=250. Let's say delta is 2000 jiffies.
So if this bank wasn't in storm mode, I'd have
shift = (2000 + (250 / 64)) / (250 / 64) = 513
...
Aaaha, so only when the diff is < 250 in my case, i.e., it polls the
same bank within a second, only then it would shift the history. I.e.,
what you mean with that "The 64 bit width corresponds to about one
second."
> + history = (shift < 64) ? this_cpu_read(bank_history[bank]) << shift : 0;
> + this_cpu_write(bank_time_stamp[bank], now);
> +
> + if ((status & (MCI_STATUS_VAL | MCI_STATUS_UC)) == MCI_STATUS_VAL)
> + history |= 1;
> + this_cpu_write(bank_history[bank], history);
> +
> + if (this_cpu_read(bank_storm[bank])) {
> + if (history & GENMASK_ULL(STORM_END - 1, 0))
> + return;
Aha, under STORM_END polls you don't declare the storm as being over.
> + pr_notice("CPU%d BANK%d CMCI storm subsided\n", smp_processor_id(), bank);
> + cmci_set_threshold(bank, cmci_threshold[bank]);
> + cmci_storm_end(bank);
> + } else {
> + if (hweight64(history) < STORM_BEGIN)
> + return;
Aha, so you need STORM_BEGIN errors within the last second to cause the
storm polling. Ok I guess.
So all in all I can't find anything eeewy in this - it would need to
have a lot more documentation, though, as this is not the most trivial
thing to stare at.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Mar 07, 2022 at 02:31:21PM +0100, Borislav Petkov wrote:
> So all in all I can't find anything eeewy in this - it would need to
> have a lot more documentation, though, as this is not the most trivial
> thing to stare at.
Thanks for the review. I'll see about re-naming things and adding comments
to make this easier to read.
> > m.status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));
> >
> > + mce_intel_storm_tracker(i, m.status);
>
> Why is this called before the VALID bit check?
>
> Because you want to still run the tracker on each polling - not only
> when it sees a valid error?
Yes. The tracker cares both about polls that find errors, and polls
that don't. Storm detection is triggered with some threshold of positive
scans, even if they are mixed in with some negative ones. End of storm
is determined by a number of consecutive negative polls.
> > diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
> > index cee9d989f791..2ed5634ec277 100644
> > --- a/arch/x86/kernel/cpu/mce/intel.c
> > +++ b/arch/x86/kernel/cpu/mce/intel.c
> > @@ -47,8 +47,48 @@ static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
> > */
> > static DEFINE_RAW_SPINLOCK(cmci_discover_lock);
> >
> > +/*
> > + * CMCI storm tracking state
> > + */
>
> Those could use some comments explaining what is tracking what:
>
> > +static DEFINE_PER_CPU(int, stormy_bank_count);
This one is a count of how many banks on this CPU are in storm mode.
> > +static DEFINE_PER_CPU(u64 [MAX_NR_BANKS], bank_history);
Bitmask history of the most recent 64 polls for each bank.
> > +static DEFINE_PER_CPU(bool [MAX_NR_BANKS], bank_storm);
>
> AFAICT, this says whether a bank is in storm mode?
Yes.
> > +static DEFINE_PER_CPU(unsigned long [MAX_NR_BANKS], bank_time_stamp);
>
> This looks like it collects the jiffies when the bank was looked at in
> the storm tracker.
Yes.
> > +static int cmci_threshold[MAX_NR_BANKS];
> > +
> > #define CMCI_THRESHOLD 1
> >
> > +/*
> > + * High threshold to limit CMCI rate during storms. Max supported is
> > + * 0x7FFF. Use this slightly smaller value so it has a distinctive
> > + * signature when some asks "Why am I not seeing all corrected errors?"
> > + */
> > +#define CMCI_STORM_THRESHOLD 0x7FED
>
> Why is a "threshold" in hex?
For debugging ... I was using rdmsr tool to read the MCi_CTL2 machine
check bank MSRs. It could be defined in decimal.
> > +
> > +/*
> > + * How many errors within the history buffer mark the start of a storm
> > + */
> > +#define STORM_BEGIN 5
>
> That looks like a STORM_BEGIN_THRESHOLD to me.
Yes.
> > +
> > +/*
> > + * How many polls of machine check bank without an error before declaring
> > + * the storm is over
> > + */
> > +#define STORM_END 30
>
> Similarly:
>
> STORM_END_POLL_THRESHOLD
Ditto yes.
> > +
> > +/*
> > + * If there is no poll data for a bank for this amount of time, just
> > + * discard the history.
> > + */
> > +#define STORM_INTERVAL (1 * HZ)
>
> That looks unused.
It was at one point a replacement for CMCI_STORM_INTERVAL deleted
in patch 0001. But I must have dropped the place it was used. Will
delete.
> > +static void cmci_storm_begin(int bank)
> > +{
> > + __set_bit(bank, this_cpu_ptr(mce_poll_banks));
> > + this_cpu_write(bank_storm[bank], true);
> > + if (this_cpu_inc_return(stormy_bank_count) == 1)
>
> s/ == 1//
Not sure about this. The stormy_bank_count variable keeps
track of how many banks on this CPU are in storm mode. So
the transition from zero to one is significant. Need to start
polling. But for 1->2, 2->3 etc. nothing needs to happen. The
CPU is already polling.
> > + mce_timer_kick(true);
> > +}
> > +
> > +static void cmci_storm_end(int bank)
> > +{
> > + __clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> > + this_cpu_write(bank_history[bank], 0ull);
> > + this_cpu_write(bank_storm[bank], false);
> > + if (this_cpu_dec_return(stormy_bank_count) == 0)
>
> if (!...
OK.
> > + mce_timer_kick(false);
> > +}
> > +
> > +void mce_intel_storm_tracker(int bank, u64 status)
>
> Function name needs a verb.
How about: track_cmci_storm(int bank, u64 status) ?
> > +{
> > + unsigned long now = jiffies, delta;
> > + unsigned int shift;
> > + u64 history;
> > +
> > + delta = now - this_cpu_read(bank_time_stamp[bank]);
> > + shift = this_cpu_read(bank_storm[bank]) ? 1 : (delta + HZBITS) / HZBITS;
>
> Do
>
> shift = 1;
>
> on function entry to simplify this assignment.
Yes. That will be easier to read.
> Also, I'm having trouble with this shift calculation. The laptop here has
> HZ=250. Let's say delta is 2000 jiffies.
>
> So if this bank wasn't in storm mode, I'd have
>
> shift = (2000 + (250 / 64)) / (250 / 64) = 513
>
> ...
>
> Aaaha, so only when the diff is < 250 in my case, i.e., it polls the
> same bank within a second, only then it would shift the history. I.e.,
> what you mean with that "The 64 bit width corresponds to about one
> second."
Yes, you got it ... but I'll add a comment so code readers don't have
to redo that deduction every time.
> > + history = (shift < 64) ? this_cpu_read(bank_history[bank]) << shift : 0;
> > + this_cpu_write(bank_time_stamp[bank], now);
> > +
> > + if ((status & (MCI_STATUS_VAL | MCI_STATUS_UC)) == MCI_STATUS_VAL)
> > + history |= 1;
> > + this_cpu_write(bank_history[bank], history);
> > +
> > + if (this_cpu_read(bank_storm[bank])) {
> > + if (history & GENMASK_ULL(STORM_END - 1, 0))
> > + return;
>
> Aha, under STORM_END polls you don't declare the storm as being over.
>
> > + pr_notice("CPU%d BANK%d CMCI storm subsided\n", smp_processor_id(), bank);
> > + cmci_set_threshold(bank, cmci_threshold[bank]);
> > + cmci_storm_end(bank);
> > + } else {
> > + if (hweight64(history) < STORM_BEGIN)
> > + return;
>
> Aha, so you need STORM_BEGIN errors within the last second to cause the
> storm polling. Ok I guess.
Agreed. This needs comments to explain what is going on.
> So all in all I can't find anything eeewy in this - it would need to
> have a lot more documentation, though, as this is not the most trivial
> thing to stare at.
Thanks again for the review.
-Tony