Re: [RFC PATCH v3 08/13] sparc64: Initialize sbm topology on multi-LLC system

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] sparc64: sbm topology initialization runs before CPU data is populated
- [High] sparc64: ghost SBM instances created by iterating uninitialized possible CPUs
--

commit e3accb8257522fa2bde71974a4d31cb5abc096f8
Author: K Prateek Nayak <kprateek.nayak@amd.com>

sparc64: Initialize sbm topology on multi-LLC system

This commit initializes the sparsebitmap (sbm) topology for SPARC64 after
the CPU topology is parsed. It introduces init_sbm_topology() which counts
LLC/NUMA instances and their maximum thread counts, calling it from
setup_arch().

> diff --git a/arch/sparc/kernel/setup_64.c b/arch/sparc/kernel/setup_64.c
> index 63615f5c99b47..7ab52d17e46f4 100644
> --- a/arch/sparc/kernel/setup_64.c
> +++ b/arch/sparc/kernel/setup_64.c

[ ... ]

> @@ -619,6 +621,48 @@ static void __init alloc_irqstack_bootmem(void)
>  	}
>  }
>  
> +static struct cpumask cpu_sbm_setup_map __initdata;
> +static int __node_thread_count[NR_CPUS] __initdata;
> +
> +int arch_sbm_cpu_instance_id(int cpu)
> +{
> +	/* Combine the node and cache identifier. */
> +	return ((u32)cpu_to_node(cpu) << 16) |
> +	       cpu_data(cpu).max_cache_id;
> +}
> +
> +static void __init init_sbm_topology(void)
> +{
> +	int num_sbm_instances, max_threads_per_instance = 1;
> +	int i;
> +
> +	for_each_possible_cpu(i) {

[Severity: High]
Will this loop over uninitialized offline or absent CPUs?

If the possible mask exceeds the present mask, absent CPUs will evaluate
their uninitialized max_cache_id to 0.

They would then incorrectly match each other and be grouped into a single
phantom instance, which could heavily inflate the max_threads_per_instance
calculation below and misallocate the global sparsebitmap arrays.

Should this use for_each_present_cpu() instead?

> +		bool found = false;
> +		int j;
> +
> +		for_each_cpu(j, &cpu_sbm_setup_map) {
> +			if (cpu_to_node(i) == cpu_to_node(j) &&
> +			    cpu_data(i).max_cache_id == cpu_data(j).max_cache_id) {

[Severity: High]
Are the max_cache_id and node values actually initialized when this runs?

This function is called from setup_arch(), but max_cache_id is populated
later during SMP initialization:

arch/sparc/kernel/smp_64.c:setup_per_cpu_areas() {
    ...
    of_fill_in_cpu_data();
    if (tlb_type == hypervisor)
        mdesc_fill_in_cpu_data(cpu_all_mask);
    ...
}

Because setup_per_cpu_areas() runs after setup_arch(),
cpu_data(i).max_cache_id and cpu_to_node(i) will evaluate to zero for all
CPUs here. This would group all CPUs into a single instance instead of
distributing them based on max_cache_id.

If alloc_sbm_index() later finds the true max_cache_id, could it fail to
allocate leaves and return -ENOENT? This might leave __sbm_cpu_to_idx[cpu]
as -1, potentially causing out-of-bounds array accesses in the scheduler
fast paths.

> +				found = true;
> +				break;
> +			}
> +		}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001192849.74788-1-kprateek.nayak@amd.com?part=8