Re: [PATCH v5 13/18] mm: Remove __alloc_pages_node()

["Brendan Jackman" <brendan.jackman@linux.dev>]

On Fri Jul 3, 2026 at 2:57 PM UTC, Zi Yan wrote:
> On Fri Jul 3, 2026 at 8:31 AM EDT, Brendan Jackman wrote:
>> There were only a few users, which have been removed. The only advantage
>> of this API over alloc_pages_node() is avoiding a single conditional
>> branch. The disadvantages are:
>>
>> 1. More API surface, more sources of confusion, more maintenance.
>>
>> 2. Worse impact of CPU hotplug bugs: most users of __alloc_pages_node()
>>    were using the result of cpu_to_node(); if the CPU gets hotplugged
>>    out this will return NUMA_NO_NODE. If one of these paths fails to
>>    protect against a concurrent hotplug then page_alloc.c will use
>>    NUMA_NO_NODE as an index into NODE_DATA() and cause some horrible
>>    memory corruption or other. With alloc_pages_node(), the code might
>>    just work fine.
>>
>> Ulterior motive: this frees up the __* variants of the allocator APIs to
>> serve specifically for use as mm-internal API.
>>
>> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
>> Reviewed-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
>> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>> ---
>>  include/linux/gfp.h | 20 ++++----------------
>>  1 file changed, 4 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
>> index 01d6d2591f49e..3bf55a5f9143e 100644
>> --- a/include/linux/gfp.h
>> +++ b/include/linux/gfp.h
>> @@ -256,21 +256,6 @@ static inline void warn_if_node_offline(int this_node, gfp_t gfp_mask)
>>  	dump_stack();
>>  }
>>  
>> -/*
>> - * Allocate pages, preferring the node given as nid. The node must be valid and
>> - * online. For more general interface, see alloc_pages_node().
>> - */
>> -static inline struct page *
>> -__alloc_pages_node_noprof(int nid, gfp_t gfp_mask, unsigned int order)
>> -{
>> -	VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
>> -	warn_if_node_offline(nid, gfp_mask);
>> -
>> -	return __alloc_pages_noprof(gfp_mask, order, nid, NULL);
>> -}
>> -
>> -#define  __alloc_pages_node(...)		alloc_hooks(__alloc_pages_node_noprof(__VA_ARGS__))
>> -
>>  static inline
>>  struct folio *__folio_alloc_node_noprof(gfp_t gfp, unsigned int order, int nid)
>>  {
>> @@ -293,7 +278,10 @@ static inline struct page *alloc_pages_node_noprof(int nid, gfp_t gfp_mask,
>>  	if (nid == NUMA_NO_NODE)
>>  		nid = numa_mem_id();
>>  
>> -	return __alloc_pages_node_noprof(nid, gfp_mask, order);
>> +	VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
>
> Could this become a VM_WARN_ON?

Er, it will almost certainly crash later if this is violated. Personally
if I'm enabling DEBUG_VM I'd rather it crashes with a helpful stacktrace
instead of chasing random poihnters, likely corrupting memory, and then
crashing later in a completely undebuggable way instead.

TBH I don't really understand the "don't BUG" culture we have in Linux.
I usually just go along with it coz it doesn't seem important enough to
argue about. But now I'm wondering: if we really don't like VM_BUG_ON()
why not get rid of it completely? I wonder if there's some case-by-case
difference that I'm not picking up on.