[RFC PATCH] mce: prevent concurrent polling of MCE events

[RFC PATCH] mce: prevent concurrent polling of MCE events

[Aristeu Rozanski]

I've considered creating an array so there'd be one lock per package but
that'd add excessive complexity for something that happens by default every
5 minutes. Thoughts?

---------

Error injection in modern HP machines with CMCI disabled will cause the
injected MCE to be found only by polling. Because these newer machines have a
big number of CPUs per package, it makes a lot more likely for multiple
CPUs polling IMC registers (that are shared in the same package) at same time,
causing multiple reports of the same MCE.

Signed-off-by: Aristeu Rozanski <aris@ruivo.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org

--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1597,6 +1597,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_RAW_SPINLOCK(timer_fn_lock);
 
 static unsigned long mce_adjust_timer_default(unsigned long interval)
 {
@@ -1628,7 +1629,9 @@ static void mce_timer_fn(struct timer_list *t)
 	iv = __this_cpu_read(mce_next_interval);
 
 	if (mce_available(this_cpu_ptr(&cpu_info))) {
+		raw_spin_lock(&timer_fn_lock);
 		machine_check_poll(0, this_cpu_ptr(&mce_poll_banks));
+		raw_spin_unlock(&timer_fn_lock);
 
 		if (mce_intel_cmci_poll()) {
 			iv = mce_adjust_timer(iv);

RE: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Luck, Tony]

> +static DEFINE_RAW_SPINLOCK(timer_fn_lock);
>
>  static unsigned long mce_adjust_timer_default(unsigned long interval)
>  {
> @@ -1628,7 +1629,9 @@ static void mce_timer_fn(struct timer_list *t)
>       iv = __this_cpu_read(mce_next_interval);
>
>       if (mce_available(this_cpu_ptr(&cpu_info))) {
> +             raw_spin_lock(&timer_fn_lock);
>               machine_check_poll(0, this_cpu_ptr(&mce_poll_banks));
> +             raw_spin_unlock(&timer_fn_lock);
>
>               if (mce_intel_cmci_poll()) {
>                       iv = mce_adjust_timer(iv);

If the CMCI polling interrupts on those large number of CPUs are
staggered at different times, then this should be fine. But if a largish
number of CPUs get in lockstep with each other, then this could get
ugly.  I.e. 200 CPUs all take the "CMCI poll" interrupt together. Then
get stuck in a convoy here as one at a time step through checking
the machine check banks.

I'm not sure if that is just a theoretical issue, or how bad it might
actually be if it happened.

One option to avoid this might be to change from a fixed five minute
polling interval to "five minute plus/minus rand(50) jiffies". Then even
if some CPUs did sync up, they'd quickly diverge again.

-Tony

Re: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Aristeu Rozanski]

On Tue, Jan 17, 2023 at 05:42:36PM +0000, Luck, Tony wrote:
> >
> >               if (mce_intel_cmci_poll()) {
> >                       iv = mce_adjust_timer(iv);
> 
> If the CMCI polling interrupts on those large number of CPUs are
> staggered at different times, then this should be fine. But if a largish
> number of CPUs get in lockstep with each other, then this could get
> ugly.  I.e. 200 CPUs all take the "CMCI poll" interrupt together. Then
> get stuck in a convoy here as one at a time step through checking
> the machine check banks.

Yes, but could change the patch to include mce_available() into the
protection of the lock. It should cleared once machine_check_poll() clears
the bank state, no?

> One option to avoid this might be to change from a fixed five minute
> polling interval to "five minute plus/minus rand(50) jiffies". Then even
> if some CPUs did sync up, they'd quickly diverge again.

That'd be fine for me as well. Perhaps initialize the polling interval using
cpu numbers to space them then random for rescheduling?

-- 
Aristeu

RE: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Luck, Tony]

> Yes, but could change the patch to include mce_available() into the
> protection of the lock. It should cleared once machine_check_poll() clears
> the bank state, no?

Which machines are showing this problem? Most modern systems support
CMCI. So I'm thinking that this case shows up because the sysadmin booted
with "mce=no_cmc;". In that case I don't think mce_available() check would
change anything.

-Tony

Re: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Aristeu Rozanski]

On Tue, Jan 17, 2023 at 06:54:02PM +0000, Luck, Tony wrote:
> > Yes, but could change the patch to include mce_available() into the
> > protection of the lock. It should cleared once machine_check_poll() clears
> > the bank state, no?
> 
> Which machines are showing this problem? Most modern systems support
> CMCI. So I'm thinking that this case shows up because the sysadmin booted
> with "mce=no_cmc;". In that case I don't think mce_available() check would
> change anything.

That is correct, ignore what I said.

-- 
Aristeu

Re: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Aristeu Rozanski]

Hi Tony,

On Tue, Jan 17, 2023 at 05:42:36PM +0000, Luck, Tony wrote:
> > +static DEFINE_RAW_SPINLOCK(timer_fn_lock);
> >
> >  static unsigned long mce_adjust_timer_default(unsigned long interval)
> >  {
> > @@ -1628,7 +1629,9 @@ static void mce_timer_fn(struct timer_list *t)
> >       iv = __this_cpu_read(mce_next_interval);
> >
> >       if (mce_available(this_cpu_ptr(&cpu_info))) {
> > +             raw_spin_lock(&timer_fn_lock);
> >               machine_check_poll(0, this_cpu_ptr(&mce_poll_banks));
> > +             raw_spin_unlock(&timer_fn_lock);
> >
> >               if (mce_intel_cmci_poll()) {
> >                       iv = mce_adjust_timer(iv);
> 
> If the CMCI polling interrupts on those large number of CPUs are
> staggered at different times, then this should be fine. But if a largish
> number of CPUs get in lockstep with each other, then this could get
> ugly.  I.e. 200 CPUs all take the "CMCI poll" interrupt together. Then
> get stuck in a convoy here as one at a time step through checking
> the machine check banks.
> 
> I'm not sure if that is just a theoretical issue, or how bad it might
> actually be if it happened.
> 
> One option to avoid this might be to change from a fixed five minute
> polling interval to "five minute plus/minus rand(50) jiffies". Then even
> if some CPUs did sync up, they'd quickly diverge again.

I did experiment different ranges including forcing a minimum difference
based on cpu number but eventually I'd see repeated events. Perhaps a
combination would be acceptable? Set the first run of each cpu spaced based on
CPU number then use a spinlock to synchronize it? That way we minimize the
chance of a big number of CPUs stuck on the same lock but at same time
guarantee we won't have duplicated events.

-- 
Aristeu

RE: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Luck, Tony]

> One option to avoid this might be to change from a fixed five minute
> > polling interval to "five minute plus/minus rand(50) jiffies". Then even
> > if some CPUs did sync up, they'd quickly diverge again.
>
> I did experiment different ranges including forcing a minimum difference
> based on cpu number but eventually I'd see repeated events. Perhaps a
> combination would be acceptable? Set the first run of each cpu spaced based on
> CPU number then use a spinlock to synchronize it? That way we minimize the
> chance of a big number of CPUs stuck on the same lock but at same time
> guarantee we won't have duplicated events.

Aristeu,

Yes. A variable poll interval (to avoid CPUs all bunching together) with
the spinlock to serialize the cases where some CPUs do line up their
polling would work.

-Tony

Re: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Tony Luck]

On Fri, Apr 28, 2023 at 04:43:46PM +0000, Luck, Tony wrote:
> > One option to avoid this might be to change from a fixed five minute
> > > polling interval to "five minute plus/minus rand(50) jiffies". Then even
> > > if some CPUs did sync up, they'd quickly diverge again.
> >
> > I did experiment different ranges including forcing a minimum difference
> > based on cpu number but eventually I'd see repeated events. Perhaps a
> > combination would be acceptable? Set the first run of each cpu spaced based on
> > CPU number then use a spinlock to synchronize it? That way we minimize the
> > chance of a big number of CPUs stuck on the same lock but at same time
> > guarantee we won't have duplicated events.
> 
> Aristeu,
> 
> Yes. A variable poll interval (to avoid CPUs all bunching together) with
> the spinlock to serialize the cases where some CPUs do line up their
> polling would work.

I just ran a test on a system booted with mce=no_cmci and saw the
duplicate reporting.  Digging into the code I see that I'm not the
first to think that a little randomness in timers on different CPUs
would be a good idea. The MCE code starts/restarts timers with:

	mod_timer(t, round_jiffies(when));

Looking at the round_jiffies() function I sw that it is grabbing
the current CPU number.

unsigned long round_jiffies(unsigned long j)
{
        return round_jiffies_common(j, raw_smp_processor_id(), false);
}

And round_jiffies_common() explains with this comment:

        /*
         * We don't want all cpus firing their timers at once hitting the
         * same lock or cachelines, so we skew each extra cpu with an extra
         * 3 jiffies. This 3 jiffies came originally from the mm/ code which
         * already did this.
         * The skew is done by adding 3*cpunr, then round, then subtract this
         * extra offset again.
         */
        j += cpu * 3;

	rem = j % HZ;

So your original patch https://lore.kernel.org/all/Y8WtE7BNJ0gTrqIS@cathedrallabs.org/
that just added the spinlock should be fine!

-Tony

Re: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Aristeu Rozanski]

On Thu, May 11, 2023 at 04:38:57PM -0700, Tony Luck wrote:
> I just ran a test on a system booted with mce=no_cmci and saw the
> duplicate reporting.  Digging into the code I see that I'm not the
> first to think that a little randomness in timers on different CPUs
> would be a good idea. The MCE code starts/restarts timers with:
> 
> 	mod_timer(t, round_jiffies(when));
> 
> Looking at the round_jiffies() function I sw that it is grabbing
> the current CPU number.
> 
> unsigned long round_jiffies(unsigned long j)
> {
>         return round_jiffies_common(j, raw_smp_processor_id(), false);
> }
> 
> And round_jiffies_common() explains with this comment:
> 
>         /*
>          * We don't want all cpus firing their timers at once hitting the
>          * same lock or cachelines, so we skew each extra cpu with an extra
>          * 3 jiffies. This 3 jiffies came originally from the mm/ code which
>          * already did this.
>          * The skew is done by adding 3*cpunr, then round, then subtract this
>          * extra offset again.
>          */
>         j += cpu * 3;
> 
> 	rem = j % HZ;
> 
> So your original patch https://lore.kernel.org/all/Y8WtE7BNJ0gTrqIS@cathedrallabs.org/
> that just added the spinlock should be fine!

Thanks Tony. You want me to resend it or the original is enough?

-- 
Aristeu

RE: [RFC PATCH] mce: prevent concurrent polling of MCE events

[Luck, Tony]

> Thanks Tony. You want me to resend it or the original is enough?

The original was marked RFC. So send a new version without the RFC tag.

You can include:

Reviewed-by: Tony Luck <tony.luck@intel.com>

-Tony