Re: [PATCH 0/4] powerpc/topology: Introduce llc_override debugfs

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

On Tue, 2026-10-13 at 15:28 +0530, Shrikanth Hegde wrote:
> 

[snip]

> 
> One way to fix it is with a change like below.
> But more thoughts after the patch.
> 
> ---
>   arch/powerpc/include/asm/topology.h |  7 +------
>   arch/powerpc/kernel/smp.c           | 23 +++++++++++++++++++++++
>   include/linux/sched.h               |  2 +-
>   kernel/sched/topology.c             |  7 +++++++
>   4 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index 44ec416fa489..f4555632e692 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -135,12 +135,7 @@ struct cpumask *cpu_coregroup_mask(int cpu);
>   const struct cpumask *cpu_die_mask(int cpu);
>   int cpu_die_id(int cpu);
>   
> -/*
> - * Points to where the LLC is. On power9 this will point at CACHE
> - * domain, On others it will point to SMT domain. In all cases
> - * cpu_l2_cache_mask points to where LLC is
> - */
> -#define arch_llc_mask(cpu)     cpu_l2_cache_mask(cpu)
> +const struct cpumask *arch_llc_mask(int cpu);

You will need to add
#define arch_llc_mask arch_llc_mask
here.

Otherwise kernel/sched/topology.c looks for a macro:

  # ifndef arch_llc_mask
  #define arch_llc_mask(cpu) cpu_coregroup_mask(cpu)
  # endif

With the #define gone, the generic code falls back to
cpu_coregroup_mask().

>   
>   #ifdef CONFIG_PPC64
>   #include <asm/smp.h>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 6a4ee7d3b8e5..139c54fc3e2d 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1864,6 +1864,28 @@ void __noreturn arch_cpu_idle_dead(void)
>   
>   #endif
>   
> +/*
> + * Points to where the LLC is. On power9 this will point at CACHE
> + * domain, On others it will point to SMT domain. In all cases
> + * cpu_l2_cache_mask points to where LLC is.
> + *
> + * If there is llc_override debug knob, return appropriate mask
> + */
> +const struct cpumask *arch_llc_mask(int cpu)
> +{
> +	switch (READ_ONCE(llc_override)) {
> +	case LLC_OVERRIDE_SMT:
> +		return cpu_smt_mask(cpu);
> +	case LLC_OVERRIDE_MC:
> +		return cpu_coregroup_mask(cpu);
> +	case LLC_OVERRIDE_PKG:
> +		return cpu_die_mask(cpu);
> +	case LLC_OVERRIDE_DISABLED:
> +	default:
> +		return cpu_l2_cache_mask(cpu);
> +	}
> +}
> +

The mask here has to match the span of the highest SD_SHARE_LLC
domain, and there seems to be some mismatches.                                   
                                                                               
- PKG: On power pc, seems like cpu_die_mask() returns
  cpu_coregroup_map when coregroups are
  supported, 

	if (has_coregroup_support())                                                                                                                                                   
		return per_cpu(cpu_coregroup_map, cpu);                                                                                                                                
	else                                                                                                                                                                           
		return cpu_node_mask(cpu);             

  so this is the MC span while sd_llc sits at PKG. It
  should be cpu_node_mask(), the same as tl_pkg_mask().   

- SMT: patch 3 adds SD_SHARE_LLC only to MC/PKG, so for "smt" the
  flags are as before. I think it is possible for P9 to have
  shared_caches between two cores:

  /*
   * P9 has a slightly odd architecture where pairs of cores share an L2 cache.
   * This topology makes it *much* cheaper to migrate tasks between adjacent cores
   * since the migrated task remains cache hot. We want to take advantage of this
   * at the scheduler level so an extra topology level is required.
   */

  the CACHE level also has SD_SHARE_LLC, and highest_flag_domain() picks it, so sd_llc spans
  cpu_l2_cache_map. shared_caches is only set when that map is wider
  than the core's threads, e.g. a P9 core pair sharing an L2:

    CPU:               0 1 2 3 | 4 5 6 7
    cpu_smt_mask():    core A  | core B
    sd_llc (CACHE):    0-7

  The ids follow cpu_smt_mask(), so 0-3 and 4-7 get different ids
  while sd_llc and sd_llc_size cover all eight, and
  cpus_share_cache(0, 4) is false, which is incorrect.

>   int arch_update_cpu_topology(void)
>   {
>   	if (READ_ONCE(llc_override_changed)) {
> @@ -1923,6 +1945,7 @@ static ssize_t llc_override_set(struct file *file, const char __user *ubuf,
>   		pr_info("sched LLC override set to %s. Rebuilding sched domains\n",
>   			llc_override_str[override]);
>   
> +		sched_reset_llc_id();

The reset here seems racy without taking cpus_read_lock, and taking
sched_domains_mutex for each CPU and you could have a cpu hot plug event
or cpu set rebuild in between.  

That's why I would go with the boot parameter. The llc ids are assigned once,
with the right mask, so there is nothing to reset or rebuild, no
change to kernel/sched/, and everything stays in arch/powerpc. Since
this is an experiment knob, needing a reboot to change it is a small
cost, and every run starts from a clean state.

Tim

>   		rebuild_sched_domains();
>   	}
>   
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 7b91cae4a59c..4d7e2ee841a9 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2505,5 +2505,5 @@ extern void migrate_enable(void);
>   #endif /* MODULE */
>   
>   DEFINE_LOCK_GUARD_0(migrate, migrate_disable(), migrate_enable())
> -
> +void sched_reset_llc_id(void);
>   #endif
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index dd2b1d70ec9d..63c9bca4c5eb 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -3087,6 +3087,13 @@ void sched_domains_free_llc_id(int cpu)
>   	sched_domains_mutex_unlock();
>   }
>   
> +void sched_reset_llc_id(void)
> +{
> +	int cpu;
> +
> +	for_each_online_cpu(cpu)
> +		sched_domains_free_llc_id(cpu);
> +}
>   /*
>    * Build sched domains for a given set of CPUs and attach the sched domains
>    * to the individual CPUs