Re: [PATCH 2/2] mm/page_alloc: refactor build_node_zonelist() out of build_zonelists()

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 9/2/26 23:58, Gregory Price wrote:
> Extract per-node fallback-list construction into build_node_zonelist().
> 
> This lets us build new zonelists from candidate nodemasks instead of
> just the default N_MEMORY node state list.
> 
> No functional change: build_zonelists() builds the same FALLBACK list over
> N_MEMORY with node_load updates as before.
> 
> Signed-off-by: Gregory Price <gourry@gourry.net>

LGTM, but, while we're at it, could we just do the pr_cont() printing in
build_node_zonelist() itself (maybe behind a flag if you don't want to print
from future new caller) so it doesn't need to pass nr_nodes back to
build_zonelists(). Then also the node_order array could live in
build_node_zonelist() itself?

Actually I wonder if we could get ride of the node_order array completely.
It would mean the loop processing in build_zonelists_in_node_order() would
have to be done piece-meal in build_node_zonelist() itself. But seems
feasible? Depends on how you intend to reuse/extend the new functions later,
I guess...

> ---
>  mm/page_alloc.c | 44 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 4dde1cbe2fd43..e1d7c8b221d0d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5849,12 +5849,12 @@ int find_next_best_node_in(int node, nodemask_t *used_node_mask,
>   * DMA zone, if any--but risks exhausting DMA zone.
>   */
>  static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order,
> -		unsigned nr_nodes)
> +		unsigned int nr_nodes, int zlidx)
>  {
>  	struct zoneref *zonerefs;
>  	int i;
>  
> -	zonerefs = pgdat->node_zonelists[ZONELIST_FALLBACK]._zonerefs;
> +	zonerefs = pgdat->node_zonelists[zlidx]._zonerefs;
>  
>  	for (i = 0; i < nr_nodes; i++) {
>  		int nr_zones;
> @@ -5883,26 +5883,28 @@ static void build_thisnode_zonelists(pg_data_t *pgdat)
>  	zonerefs->zone_idx = 0;
>  }
>  
> -static void build_zonelists(pg_data_t *pgdat)
> +/*
> + * Build one node-ordered fallback list from a candidate nodemask.
> + * update_load round-robins node_load across equidistant nodes.
> + */
> +static void build_node_zonelist(pg_data_t *pgdat, const nodemask_t *candidates,
> +				int zlidx, bool update_load,
> +				int *node_order, int *nr)
>  {
> -	static int node_order[MAX_NUMNODES];
> -	int node, nr_nodes = 0;
>  	nodemask_t used_mask = NODE_MASK_NONE;
> -	int local_node, prev_node;
> -
> -	/* NUMA-aware ordering of nodes */
> -	local_node = pgdat->node_id;
> -	prev_node = local_node;
> +	int local_node = pgdat->node_id;
> +	int prev_node = local_node;
> +	int node, nr_nodes = 0;
>  
> -	memset(node_order, 0, sizeof(node_order));
>  	while ((node = find_next_best_node_in(local_node, &used_mask,
> -					      &node_states[N_MEMORY])) >= 0) {
> +					      candidates)) >= 0) {
>  		/*
>  		 * We don't want to pressure a particular node.
>  		 * So adding penalty to the first node in same
>  		 * distance group to make it round-robin.
>  		 */
> -		if (node_distance(local_node, node) !=
> +		if (update_load &&
> +		    node_distance(local_node, node) !=
>  		    node_distance(local_node, prev_node))
>  			node_load[node] += 1;
>  
> @@ -5910,8 +5912,22 @@ static void build_zonelists(pg_data_t *pgdat)
>  		prev_node = node;
>  	}
>  
> -	build_zonelists_in_node_order(pgdat, node_order, nr_nodes);
> +	build_zonelists_in_node_order(pgdat, node_order, nr_nodes, zlidx);
> +	*nr = nr_nodes;
> +}
> +
> +static void build_zonelists(pg_data_t *pgdat)
> +{
> +	static int node_order[MAX_NUMNODES];
> +	int local_node = pgdat->node_id;
> +	int node, nr_nodes = 0;
> +
> +	memset(node_order, 0, sizeof(node_order));
> +
> +	build_node_zonelist(pgdat, &node_states[N_MEMORY], ZONELIST_FALLBACK,
> +			    true, node_order, &nr_nodes);
>  	build_thisnode_zonelists(pgdat);
> +
>  	pr_info("Fallback order for Node %d: ", local_node);
>  	for (node = 0; node < nr_nodes; node++)
>  		pr_cont("%d ", node_order[node]);