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

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

On Fri, 2025-09-12 at 08:53 +0530, K Prateek Nayak wrote:
> Hello Tim,
> 
> On 9/12/2025 12:00 AM, Tim Chen wrote:
> > +static int sched_record_numa_dist(int offline_node, int (*n_dist)(int, int),
> > +		int **dist, int *levels)
> > +
> 
> nit. Is the blank line above intentional?
> 
> Also personally I prefer breaking the two lines above as:
> 
> static int
> sched_record_numa_dist(int offline_node, int (*n_dist)(int, int), int **dist, int *levels)

That would exceed 80 characters.  So we would still need to move some parameters to a different
line to keep within the limit.

> {
> 	...
> }
> 
> >  {
> > -	struct sched_domain_topology_level *tl;
> >  	unsigned long *distance_map;
> 
> Since we are breaking this out and adding return values, can we also
> cleanup that bitmap_free() before every return with __free(bitmap) like:
> 
> (Only build tested)

Yes, __kfree will be better here.

> 
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 6c0ff62322cb..baa79e79ced8 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1910,9 +1910,8 @@ static int numa_node_dist(int i, int j)
>  
>  static int sched_record_numa_dist(int offline_node, int (*n_dist)(int, int),
>  		int **dist, int *levels)
> -
>  {
> -	unsigned long *distance_map;
> +	unsigned long *distance_map __free(bitmap) = NULL;
>  	int nr_levels = 0;
>  	int i, j;
>  	int *distances;
> @@ -1932,7 +1931,6 @@ static int sched_record_numa_dist(int offline_node, int (*n_dist)(int, int),
>  
>  			if (distance < LOCAL_DISTANCE || distance >= NR_DISTANCE_VALUES) {
>  				sched_numa_warn("Invalid distance value range");
> -				bitmap_free(distance_map);
>  				return -EINVAL;
>  			}
>  
> @@ -1946,19 +1944,17 @@ static int sched_record_numa_dist(int offline_node, int (*n_dist)(int, int),
>  	nr_levels = bitmap_weight(distance_map, NR_DISTANCE_VALUES);
>  
>  	distances = kcalloc(nr_levels, sizeof(int), GFP_KERNEL);
> -	if (!distances) {
> -		bitmap_free(distance_map);
> +	if (!distances)
>  		return -ENOMEM;
> -	}
> +
>  	for (i = 0, j = 0; i < nr_levels; i++, j++) {
>  		j = find_next_bit(distance_map, NR_DISTANCE_VALUES, j);
>  		distances[i] = j;
>  	}
> +
>  	*dist = distances;
>  	*levels = nr_levels;
>  
> -	bitmap_free(distance_map);
> -
>  	return 0;
>  }
>  
> ---
> 
> >  	int nr_levels = 0;
> >  	int i, j;
> >  	int *distances;
> > -	struct cpumask ***masks;
> >  
> >  	/*
> >  	 * O(nr_nodes^2) de-duplicating selection sort -- in order to find the
> > @@ -1902,17 +1923,17 @@ void sched_init_numa(int offline_node)
> >  	 */
> >  	distance_map = bitmap_alloc(NR_DISTANCE_VALUES, GFP_KERNEL);
> >  	if (!distance_map)
> > -		return;
> > +		return -ENOMEM;
> >  
> >  	bitmap_zero(distance_map, NR_DISTANCE_VALUES);
> >  	for_each_cpu_node_but(i, offline_node) {
> >  		for_each_cpu_node_but(j, offline_node) {
> > -			int distance = node_distance(i, j);
> > +			int distance = n_dist(i, j);
> >  
> >  			if (distance < LOCAL_DISTANCE || distance >= NR_DISTANCE_VALUES) {
> >  				sched_numa_warn("Invalid distance value range");
> >  				bitmap_free(distance_map);
> > -				return;
> > +				return -EINVAL;
> >  			}
> >  
> >  			bitmap_set(distance_map, distance, 1);
> > @@ -1927,17 +1948,66 @@ void sched_init_numa(int offline_node)
> >  	distances = kcalloc(nr_levels, sizeof(int), GFP_KERNEL);
> >  	if (!distances) {
> >  		bitmap_free(distance_map);
> > -		return;
> > +		return -ENOMEM;
> >  	}
> > -
> >  	for (i = 0, j = 0; i < nr_levels; i++, j++) {
> >  		j = find_next_bit(distance_map, NR_DISTANCE_VALUES, j);
> >  		distances[i] = j;
> >  	}
> > -	rcu_assign_pointer(sched_domains_numa_distance, distances);
> > +	*dist = distances;
> > +	*levels = nr_levels;
> >  
> >  	bitmap_free(distance_map);
> >  
> > +	return 0;
> > +}
> > +
> > +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;
> > +	struct cpumask ***masks;
> > +
> > +	if (sched_record_numa_dist(offline_node, numa_node_dist, &distances,
> > +				   &nr_node_levels))
> > +		return;
> > +
> > +	WRITE_ONCE(sched_avg_remote_numa_distance,
> > +		   avg_remote_numa_distance(offline_node));
> 
> nit.
> 
> Can add a small comment here saying arch_sched_node_distance() may
> depend on sched_avg_remote_numa_distance and requires it to be
> initialized correctly before computing domain_distances.

Sure.

Thanks for the review.

Tim

> 
> Apart from those nitpicks, the changes look good to me. Please feel free
> to include:
> 
> Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>