Message-ID: <c76723df-f2f1-4888-9e05-61917145503c@amd.com>
Date:   Tue, 19 Sep 2023 13:44:55 -0400
From:   Yazen Ghannam <yazen.ghannam@....com>
To:     Tony Luck <tony.luck@...el.com>, Borislav Petkov <bp@...en8.de>
Cc:     yazen.ghannam@....com, Smita.KoralahalliChannabasappa@....com,
        dave.hansen@...ux.intel.com, x86@...nel.org,
        linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...ts.linux.dev
Subject: Re: [PATCH v7 2/3] x86/mce: Add per-bank CMCI storm mitigation

On 7/18/23 5:08 PM, Tony Luck wrote:
> This is the core functionality to track CMCI storms at the
> machine check bank granularity. Subsequent patches will add
> the vendor specific hooks to supply input to the storm
> detection and take actions on the start/end of a storm.
> 
> 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 a CPU vendor specific action is taken to reduce
> or stop CMCI from the bank that is the source of the storm.  The 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 (defined as 5 in this
> series, could be tuned later if there is data to suggest a better
> value).
> 
> A storm on a bank ends if enough consecutive polls of the bank show
> no corrected errors (defined as 30, may also change). That calls the
> CPU vendor specific function to revert to normal operational mode,
> and changes the polling rate back to the default.
> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  arch/x86/kernel/cpu/mce/internal.h |  41 ++++++++++-
>  arch/x86/kernel/cpu/mce/core.c     | 108 ++++++++++++++++++++++++++---
>  2 files changed, 140 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
> index 9dcad55835fa..da790d13d010 100644
> --- a/arch/x86/kernel/cpu/mce/internal.h
> +++ b/arch/x86/kernel/cpu/mce/internal.h
> @@ -54,7 +54,46 @@ 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);
> +void mce_handle_storm(int bank, bool on);
> +void cmci_storm_begin(int bank);
> +void cmci_storm_end(int bank);

We usually use "unsigned int" for bank numbers. Can we also do so for
these functions?

> +
> +/*
> + * history:	bitmask tracking whether errors were seen or not seen in
> + *		the most recent polls of a bank.
> + * timestamp:	last time (in jiffies) that the bank was polled
> + * storm:	Is this bank in storm mode?
> + */
> +struct storm_bank {
> +	u64 history;
> +	u64 timestamp;
> +	bool storm;
> +};
> +
> +/*
> + * banks:		per-cpu, per-bank details
> + * stormy_bank_count:	count of MC banks in storm state
> + * poll_mode:		CPU is in poll mode
> + */
> +struct mca_storm_desc {
> +	struct storm_bank	banks[MAX_NR_BANKS];
> +	int			stormy_bank_count;

Can we use unsigned values for stormy_bank_count? I'm not clear on the
restrictions for the inc/dec functions.

The maximum possible MCA bank count is 255, i.e. MCG_CAP[Count] = 0xFF.
So stormy_bank_count can be a u8, I think.

> +	bool			poll_mode;
> +};
> +DECLARE_PER_CPU(struct mca_storm_desc, storm_desc);
> +
> +/*
> + * How many errors within the history buffer mark the start of a storm
> + */

This should use single-line style comments.

> +#define STORM_BEGIN_THRESHOLD	5
> +
> +/*
> + * How many polls of machine check bank without an error before declaring
> + * the storm is over. Since it is tracked by the bitmaks in the history
> + * field of struct storm_bank the mask is 30 bits [0 ... 29]

Should have a (.) at the end of this sentence, and also the one above.

> + */
> +#define STORM_END_POLL_THRESHOLD	29
>  
>  #ifdef CONFIG_ACPI_APEI
>  int apei_write_mce(struct mce *m);
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index b45c5008df34..6a44e15d74fe 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -607,6 +607,83 @@ static struct notifier_block mce_default_nb = {
>  	.priority	= MCE_PRIO_LOWEST,
>  };
>  
> +DEFINE_PER_CPU(struct mca_storm_desc, storm_desc);
> +
> +void cmci_storm_begin(int bank)
> +{
> +	struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
> +
> +	__set_bit(bank, this_cpu_ptr(mce_poll_banks));

This assumes that mce_poll_banks does not include banks with correctable
interrupts. This is true for Intel, but not for AMD. 

I'll include a change for AMD to match the Intel behavior. I don't think
there's a good reason for the difference. Also, it may be good to save a
few cycles by disabling polling when interrupts are enabled.

There will be an action for me to make sure AMD users are aware of this.

> +	storm->banks[bank].storm = true;
> +
> +	/*
> +	 * If this is the first bank on this CPU to enter storm mode
> +	 * start polling

Need (.) here.

> +	 */
> +	if (++storm->stormy_bank_count == 1)
> +		mce_timer_kick(true);
> +}
> +
> +void cmci_storm_end(int bank)
> +{
> +	struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
> +
> +	__clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> +	storm->banks[bank].history = 0ull;

Why is the "ull" needed?

> +	storm->banks[bank].storm = false;
> +
> +	/* If no banks left in storm mode, stop polling */

Need (.) here.

> +	if (!this_cpu_dec_return(storm_desc.stormy_bank_count))
> +		mce_timer_kick(false);
> +}
> +
> +static void track_storm(int bank, u64 status)
> +{
> +	struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
> +	unsigned long now = jiffies, delta;
> +	unsigned int shift = 1;
> +	u64 history;
> +
> +	/*
> +	 * When a bank is in storm mode it is polled once per second and
> +	 * the history mask will record about the last minute of poll results.
> +	 * If it is not in storm mode, then the bank is only checked when
> +	 * there is a CMCI interrupt. Check how long it has been since
> +	 * this bank was last checked, and adjust the amount of "shift"
> +	 * to apply to history.
> +	 */
> +	if (!storm->banks[bank].storm) {
> +		delta = now - storm->banks[bank].timestamp;
> +		shift = (delta + HZ) / HZ;
> +	}
> +
> +	/* If has been a long time since the last poll, clear history */

"If it has..." and need (.) here.

> +	if (shift >= 64)
> +		history = 0;

Can this be initialized to '0' up top, then set "if (shift < 64)"?

> +	else
> +		history = storm->banks[bank].history << shift;

Newline here, please.

> +	storm->banks[bank].timestamp = now;
> +
> +	/* History keeps track of corrected errors. VAL=1 && UC=0 */
> +	if ((status & (MCI_STATUS_VAL | MCI_STATUS_UC)) == MCI_STATUS_VAL)

This should be a call to mce_is_correctable(). The VALID bit check isn't
included, but this can be handled by moving the track_storm() call.
Please see below.

...Actually, I wrote this and the comment in the polling function, but
then realized that may not work as intended.

We want the per-bank history to 'tick' every time we enter the polling
function, not just for "valid" errors, correct? You mention this in the
commit message, but it didn't click for me until later... :P

> +		history |= 1;

Newline here, please.

> +	storm->banks[bank].history = history;
> +
> +	if (storm->banks[bank].storm) {
> +		if (history & GENMASK_ULL(STORM_END_POLL_THRESHOLD, 0))
> +			return;
> +		pr_notice("CPU%d BANK%d CMCI storm subsided\n", smp_processor_id(), bank);
> +		mce_handle_storm(bank, false);
> +		cmci_storm_end(bank);
> +	} else {
> +		if (hweight64(history) < STORM_BEGIN_THRESHOLD)
> +			return;
> +		pr_notice("CPU%d BANK%d CMCI storm detected\n", smp_processor_id(), bank);
> +		mce_handle_storm(bank, true);
> +		cmci_storm_begin(bank);
> +	}
> +}
> +
>  /*
>   * Read ADDR and MISC registers.
>   */
> @@ -680,6 +757,8 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
>  		barrier();
>  		m.status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));
>  
> +		track_storm(i, m.status);

This can go after the VALID bit check below.

Also, the "status" parameter can be dropped if the correctable check is
done here...

> +
>  		/* If this entry is not valid, ignore it */
>  		if (!(m.status & MCI_STATUS_VAL))
>  			continue;

...like this.

		if (mce_is_correctable(&m))
			track_storm(i);


...please disregard this comment, if my realization above was correct
about the "history tick". If so, then I think it'd be better to pass
struct mce to track_storm(), because it'll have both the bank number and
status register. And it can then be passed to mce_is_correctable().

> @@ -1622,22 +1701,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_desc.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_desc.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 */
> @@ -1965,6 +2051,12 @@ static void mce_zhaoxin_feature_clear(struct cpuinfo_x86 *c)
>  	intel_clear_lmce();
>  }
>  
> +void mce_handle_storm(int bank, bool on)
> +{
> +	switch (boot_cpu_data.x86_vendor) {
> +	}
> +}
> +
>  static void __mcheck_cpu_init_vendor(struct cpuinfo_x86 *c)
>  {
>  	switch (c->x86_vendor) {

Thanks,
Yazen

Powered by blists - more mailing lists