Re: [PATCH RFC x86/mce] Make mce_timed_out() identify holdout CPUs

[Paul E. McKenney]

On Wed, Jan 06, 2021 at 07:32:44PM +0100, Borislav Petkov wrote:
> On Wed, Jan 06, 2021 at 09:41:02AM -0800, Paul E. McKenney wrote:
> > The "Timeout: Not all CPUs entered broadcast exception handler" message
> > will appear from time to time given enough systems, but this message does
> > not identify which CPUs failed to enter the broadcast exception handler.
> > This information would be valuable if available, for example, in order to
> > correlated with other hardware-oriented error messages.
> 
> Because you're expecting that the CPUs which have not entered the
> exception handler might have stuck earlier and that's the correlation
> there?

Or that there might have been any number of other error messages that
flagged that CPU.  For a few examples, watchdogs, hung tasks, and RCU
CPU stall warnings.

> > This commit
> 
> That's a tautology. :)

Not yet, it isn't!  Well, except in -rcu.  ;-)

> > therefore maintains a cpumask_t of CPUs that have entered this handler,
> > and prints out which ones failed to enter in the event of a timeout.
> > Build-tested only.
> > 
> > Cc: Tony Luck <tony.luck@xxxxxxxxx>
> > Cc: Borislav Petkov <bp@xxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > Cc: <x86@xxxxxxxxxx>
> > Cc: <linux-edac@xxxxxxxxxxxxxxx>
> > Reported-by: Jonathan Lemon <bsd@xxxxxx>
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > 
> > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> > index 13d3f1c..44d2b99 100644
> > --- a/arch/x86/kernel/cpu/mce/core.c
> > +++ b/arch/x86/kernel/cpu/mce/core.c
> > @@ -878,6 +878,12 @@ static atomic_t mce_executing;
> >  static atomic_t mce_callin;
> >  
> >  /*
> > + * Track which CPUs entered and not in order to print holdouts.
> > + */
> > +static cpumask_t mce_present_cpus;
> > +static cpumask_t mce_missing_cpus;
> > +
> > +/*
> >   * Check if a timeout waiting for other CPUs happened.
> >   */
> >  static int mce_timed_out(u64 *t, const char *msg)
> > @@ -894,8 +900,12 @@ static int mce_timed_out(u64 *t, const char *msg)
> >  	if (!mca_cfg.monarch_timeout)
> >  		goto out;
> >  	if ((s64)*t < SPINUNIT) {
> > -		if (mca_cfg.tolerant <= 1)
> > +		if (mca_cfg.tolerant <= 1) {
> > +			if (!cpumask_andnot(&mce_missing_cpus, cpu_online_mask, &mce_present_cpus))
> > +				pr_info("%s: MCE holdout CPUs: %*pbl\n",
> > +					__func__, cpumask_pr_args(&mce_missing_cpus));
> >  			mce_panic(msg, NULL, NULL);
> > +		}
> >  		cpu_missing = 1;
> >  		return 1;
> >  	}
> > @@ -1006,6 +1016,7 @@ static int mce_start(int *no_way_out)
> >  	 * is updated before mce_callin.
> >  	 */
> >  	order = atomic_inc_return(&mce_callin);
> 
> Doesn't a single mce_callin_mask suffice?

You are suggesting dropping mce_missing_cpus and just doing this?

if (!cpumask_andnot(&mce_present_cpus, cpu_online_mask, &mce_present_cpus))

I was worried (perhaps unnecessarily) about the possibility of CPUs
checking in during the printout operation, which would set rather than
clear the bit.  But perhaps the possible false positives that Tony points
out make this race not worth worrying about.

Thoughts?

							Thanx, Paul