Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 20 Jun 2022 22:08:25 -0700
From:   "Luck, Tony" <tony.luck@intel.com>
To:     Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Cc:     Borislav Petkov <bp@alien8.de>, hpa@zytor.com,
        Yazen Ghannam <yazen.ghannam@amd.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
        x86@kernel.org
Subject: Re: [RFC PATCH 4/5] x86/mce: Move storm handling to core.
Message-ID: <YrFSSZqjtWlm9rUr@agluck-desk3.sc.intel.com>
References: <20220406063542.183946-1-Smita.KoralahalliChannabasappa@amd.com>
 <20220406063542.183946-5-Smita.KoralahalliChannabasappa@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220406063542.183946-5-Smita.KoralahalliChannabasappa@amd.com>
Precedence: bulk

On Wed, Apr 06, 2022 at 01:35:41AM -0500, Smita Koralahalli wrote:
> +	/*
> +	 * When a bank is in storm mode, the history mask covers about
> +	 * one second of elapsed time. Check how long it has been since
> +	 * this bank was last polled, and compute a shift value to update
> +	 * the history bitmask.  When not in storm mode, each consecutive
> +	 * poll of the bank is logged in the next history bit, so shift
> +	 * is kept at "1".
> +	 */
> +	if (this_cpu_read(bank_storm[bank])) {
> +		delta = now - this_cpu_read(bank_time_stamp[bank]);
> +		shift = (delta + HZBITS) / HZBITS;
> +	}

Apologies for the long delay in following up on this.

I tested out your patches on an Intel system, and they "work"
in that storms are detected, mitigations applied, and then the
storm end is detected and the system returns to regular mode.

But the storm end happens far more quickly than I expected (in
just over a second).  So I stared again at the code above, and
realized it doesn't do what I expected.  Not your fault, you
just copied from my patches ... which means that my comment
didn't help explain what I was trying to do ... and so it wasn't
obvious that:
1) the test is backwards (need to adjust when the bank is NOT in
storm mode ... in storm mode we poll every second).
2) I can't even remember what I was trying to do with HZBITS, but
it seems wrong too. Just need to use HZ.

Patch below to be merged back into the series. This lets things
run for just over 30 seconds without finding a logged error while
polling in storm mode. Which is what I wanted.

[  111.486306] mce: CPU48 BANK7 CMCI storm detected
[  111.486394] mce: [Hardware Error]: Machine check events logged
[  111.486401] mce: [Hardware Error]: Machine check events logged
[  142.861874] mce: CPU48 BANK7 CMCI storm subsided

-Tony

---

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 74254f15f5db..8e6b77349911 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -655,16 +655,16 @@ void track_cmci_storm(int bank, u64 status)
 	u64 history;
 
 	/*
-	 * When a bank is in storm mode, the history mask covers about
-	 * one second of elapsed time. Check how long it has been since
-	 * this bank was last polled, and compute a shift value to update
-	 * the history bitmask.  When not in storm mode, each consecutive
-	 * poll of the bank is logged in the next history bit, so shift
-	 * is kept at "1".
+	 * 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 (this_cpu_read(bank_storm[bank])) {
+	if (!this_cpu_read(bank_storm[bank])) {
 		delta = now - this_cpu_read(bank_time_stamp[bank]);
-		shift = (delta + HZBITS) / HZBITS;
+		shift = (delta + HZ) / HZ;
 	}
 
 	/* If has been a long time since the last poll, clear history */
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index b9e8c8155c66..b88773a212cf 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -79,13 +79,6 @@ DECLARE_PER_CPU(unsigned long [MAX_NR_BANKS], bank_time_stamp);
  */
 #define STORM_END_POLL_THRESHOLD	30
 
-/*
- * 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)
-
 #ifdef CONFIG_ACPI_APEI
 int apei_write_mce(struct mce *m);
 ssize_t apei_read_mce(struct mce *m, u64 *record_id);