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

[Brendan Jackman <jackmanb@google.com>]

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

>  
> -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);