Re: [PATCH v6 1/2] perf pmu intel: Generalize SNC cpumask adjustment for multiple platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The bounds check for `imc_num` incorrectly limits the maximum IMC number based on the number of active sockets, causing valid IMC PMUs to be rejected and leaving their cpumasks unadjusted on single-socket systems.
- [High] A divide-by-zero crash can occur when computing `cha_snc` due to an unhandled edge case where the number of discovered CHA PMUs is less than the calculated `snc_nodes`.
--

commit c91c5bd96c1936822e95d0f574b8f633d5afb4c1
Author: Chun-Tse Shao <ctshao@google.com>

perf pmu intel: Generalize SNC cpumask adjustment for multiple platforms

Prepare for supporting more Intel platforms with sub-NUMA clustering by
generalizing the GNR specific logic.

> diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c
> index 7c9d238922a6c..b8fab260c4238 100644
> --- a/tools/perf/arch/x86/util/pmu.c
> +++ b/tools/perf/arch/x86/util/pmu.c

[ ... ]

> @@ -156,11 +166,12 @@ static int uncore_imc_snc(struct perf_pmu *pmu)
>  		pr_warning("Unexpected: unable to compute IMC number '%s'\n", pmu->name);
>  		return 0;
>  	}
> -	if (imc_num >= snc_map_len) {
> +	if (imc_num >= snc_map_len * perf_cpu_map__nr(pmu->cpus)) {

Will this condition incorrectly limit the maximum IMC number based on the
number of active sockets?

The expression perf_cpu_map__nr(pmu->cpus) represents the number of active
sockets, meaning it will evaluate to 1 on a single-socket system. In an
SNC3 configuration where snc_map_len is 6, this would limit imc_num to
less than 6.

Could this cause single-socket systems to wrongly reject imc_num >= 6,
given that the hardware might still expose 12 IMC blocks on that socket?
If so, processing uncore_imc_6 through uncore_imc_11 would trigger the
"Unexpected IMC" warning and skip the necessary cpumask adjustments.

(Note that subsequent patches in the series, such as "perf pmu intel:
Adjust cpumasks for sub-NUMA clusters on Sapphire Rapids and Emerald
Rapids", do not appear to resolve this bounds check).

>  		pr_warning("Unexpected IMC %d for SNC%d mapping\n", imc_num, snc_nodes);
>  		return 0;
>  	}

[ ... ]

> @@ -300,11 +311,12 @@ void perf_pmu__arch_init(struct perf_pmu *pmu)
>  				pmu->mem_events = perf_mem_events_intel_aux;
>  			else
>  				pmu->mem_events = perf_mem_events_intel;
> -		} else if (x86__is_intel_graniterapids()) {
> +		} else if (x86__is_snc_supported()) {
>  			if (strstarts(pmu->name, "uncore_cha_"))
> -				gnr_uncore_cha_imc_adjust_cpumask_for_snc(pmu, /*cha=*/true);
> -			else if (strstarts(pmu->name, "uncore_imc_"))
> -				gnr_uncore_cha_imc_adjust_cpumask_for_snc(pmu, /*cha=*/false);
> +				uncore_cha_imc_adjust_cpumask_for_snc(pmu, /*cha=*/true);

Is it possible for a divide-by-zero to occur when this generalized
adjustment logic invokes uncore_cha_snc()?

If a system or container restricts access to sysfs and exposes fewer CHA
devices, or if testing under a fake PMU environment, the dynamically
determined num_cha could be less than the calculated snc_nodes.

In uncore_cha_snc(), the code performs:

chas_per_node = num_cha / snc_nodes;

And later evaluates:

cha_snc = cha_num / chas_per_node;

Could the initial integer division result in chas_per_node = 0, causing
an unhandled crash when evaluating cha_snc?

> +			else if (strstarts(pmu->name, "uncore_imc_") &&
> +				 !strstarts(pmu->name, "uncore_imc_free_running"))
> +				uncore_cha_imc_adjust_cpumask_for_snc(pmu, /*cha=*/false);
>  		}
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515172710.428474-1-ctshao@google.com?part=1