Re: [RFC PATCH v3 02/13] drivers/base/arch_topology: Add support for initializing sbm topology

[Tim Chen <tim.c.chen@linux.intel.com>]

On Thu, 2026-10-01 at 19:28 +0000, K Prateek Nayak wrote:
> Add (fragile) support for initializing sparsebitmap topology for
> architectures that support GENERIC_ARCH_TOPOLOGY.
> 
> Similar to setting cpu_smt_set_num_threads(), count the number of unique
> LLCs (if last_level_cache_is_valid() true, otherwise) / packages, and
> the maximum threads that exist in those instance and set
> sbm_set_proc_config().
> 
> Any failure on the path will default to considering the entire processor
> as a single sbm domain and the default logic will divide the system into
> BITS_PER_LONG chunks. In case of a failure, sbm core will skip using
> arch_sbm_cpu_instance_id() and assume all CPUs belong to the same
> instance with the same instance id.
> 
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
>  drivers/base/arch_topology.c | 122 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 121 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 8c5e47c28d9a..f55745a93298 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -20,6 +20,7 @@
>  #include <linux/cpumask.h>
>  #include <linux/init.h>
>  #include <linux/rcupdate.h>
> +#include <linux/sbm.h>
>  #include <linux/sched.h>
>  #include <linux/units.h>
>  
> @@ -930,6 +931,114 @@ __weak int __init parse_acpi_topology(void)
>  	return 0;
>  }
>  
> +/*
> + * Note: arch_sbm_cpu_instance_id() is only used if init_sbm_topology()
> + * below succeeds at setting the sbm topology. Either the LLC
> + * information is valid or package_id is dependable.
> + */
> +int arch_sbm_cpu_instance_id(int cpu)
> +{
> +	if (last_level_cache_is_valid(cpu)) {
> +		struct cacheinfo *llc_info = get_cpu_cacheinfo_llc(cpu);
> +
> +		if (!llc_info)
> +			goto out;
> +
> +		if (llc_info->attributes & CACHE_ID)
> +			return llc_info->id;
> +
> +		/*
> +		 * XXX: fw_token be truncated from cast
> +		 * when the value is returned as an int.
> +		 */
> +		return (int)((long)llc_info->fw_token);
> +	}
> +out:
> +	return cpu_topology[cpu].package_id;
> +}
> +
> +static void __init init_sbm_topology(void)
> +{
> +	int num_sbm_instances = 0, max_threads_per_instance = -1;
> +	bool has_cache = false, has_package = false;
> +	cpumask_var_t unique_cpus;
> +	struct xarray instances;
> +	unsigned long cpu, *count;
> +
> +	/*
> +	 * If the allocation fails, the sbm core will use
> +	 * the default logic of splitting CPUs evenly in
> +	 * BITS_PER_LONG chunk.
> +	 */
> +	if (!zalloc_cpumask_var(&unique_cpus, GFP_KERNEL))
> +		return;
> +
> +	xa_init(&instances);
> +
> +	for_each_possible_cpu(cpu) {
> +		bool found = false;
> +		int unique_cpu;
> +
> +		for_each_cpu(unique_cpu, unique_cpus) {
> +			/*
> +			 * XXX: Assumes last_level_cache_is_valid() is uniformly true
> +			 * across the entire system if it is true for one CPU.
> +			 */
> +			if (last_level_cache_is_valid(cpu)) {

I don't think last_level_cache_is_valid() can be true for any CPU at
this point. Here is the call order on arm64. riscv is the same in
arch/riscv/kernel/smpboot.c.

  smp_prepare_cpus()                          arch/arm64/kernel/smp.c
    init_cpu_topology()
      fetch_cache_info(cpu)                   for every possible CPU
        allocate_cache_info(cpu)              kzalloc: all leaves are zero
      init_sbm_topology()                     <-- counts instances HERE
        last_level_cache_is_valid(cpu)        false for every CPU
        => groups by package_id
    store_cpu_topology(boot cpu)
      update_siblings_masks()
        detect_cache_attributes()             boot CPU leaves filled HERE
    
  kernel_init_freeable()                      init/main.c 
    sbm_init()
    alloc_sbm_index(boot cpu)
      arch_sbm_cpu_instance_id()              boot CPU: LLC id

  secondary_start_kernel()                    on each secondary CPU
    store_cpu_topology(cpu)
      update_siblings_masks()
        detect_cache_attributes(cpu)          this CPU's leaves filled HERE
  sched_cpu_activate(cpu)
    alloc_sbm_index(cpu)
      arch_sbm_cpu_instance_id()              this CPU: LLC id

last_level_cache_is_valid() only returns true when the LLC leaf has
CACHE_ID set or a fw_token:

        return (llc->attributes & CACHE_ID) || !!llc->fw_token;

Both are written only from detect_cache_attributes(), through
cache_shared_cpu_map_setup() -> cache_setup_properties():

  - DT:   cache_setup_of_node() sets fw_token and CACHE_ID
          (drivers/base/cacheinfo.c)
  - ACPI: cache_setup_acpi() sets fw_token and CACHE_ID
          (drivers/acpi/pptt.c)

fetch_cache_info() only counts the cache levels and allocates the
leaves with kzalloc, so at init_sbm_topology() time every leaf still
has attributes == 0 and fw_token == NULL. Not even the boot CPU has its
leaves filled yet, because its store_cpu_topology() runs after
init_cpu_topology().

I think complications arise if we don't have the complete cache info.

How about updating the design to wait for cache detection instead? The
earliest point where all booted CPUs have their cacheinfo is
sched_init_smp(). That is also where the scheduler builds its own LLC
domains, from the llc_sibling masks that store_cpu_topology() fills.
Nothing needs an sbm index before then: nohz_balance_enter_idle()
returns early on on_null_domain() until the domains exist, and 12/13
already moves the nohz allocation into sched_init_smp(). Roughly:

 1. Move sbm_init() and the boot CPU's alloc_sbm_index() from
    kernel_init_freeable() to the start of sched_init_smp(), before
    init_sched_fair_class_smp().
    
 2. Count the instances there, over the online CPUs, with
    arch_sbm_cpu_instance_id(). At that point it can safely return
    the LLC ID. Counting and runtime then use the same function on
    the same data.
    
 3. Hand out indices to all active CPUs in one pass at that point. In
    sched_cpu_activate(), call alloc_sbm_index() only once
    sched_smp_initialized is set, i.e. for CPUs hotplugged later.

 4. Leave room for possible CPUs that did not boot (maxcpus=, or arm64
    ACPI vCPU hotplug). Their cacheinfo is still unknown at
    sched_init_smp(). Possible choice is what powerpc and s390
    do in this series: assume each of them may add a new instance, or
    land in the largest one. Their package could give a tighter bound.

Perhaps this could simplify the design.

Thanks.

Tim

> +				has_cache = true;
> +				if (last_level_cache_is_shared(cpu, unique_cpu)) {
> +					found = true;
> +					break;
> +				}
> +			} else {
> +				/* Go by package_id if no LLC information is found. */
> +				has_package = true;
> +				if (cpu_topology[unique_cpu].package_id ==
> +				    cpu_topology[cpu].package_id) {
> +					found = true;
> +					break;
> +				}
> +			}
> +		}
> +
> +		/* XXX: CPUs should not suddenly switch IDs mid way. */
> +		if (has_cache && has_package)
> +			goto out;
> +
> +		if (!found) {
> +			count = kzalloc_obj(*count);
> +			if (!count)
> +				goto out;
> +
> +			cpumask_set_cpu(cpu, unique_cpus);
> +			*count += 1;
> +
> +			xa_store(&instances, cpu, count, GFP_KERNEL);
> +			continue;
> +		}
> +
> +		count = xa_load(&instances, unique_cpu);
> +		if (!count)
> +			goto out;
> +
> +		*count += 1;
> +	}
> +
> +	xa_for_each(&instances, cpu, count) {
> +		max_threads_per_instance = max_t(int, max_threads_per_instance, *count);
> +		num_sbm_instances++;
> +	}
> +
> +	sbm_set_topology(num_sbm_instances, max_threads_per_instance);
> +out:
> +	xa_for_each(&instances, cpu, count) {
> +		xa_erase(&instances, cpu);
> +		kfree(count);
> +	}
> +	xa_destroy(&instances);
> +	free_cpumask_var(unique_cpus);
> +}
> +
>  void __init init_cpu_topology(void)
>  {
>  	int cpu, ret;
> @@ -954,8 +1063,19 @@ void __init init_cpu_topology(void)
>  			continue;
>  		else if (ret != -ENOENT)
>  			pr_err("Early cacheinfo failed, ret = %d\n", ret);
> -		return;
> +		break;
>  	}
> +
> +	/*
> +	 * If fetch_cache_info() fails for first CPU,
> +	 * init_cpu_sbm_topology() will use pacakge_id instead.
> +	 *
> +	 * Uniform cache topology is a necessary since implementation
> +	 * assumes last_level_cache_is_valid() gives same result for
> +	 * all possible CPUs
> +	 */
> +	if (!ret || (cpu == 0 && ret == -ENOENT))
> +		init_sbm_topology();
>  }
>  
>  void store_cpu_topology(unsigned int cpuid)