Re: [PATCH 1/3] mm: page_alloc: don't steal single pages from biggest buddy

[Vlastimil Babka <vbabka@suse.cz>]

On 2/25/25 14:34, Brendan Jackman wrote:
> On Mon, Feb 23, 2025 at 07:08:24PM -0500, Johannes Weiner wrote:
>> The fallback code searches for the biggest buddy first in an attempt
>> to steal the whole block and encourage type grouping down the line.
>> 
>> The approach used to be this:
>> 
>> - Non-movable requests will split the largest buddy and steal the
>>   remainder. This splits up contiguity, but it allows subsequent
>>   requests of this type to fall back into adjacent space.
>> 
>> - Movable requests go and look for the smallest buddy instead. The
>>   thinking is that movable requests can be compacted, so grouping is
>>   less important than retaining contiguity.
>> 
>> c0cd6f557b90 ("mm: page_alloc: fix freelist movement during block
>> conversion") enforces freelist type hygiene, which restricts stealing
>> to either claiming the whole block or just taking the requested chunk;
>> no additional pages or buddy remainders can be stolen any more.
>> 
>> The patch mishandled when to switch to finding the smallest buddy in
>> that new reality. As a result, it may steal the exact request size,
>> but from the biggest buddy. This causes fracturing for no good reason.
>> 
>> Fix this by committing to the new behavior: either steal the whole
>> block, or fall back to the smallest buddy.
>> 
>> Remove single-page stealing from steal_suitable_fallback(). Rename it
>> to try_to_steal_block() to make the intentions clear. If this fails,
>> always fall back to the smallest buddy.
> 
> Nit - I think the try_to_steal_block() changes could be a separate
> patch, the history might be easier to understand if it went:
> 
> [1/N] mm: page_alloc: don't steal single pages from biggest buddy
> [2/N] mm: page_alloc: drop unused logic in steal_suitable_fallback()
> 
> (But not a big deal, it's not that hard to follow as-is).
> 
>>  static __always_inline struct page *
>>  __rmqueue_fallback(struct zone *zone, int order, int start_migratetype,
>> @@ -2291,45 +2289,35 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype,
>>  		if (fallback_mt == -1)
>>  			continue;
>>  
>> -		/*
>> -		 * We cannot steal all free pages from the pageblock and the
>> -		 * requested migratetype is movable. In that case it's better to
>> -		 * steal and split the smallest available page instead of the
>> -		 * largest available page, because even if the next movable
>> -		 * allocation falls back into a different pageblock than this
>> -		 * one, it won't cause permanent fragmentation.
>> -		 */
>> -		if (!can_steal && start_migratetype == MIGRATE_MOVABLE
>> -					&& current_order > order)
>> -			goto find_smallest;
>> +		if (!can_steal)
>> +			break;
>>  
>> -		goto do_steal;
>> +		page = get_page_from_free_area(area, fallback_mt);
>> +		page = try_to_steal_block(zone, page, current_order, order,
>> +					  start_migratetype, alloc_flags);
>> +		if (page)
>> +			goto got_one;
>>  	}
>>  
>> -	return NULL;
>> +	if (alloc_flags & ALLOC_NOFRAGMENT)
>> +		return NULL;
> 
> Is this a separate change? Is it a bug that we currently allow
> stealing a from a fallback type when ALLOC_NOFRAGMENT? (I wonder if
> the second loop was supposed to start from min_order).

It's subtle but not a new condition. Previously ALLOC_NOFRAGMENT would
result in not taking the "goto find_smallest" path because it means
searching >=pageblock_order only and that would always be can_steal == true
if it found a fallback. And failure to find fallback would reach an
unconditional return NULL here. Now we fall through the search below
(instead of the goto), but ALLOC_NOFRAGMENT must not do it so it's now
explicit here.

>>  
>> -find_smallest:
>> +	/* No luck stealing blocks. Find the smallest fallback page */
>>  	for (current_order = order; current_order < NR_PAGE_ORDERS; current_order++) {
>>  		area = &(zone->free_area[current_order]);
>>  		fallback_mt = find_suitable_fallback(area, current_order,
>>  				start_migratetype, false, &can_steal);
>> -		if (fallback_mt != -1)
>> -			break;
>> -	}
>> -
>> -	/*
>> -	 * This should not happen - we already found a suitable fallback
>> -	 * when looking for the largest page.
>> -	 */
>> -	VM_BUG_ON(current_order > MAX_PAGE_ORDER);
>> +		if (fallback_mt == -1)
>> +			continue;
>>  
>> -do_steal:
>> -	page = get_page_from_free_area(area, fallback_mt);
>> +		page = get_page_from_free_area(area, fallback_mt);
>> +		page_del_and_expand(zone, page, order, current_order, fallback_mt);
>> +		goto got_one;
>> +	}
>>  
>> -	/* take off list, maybe claim block, expand remainder */
>> -	page = steal_suitable_fallback(zone, page, current_order, order,
>> -				       start_migratetype, alloc_flags, can_steal);
>> +	return NULL;
>>  
>> +got_one:
>>  	trace_mm_page_alloc_extfrag(page, order, current_order,
>>  		start_migratetype, fallback_mt);