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

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

On 7/13/26 17:26, Brendan Jackman wrote:
> 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

Agreed.

> 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.

It comes from Linus. The main motivation AFAIU are cases that would be
recoverable otherwise. This is almost certainly not the one, but it's hard
sell anyway.
I think he would also argue that the "almost certainly crash" you argue
about above should give you enough info for you to debug it :)
It's likely not the case that would silently corrupt something and then you
crash in completely different context.

> 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.

IIRC we did agree on getting rid of it, but there was no concerted effort
yet. So we're in this "if you touch something, remove it" mode for a while.

Here I think you can just remove it here completely, and not replace it with
a warn. Realistically it was only potentially useful for an unexpected
NUMA_NO_NODE passed to __alloc_pages_node_noprof() and that's gone with your
patch. So keeping that check would be an arbitrary paranoia at this point.