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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 10/14/26 2:14 AM, Tim Chen wrote:
> 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

Yes.

> 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().
> 

facepalm :/

> - 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.
> 

Yes. I doubt anyone wants to try this experiment on Power9.
But yes, functionally updating the flags is needed.

@srikar,
That is also the reason for having the disabled. By default on P9,
llc will be at CACHE domain. But, one may want to experiment with =smt
which i doubt anyone will ever do.


>>    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


Yes. That is what i prefer at this moment too. It will be cleaner.
Only thing is, need to get the masks right in different domains.

> 
>>    		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