Re: [PATCH v2] mm/contig_alloc: fix alloc_contig_range when __GFP_COMP and order < MAX_ORDER

[Zi Yan <ziy@nvidia.com>]

On 25 Apr 2025, at 6:33, David Hildenbrand wrote:

> On 21.04.25 03:36, Jinjiang Tu wrote:
>> When calling alloc_contig_range() with __GFP_COMP and the order of
>> requested pfn range is pageblock_order, less than MAX_ORDER, I triggered
>> WARNING as follows:
>>
>>   PFN range: requested [2150105088, 2150105600), allocated [2150105088, 2150106112)
>>   WARNING: CPU: 3 PID: 580 at mm/page_alloc.c:6877 alloc_contig_range+0x280/0x340
>>
>
> Just to verify: there is no such in-tree user, right?
>
>> alloc_contig_range() marks pageblocks of the requested pfn range to be
>> isolated, migrate these pages if they are in use and will be freed to
>> MIGRATE_ISOLATED freelist.
>>
>> Suppose two alloc_contig_range() calls at the same time and the requested
>> pfn range are [0x80280000, 0x80280200) and [0x80280200, 0x80280400)
>> respectively. Suppose the two memory range are in use, then
>> alloc_contig_range() will migrate and free these pages to MIGRATE_ISOLATED
>> freelist. __free_one_page() will merge MIGRATE_ISOLATE buddy to larger
>> buddy, resulting in a MAX_ORDER buddy. Finally, find_large_buddy() in
>> alloc_contig_range() returns a MAX_ORDER buddy and results in WARNING.
>>
>> To fix it, call free_contig_range() to free the excess pfn range.
>>
>> Fixes: e98337d11bbd ("mm/contig_alloc: support __GFP_COMP")
>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>> ---
>> Changelog since v1:
>>   * Add comment and remove redundant code, suggested by Zi Yan
>>
>>   mm/page_alloc.c | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 579789600a3c..f0162ab991ad 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -6440,6 +6440,7 @@ int alloc_contig_range_noprof(unsigned long start, unsigned long end,
>>   		.alloc_contig = true,
>>   	};
>>   	INIT_LIST_HEAD(&cc.migratepages);
>> +	bool is_range_aligned;
>
> is "aligned" the right word? Aligned to what?
>
> I do wonder if we could do the following on top, checking that the range is suitable for __GFP_COMP earlier.
>

The change below makes the code cleaner. Acked-by: Zi Yan <ziy@nvidia.com>

>
> From 6c414d786db74b1494f7cf66ebf911c01995d20a Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Fri, 25 Apr 2025 12:32:15 +0200
> Subject: [PATCH] tmp
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/page_alloc.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 57aa64dc74a05..85312903dcd8c 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6682,6 +6682,7 @@ static int __alloc_contig_verify_gfp_mask(gfp_t gfp_mask, gfp_t *gfp_cc_mask)
>  int alloc_contig_range_noprof(unsigned long start, unsigned long end,
>  		       unsigned migratetype, gfp_t gfp_mask)
>  {
> +	const int range_order = ilog2(end - start);
>  	unsigned long outer_start, outer_end;
>  	int ret = 0;
>  @@ -6695,12 +6696,19 @@ int alloc_contig_range_noprof(unsigned long start, unsigned long end,
>  		.alloc_contig = true,
>  	};
>  	INIT_LIST_HEAD(&cc.migratepages);
> -	bool is_range_aligned;
>   	gfp_mask = current_gfp_context(gfp_mask);
>  	if (__alloc_contig_verify_gfp_mask(gfp_mask, (gfp_t *)&cc.gfp_mask))
>  		return -EINVAL;
>  +	/* __GFP_COMP may only be used for certain aligned+sized ranges. */
> +	if ((gfp_mask & __GFP_COMP) &&
> +	    (!is_power_of_2(end - start) || !IS_ALIGNED(start, 1 << range_order))) {
> +		WARN_ONCE(true, "PFN range: requested [%lu, %lu) is not suitable for __GFP_COMP\n",
> +			  start, end);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * What we do here is we mark all pageblocks in range as
>  	 * MIGRATE_ISOLATE.  Because pageblock and max order pages may
> @@ -6789,9 +6797,7 @@ int alloc_contig_range_noprof(unsigned long start, unsigned long end,
>  	 * isolated free pages can have higher order than the requested
>  	 * one. Use split_free_pages() to free out of range pages.
>  	 */
> -	is_range_aligned = is_power_of_2(end - start);
> -	if (!(gfp_mask & __GFP_COMP) ||
> -		(is_range_aligned && ilog2(end - start) < MAX_PAGE_ORDER)) {
> +	if (!(gfp_mask & __GFP_COMP) || range_order < MAX_PAGE_ORDER) {
>  		split_free_pages(cc.freepages, gfp_mask);
>   		/* Free head and tail (if any) */
> @@ -6802,22 +6808,16 @@ int alloc_contig_range_noprof(unsigned long start, unsigned long end,
>   		outer_start = start;
>  		outer_end = end;
> -
> -		if (!(gfp_mask & __GFP_COMP))
> -			goto done;
>  	}
>  -	if (start == outer_start && end == outer_end && is_range_aligned) {
> +	if (gfp_mask & __GFP_COMP) {
>  		struct page *head = pfn_to_page(start);
>  		int order = ilog2(end - start);
>  +		VM_WARN_ON_ONCE(outer_start != start || outer_end != end);
>  		check_new_pages(head, order);
>  		prep_new_page(head, order, gfp_mask, 0);
>  		set_page_refcounted(head);
> -	} else {
> -		ret = -EINVAL;
> -		WARN(true, "PFN range: requested [%lu, %lu), allocated [%lu, %lu)\n",
> -		     start, end, outer_start, outer_end);
>  	}
>  done:
>  	undo_isolate_page_range(start, end, migratetype);
> -- 
> 2.49.0
>
>
> -- 
> Cheers,
>
> David / dhildenb


--
Best Regards,
Yan, Zi