Re: [PATCH v2 3/8] x86/mce/intel: Bound cmci_rediscover() to a single topology peer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] x86/mce/intel: missing LLC topology level in cmci_clear() peer selection
- [Medium] x86/mce/intel: cmci_rediscover() leaves shared banks unclaimed on hybrid CPUs
--

commit cb9d97fffee1a97a8d921b6c7ed854215ee3231b
Author: Pasha Tatashin <pasha.tatashin@soleen.com>

x86/mce/intel: Bound cmci_rediscover() to a single topology peer

This commit optimizes CPU offline CMCI bank rediscovery by recording
orphaned banks in cmci_clear(), selecting a single online topology peer,
and probing only that target peer via smp_call_function_single() in
cmci_rediscover() instead of broadcasting to all online CPUs with
on_each_cpu().

> diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
> index 4655223ba5606..f57718b097d9b 100644
> --- a/arch/x86/kernel/cpu/mce/intel.c
> +++ b/arch/x86/kernel/cpu/mce/intel.c
[ ... ]
> @@ -346,8 +353,31 @@ void cmci_clear(void)
>  	if (!cmci_supported(&banks))
>  		return;
>  	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
> +	bitmap_zero(cmci_orphaned_banks, MAX_NR_BANKS);
> +	/*
> +	 * A bank is shared by the CPUs of one SMT core, L2 cluster, die or
> +	 * package, and these scopes nest. The first online peer found from
> +	 * the innermost scope outwards is therefore part of every sharing
> +	 * scope that still has an online CPU, so it alone can claim every
> +	 * orphaned bank that is still claimable. If it already owns an index,
> +	 * the instance released here is not shared with it, hence not with
> +	 * any online CPU, and that index can be dropped from the rediscovery.
> +	 */
> +	target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu);
> +	if (target >= nr_cpu_ids)
> +		target = cpumask_any_but(topology_cluster_cpumask(cpu), cpu);
> +	if (target >= nr_cpu_ids)
> +		target = cpumask_any_but(topology_die_cpumask(cpu), cpu);
> +	if (target >= nr_cpu_ids)
> +		target = cpumask_any_but(topology_core_cpumask(cpu), cpu);
> +	cmci_orphaned_target = target;
>  	for (i = 0; i < banks; i++)
>  		__cmci_disable_bank(i);
> +	if (target < nr_cpu_ids)
> +		bitmap_andnot(cmci_orphaned_banks, cmci_orphaned_banks,
> +			      per_cpu(mce_banks_owned, target), MAX_NR_BANKS);

[Severity: High]
Should the outward peer search in cmci_clear() also check
cpu_coregroup_mask(cpu) (or cpu_llc_shared_mask(cpu)) between
topology_cluster_cpumask(cpu) and topology_die_cpumask(cpu)?

In arch/x86/kernel/smpboot.c:set_cpu_sibling_map(), x86 populates
cpu_llc_shared_mask as a distinct topology level between
cpu_l2c_shared_mask (topology_cluster_cpumask) and topology_die_cpumask:

    if ((i == cpu) || (has_smt && match_smt(c, o)))
        link_mask(topology_sibling_cpumask, cpu, i);

    if ((i == cpu) || (has_mp && match_llc(c, o)))
        link_mask(cpu_llc_shared_mask, cpu, i);

    if ((i == cpu) || (has_mp && match_l2c(c, o)))
        link_mask(cpu_l2c_shared_mask, cpu, i);

    if ((i == cpu) || (has_mp && match_die(c, o)))
        link_mask(topology_die_cpumask, cpu, i);

On processors where cpu_coregroup_mask(cpu) is narrower than
topology_die_cpumask(cpu) (for example, processors that enumerate tiles via
CPUID leaf 0x1f tile domain or leaf 0x4 L3 cache sharing rather than die
domain, such as Meteor Lake and Arrow Lake where compute tile cores and SoC
tile LP E-cores have distinct llc_id values while sharing die_id, or Xeon
systems in SNC mode), offlining the last online CPU of an L2 cluster while
other CPUs in the same LLC/tile remain online will skip directly to
topology_die_cpumask(cpu).

If cpumask_any_but(topology_die_cpumask(cpu), cpu) picks a target on a
different LLC/tile, could an MCA bank scoped to the offlined CPU's LLC/tile
either be masked out of cmci_orphaned_banks by bitmap_andnot() (because the
remote-tile target owns its own tile's instance of that bank index) or fail
to be claimed by the remote target in cmci_rediscover(), leaving the bank
with CMCI disabled and unpolled while online peers in cpu_coregroup_mask(cpu)
are never probed?

> +	else
> +		bitmap_zero(cmci_orphaned_banks, MAX_NR_BANKS);
>  	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
>  }
>  
> @@ -360,15 +390,38 @@ static void cmci_rediscover_work_func(void *arg)
>  		cmci_discover(banks);
>  }
>  
> -/* After a CPU went down cycle through all the others and rediscover */
> +/*
> + * After a CPU went down, rediscover orphaned shared banks on a single
> + * topology peer (SMT sibling, L2 cluster peer, die peer, or package peer)
> + * selected in cmci_clear(). Core-private banks whose core is now completely
> + * offline cannot be claimed by any other core, so probing more than one
> + * peer is redundant.
> + */
>  void cmci_rediscover(void)
>  {
> +	unsigned int target;
> +	unsigned long flags;
> +	bool orphaned;
>  	int banks;
>  
>  	if (!cmci_supported(&banks))
>  		return;
>  
> -	on_each_cpu(cmci_rediscover_work_func, NULL, 1);
> +	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
> +	orphaned = !bitmap_empty(cmci_orphaned_banks, MAX_NR_BANKS);
> +	target = cmci_orphaned_target;
> +	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
> +
> +	if (!orphaned)
> +		return;
> +
> +	if (target < nr_cpu_ids && cpu_online(target))
> +		smp_call_function_single(target, cmci_rediscover_work_func,
> +					 NULL, 1);
> +
> +	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
> +	bitmap_zero(cmci_orphaned_banks, MAX_NR_BANKS);
> +	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
>  }

[Severity: Medium]
Can probing only a single target CPU in cmci_rediscover() leave shared MCA
banks unclaimed on hybrid processors where CPUs on the same die or package
have different MCA bank counts or per-bank CMCI capabilities?

In cmci_rediscover_work_func(), cmci_supported(&banks) re-reads
MSR_IA32_MCG_CAP on the target CPU because CPUs do not necessarily all have
the same bank count:

    /* Recheck banks in case CPUs don't all have the same */
    if (cmci_supported(&banks))
        cmci_discover(banks);

On hybrid systems (such as Alder Lake, Raptor Lake, Meteor Lake, or Arrow
Lake), P-cores and E-cores share topology_die_cpumask(cpu) and
topology_core_cpumask(cpu) while exposing different bank counts or bank
layouts. When the last thread of a P-core goes offline through
mce_cpu_pre_down() -> mce_disable_cpu() -> cmci_clear() and
mce_cpu_dead() -> cmci_rediscover(), cpumask_any_but() on the die or package
mask can select an E-core with a smaller bank count as cmci_orphaned_target
even when another P-core is still online.

When that happens, cmci_discover(banks) on target only iterates up to target's
bank count, and cmci_rediscover() then unconditionally clears
cmci_orphaned_banks with bitmap_zero() without falling back to other online
CPUs if unclaimed bits remain. Since other CPUs sharing those higher-numbered
banks already cleared them from their per-CPU mce_poll_banks bitmaps in
cmci_skip_bank() during initial discovery, wouldn't those shared banks remain
both unowned for CMCI and unpolled?

титься

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019011446.834487-1-pasha.tatashin@soleen.com?part=3