Re: [PATCH 4/4] mm/page_alloc: remove ifdefs from pindex helpers

[Brendan Jackman]

On Wed May 13, 2026 at 5:19 PM UTC, Vlastimil Babka (SUSE) wrote:
> On 5/13/26 14:35, Brendan Jackman wrote:
>> The ifdefs are not technically needed here, everything used here is
>> always defined.
>> 
>> Switching to IS_ENABLED() makes the code a bit less tiresome to read.
>> 
>> Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
>> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
>> ---
>>  mm/page_alloc.c | 30 ++++++++++++++----------------
>>  1 file changed, 14 insertions(+), 16 deletions(-)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 5d6144c8860ed10fd641184f389c4953465d5178..2985ad0ab1044bdfda8ccc7aaed2ded19b5ac7ed 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -650,19 +650,17 @@ static void bad_page(struct page *page, const char *reason)
>>  
>>  static inline unsigned int order_to_pindex(int migratetype, int order)
>>  {
>> +	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
>> +		bool movable = migratetype == MIGRATE_MOVABLE;
>>  
>> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> -	bool movable;
>> -	if (order > PAGE_ALLOC_COSTLY_ORDER) {
>> -		VM_BUG_ON(!is_pmd_order(order));
>> +		if (order > PAGE_ALLOC_COSTLY_ORDER) {
>> +			VM_BUG_ON(!is_pmd_order(order));
>>  
>> -		movable = migratetype == MIGRATE_MOVABLE;
>> -
>> -		return NR_LOWORDER_PCP_LISTS + movable;
>> +			return NR_LOWORDER_PCP_LISTS + movable;
>> +		}
>> +	} else {
>> +		VM_BUG_ON(order > PAGE_ALLOC_COSTLY_ORDER);
>>  	}
>> -#else
>> -	VM_BUG_ON(order > PAGE_ALLOC_COSTLY_ORDER);
>> -#endif
>
> Uh yeah, VM_BUG_ONs are frowned upon now. But doing a VM_WARN_ON_ONCE here
> makes little sense. There's no safe fallback if we end up here with a wrong
> value. 

Isn't that grounds for upgrading them to a real BUG then? My experience
has been (I broke this code lots of times lol, I REALLY struggle with
this arithmetic stuff) if something goes wrong here the machine quickly
gets irrevocably and undebuggably borked. Crashing is the best case
scenario.

> And it's all internal to page alloc so I'd just drop those checks
> completely at this point.

But, I won't die on the above hill, if people really hate BUG that much
happy to go with the flow. 

In that case, I'd suggest we drop them as a separate patch.