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

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

On Mon, 2025-09-08 at 00:28 +0800, Chen, Yu C wrote:
> On 9/6/2025 2:36 AM, Tim Chen wrote:

... snip ...
> > -void sched_init_numa(int offline_node)
> > +/*
> > + * Architecture could simplify NUMA distance, to avoid
> > + * creating too many NUMA levels.
> > + */
> > +int __weak arch_sched_node_distance(int from, int to)
> > +{
> > +	return node_distance(from, to);
> > +}
> > +
> > +static int numa_node_dist(int i, int j)
> > +{
> > +	return node_distance(i, j);
> > +}
> > +
> 
> numa_node_dist() seems to be used only once by
> sched_record_numa_dist(), would it be possible to
> use node_distance() directly
> sched_record_numa_dist(offline_node, node_distance, &distances,
> 				   &max_dist, &nr_node_levels))?

Otherwise I will need to pass a flag to sched_record_numa_dist to
choose which distance to use.  I am okay either way. Choosing
the current method so it makes sched_record_numa_dist() simpler.


> 
> > +static int sched_record_numa_dist(int offline_node, int (*n_dist)(int, int),
> > +		int **dist, int *maximum_dist, int *levels)
> > +
> >   {
> > -	struct sched_domain_topology_level *tl;
> >   	unsigned long *distance_map;
> >   	int nr_levels = 0;
> >   	int i, j;
> >   	int *distances;
> > -	struct cpumask ***masks;
> > +	int max_dist = 0;
> >   
> > 
... snip ...

> > +static int avg_remote_numa_distance(int offline_node)
> > +{
> > +	int i, j;
> > +	int distance, nr_remote = 0, total_distance = 0;
> > +
> > +	for_each_cpu_node_but(i, offline_node) {
> > +		for_each_cpu_node_but(j, offline_node) {
> > +			distance = node_distance(i, j);
> > +
> > +			if (distance >= REMOTE_DISTANCE) {
> > +				nr_remote++;
> > +				total_distance += distance;
> > +			}
> > +		}
> > +	}
> > +	if (nr_remote)
> > +		return total_distance / nr_remote;
> > +	else
> > +		return REMOTE_DISTANCE;
> > +}
> > +
> > +void sched_init_numa(int offline_node)
> > +{
> > +	struct sched_domain_topology_level *tl;
> > +	int nr_levels, nr_node_levels;
> > +	int i, j;
> > +	int *distances, *domain_distances;
> > +	int max_dist;
> > +	struct cpumask ***masks;
> > +
> > +	if (sched_record_numa_dist(offline_node, numa_node_dist, &distances,
> > +				   &max_dist, &nr_node_levels))
> > +		return;
> > +
> > +	WRITE_ONCE(sched_avg_remote_numa_distance,
> > +		   avg_remote_numa_distance(offline_node));
> > +
> > +	if (sched_record_numa_dist(offline_node,
> > +				   arch_sched_node_distance, &domain_distances,
> > +				   NULL, &nr_levels)) {
> > +		kfree(distances);
> > +		return;
> > +	}
> > +	rcu_assign_pointer(sched_numa_node_distance, distances);
> > +	WRITE_ONCE(sched_numa_node_levels, nr_node_levels);
> > +
> >   	/*
> >   	 * 'nr_levels' contains the number of unique distances
> >   	 *
> > @@ -1954,6 +2028,8 @@ void sched_init_numa(int offline_node)
> >   	 *
> >   	 * We reset it to 'nr_levels' at the end of this function.
> >   	 */
> > +	rcu_assign_pointer(sched_domains_numa_distance, domain_distances);
> > +
> >   	sched_domains_numa_levels = 0;
> >   
> >   	masks = kzalloc(sizeof(void *) * nr_levels, GFP_KERNEL);
> > @@ -1979,10 +2055,13 @@ void sched_init_numa(int offline_node)
> >   			masks[i][j] = mask;
> >   
> >   			for_each_cpu_node_but(k, offline_node) {
> > -				if (sched_debug() && (node_distance(j, k) != node_distance(k, j)))
> > +				if (sched_debug() &&
> > +				    (arch_sched_node_distance(j, k) !=
> > +				     arch_sched_node_distance(k, j)))
> >   					sched_numa_warn("Node-distance not symmetric");
> >   
> > -				if (node_distance(j, k) > sched_domains_numa_distance[i])
> > +				if (arch_sched_node_distance(j, k) >
> > +					sched_domains_numa_distance[i])
> >   					continue;
> >   
> >   				cpumask_or(mask, mask, cpumask_of_node(k));
> > @@ -2022,7 +2101,7 @@ void sched_init_numa(int offline_node)
> >   	sched_domain_topology = tl;
> >   
> >   	sched_domains_numa_levels = nr_levels;
> > -	WRITE_ONCE(sched_max_numa_distance, sched_domains_numa_distance[nr_levels - 1]);
> > +	WRITE_ONCE(sched_max_numa_distance, max_dist);
> >   
> 
> Would it be possible to use
> WRITE_ONCE(sched_max_numa_distance, distance[nr_node_levels - 1]);
> so we can simplify the code by removing the introduced 'max_dist'
> both in sched_record_numa_dist() and sched_init_numa().

Sure, I think that simplifies sched_record_numa_dist().


Tim