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

[Zi Yan]

On 20 Sep 2026, at 23:29, Gregory Price wrote:

> On Sun, Sep 20, 2026 at 10:49:05PM -0400, Zi Yan wrote:
>> On Fri Sep 11, 2026 at 11:04 PM EDT, Gregory Price wrote:
>>> +static void build_node_zonelist(pg_data_t *pgdat, const nodemask_t *candidates,
>>> +				int zlidx)
>>>  {
>>> -	static int node_order[MAX_NUMNODES];
>>> -	int node, nr_nodes = 0;
>>> +	struct zoneref *zonerefs = pgdat->node_zonelists[zlidx]._zonerefs;
>>
>> Why does build_node_zonelist() need to have a new zlidx instead of using
>> ZONELIST_FALLBACK like build_zonelists_in_node_order() did?
>>
>
> The intent is to build new zonelist over a set of candidate nodes, and
> it's also just clearer: build ZONELIST_FALLBACK from N_MEMORY.
>
> With this we get:
>
> build_node_zonelists(pgdat, &node_states[N_MEMORY_COMMON], ZONELIST_FALLBACK);
> build_node_zonelists(pgdat, &node_states[N_MEMORY], ZONELIST_PRIVATE);
> #ifdef PAGEALLOC_KTEST
> nodemask_andnot(&private_only, &node_states[N_MEMORY],
> 		&node_states[N_MEMORY_COMMON]
> build_node_zonelists(pgdat, &private_only, ZONELIST_KTEST);
> #endif
>

OK, ZONELIST_PRIVATE and ZONELIST_KTEST are not upstream yet, right?
In theory, the new zlidx can be added when you add new ZONELIST_ types.

I am OK with adding it now, but you could mention this change in
the commit message to avoid confusion. Something like,
for bulid_node_zonelist(), use ZONELIST_FALLBACK explicitly.


>
>>> +static void build_zonelists(pg_data_t *pgdat)
>>> +{
>>> +	build_node_zonelist(pgdat, &node_states[N_MEMORY], ZONELIST_FALLBACK);
>>> +	build_thisnode_zonelists(pgdat);
>>
>> If build_thisnode_zonelists() means ZONELIST_NOFALLBACK, why
>> cannot build_node_zonelist() imply ZONELIST_FALLBACK?
>>
>
> thisnode actually means ZONELIST_X+1 as opposed to ZONELIST_NOFALLBACK.
>
> Since folks are adamant about not allowing another GFP flag for zonelist
> selection (beyond GFP_THISNODE), the result of this is that all future
> zonelist additions must carry a FALLBACK + NOFALLBACK variant.
>
> The question you actually want to ask is why build_thisnode_zonelists()
> even exists - it should be part of build_node_zonelist()
>
> I can probably follow up this series by just folding eveything into
>
> /* build zlidx and zlidx+1 (nofallback) */
> build_node_zonelists(pgdat, candidates, zlidx);
>
> And add a BUILD_ON_BUG/ASSERT that forces any CONFIG_NUMA to have
> balanced zonelist additions.

Got it. Thank you for the explanation. Are all combinations of
{ZONELIST_FALLBACK, ZONELIST_PRIVATE, ZONELIST_KTEST} x
{N_MEMORY_GENERAL, N_MEMORY, /* private only */} allowed?
Any enforcement if not? This is more related your “private node” series,
instead of this patchset.

>
> But I don't think it's strictly necessary for any of this, and we're
> just shuffling code from one place to another.  Probably I can just add
> that improvement when we add the next zonelist.  In the meantime - this
> makes it easier to add new zonelists as-is (and just makes the code more
> readable).

Sure, no rush.

Feel free to add

Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>

after you add some text on the added zlidx in the commit message.


Best Regards,
Yan, Zi