Re: [PATCH v4 1/2] sched: Create architecture specific sched domain distances

["Chen, Yu C" <yu.c.chen@intel.com>]

On 9/30/2025 6:18 AM, Tim Chen wrote:
> On Sat, 2025-09-27 at 20:34 +0800, Chen, Yu C wrote:
>> [snip]
>>
>>> @@ -1591,10 +1591,12 @@ static void claim_allocations(int cpu, struct sched_domain *sd)
>>>    enum numa_topology_type sched_numa_topology_type;
>>>    
>>>    static int			sched_domains_numa_levels;
>>> +static int			sched_numa_node_levels;
>>
>> I agree that the benefit of maintaining two NUMA distances - one for the
>> sched_domain and another for the NUMA balancing/page allocation policy - is
>> to avoid complicating the sched_domain hierarchy while preserving the
>> advantages of NUMA locality.
>>
>> Meanwhile, I wonder if we could also add a "orig" prefix to the original
>> NUMA distance. This way, we can quickly understand its meaning later.
>> For example,
>> sched_orig_node_levels
>> sched_orig_node_distance
> 
> I am not sure adding orig will make the meaning any clearer.
> I can add comments to note that
> 
> sched_numa_node_distance mean the node distance between numa nodes
> sched_numa_nodel_levels  mean the number of unique distances between numa nodes
> 

OK, looks good to me.

>>
>>>    static int			sched_domains_curr_level;
>>>    
>>>    int				sched_max_numa_distance;
>>>    static int			*sched_domains_numa_distance;
>>> +static int			*sched_numa_node_distance;
>>>    static struct cpumask		***sched_domains_numa_masks;
>>>    #endif /* CONFIG_NUMA */
>>>    
>>> @@ -1808,10 +1810,10 @@ bool find_numa_distance(int distance)
>>>    		return true;
>>>    
>>>    	rcu_read_lock();
>>> -	distances = rcu_dereference(sched_domains_numa_distance);
>>> +	distances = rcu_dereference(sched_numa_node_distance);
>>>    	if (!distances)
>>>    		goto unlock;
>>> -	for (i = 0; i < sched_domains_numa_levels; i++) {
>>> +	for (i = 0; i < sched_numa_node_levels; i++) {
>>>    		if (distances[i] == distance) {
>>>    			found = true;
>>>    			break;
>>> @@ -1887,14 +1889,48 @@ static void init_numa_topology_type(int offline_node)
>>>    
>>>    #define NR_DISTANCE_VALUES (1 << DISTANCE_BITS)
>>>    
>>> -void sched_init_numa(int offline_node)
>>> +/*
>>> + * An architecture could modify its NUMA distance, to change
>>> + * grouping of NUMA nodes and number of NUMA levels when creating
>>> + * NUMA level sched domains.
>>> + *
>>> + * A NUMA level is created for each unique
>>> + * arch_sched_node_distance.
>>> + */
>>> +static bool __modified_sched_node_dist = true;
>>> +
>>> +int __weak arch_sched_node_distance(int from, int to)
>>>    {
>>> -	struct sched_domain_topology_level *tl;
>>> -	unsigned long *distance_map;
>>> +	if (__modified_sched_node_dist)
>>> +		__modified_sched_node_dist = false;
>>> +
>>> +	return node_distance(from, to);
>>> +}
>>> +
>>> +static bool modified_sched_node_distance(void)
>>> +{
>>> +	/*
>>> +	 * Call arch_sched_node_distance()
>>> +	 * to determine if arch_sched_node_distance
>>> +	 * has been modified from node_distance()
>>> +	 * to arch specific distance.
>>> +	 */
>>> +	arch_sched_node_distance(0, 0);
>>> +	return __modified_sched_node_dist;
>>> +}
>>> +
>>
>> If our goal is to figure out whether the arch_sched_node_distance()
>> has been overridden, how about the following alias?
>>
>> int __weak arch_sched_node_distance(int from, int to)
>> {
>> 	return __node_distance(from, to);
>> }
>> int arch_sched_node_distance_original(int from, int to) __weak
>> __alias(arch_sched_node_distance);
>>
>> static bool arch_sched_node_distance_is_overridden(void)
>> {
>> 	return arch_sched_node_distance != arch_sched_node_distance_original;
>> }
>>
>> so arch_sched_node_distance_is_overridden() can replace
>> modified_sched_node_distance()
>>
> 
> I think that the alias version will still point to the replaced function and not
> the originally defined one.
> 
> How about not using __weak and just explicitly define arch_sched_node_distance
> as a function pointer.  Change the code like below.
> 

The arch_sched_node_distance_original is defined as __weak, so it
should point to the old function even if the function has been
overridden. I did a test on a X86 VM and it seems to be so.
But using the arch_sched_node_distance as a function point
should also be OK.
> Tim
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index d6b772990ec2..12db78af09d5 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -545,7 +545,7 @@ static int avg_remote_numa_distance(void)
>   	return sched_avg_remote_distance;
>   }
>   
> -int arch_sched_node_distance(int from, int to)
> +static int x86_arch_sched_node_distance(int from, int to)
>   {
>   	int d = node_distance(from, to);
>   
> @@ -918,6 +918,9 @@ static int do_boot_cpu(u32 apicid, unsigned int cpu, struct task_struct *idle)
>   	/* If 64-bit wakeup method exists, use the 64-bit mode trampoline IP */
>   	if (apic->wakeup_secondary_cpu_64)
>   		start_ip = real_mode_header->trampoline_start64;
> +#endif
> +#ifdef CONFIG_NUMA
> +	arch_sched_node_distance = x86_arch_sched_node_distance;
>   #endif

Above might be called for several APs, maybe we can just call it
once in smp_prepare_cpus_common().

thanks,
Chenyu

>   	idle->thread.sp = (unsigned long)task_pt_regs(idle);
>   	initial_code = (unsigned long)start_secondary;
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index 2d2d29553df8..3549c4a19816 100644
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -56,7 +56,7 @@ static inline int cpu_numa_flags(void)
>   {
>   	return SD_NUMA;
>   }
> -extern int arch_sched_node_distance(int from, int to);
> +extern int (*arch_sched_node_distance)(int, int);
>   #endif
>   
>   extern int arch_asym_cpu_priority(int cpu);
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index f25e4402c63e..7cfb7422e9d4 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1897,26 +1897,17 @@ static void init_numa_topology_type(int offline_node)
>    * A NUMA level is created for each unique
>    * arch_sched_node_distance.
>    */
> -static bool __modified_sched_node_dist = true;
>   
> -int __weak arch_sched_node_distance(int from, int to)
> +static int default_sched_node_distance(int from, int to)
>   {
> -	if (__modified_sched_node_dist)
> -		__modified_sched_node_dist = false;
> -
>   	return node_distance(from, to);
>   }
>   
> +int (*arch_sched_node_distance)(int, int) = default_sched_node_distance;
> +
>   static bool modified_sched_node_distance(void)
>   {
> -	/*
> -	 * Call arch_sched_node_distance()
> -	 * to determine if arch_sched_node_distance
> -	 * has been modified from node_distance()
> -	 * to arch specific distance.
> -	 */
> -	arch_sched_node_distance(0, 0);
> -	return __modified_sched_node_dist;
> +	return arch_sched_node_distance != default_sched_node_distance;
>   }
>   
>   static int numa_node_dist(int i, int j)