Re: [PATCH] mm/page_alloc: fix alignment for alloc_contig_pages_noprof()

[David Hildenbrand <david@redhat.com>]

On 24.09.25 13:32, Michal Hocko wrote:
> On Tue 23-09-25 11:07:28, Zi Yan wrote:
>> On 23 Sep 2025, at 3:48, David Hildenbrand wrote:
>>
>>> On 23.09.25 09:06, Anshuman Khandual wrote:
>>>> On 23/09/25 7:16 AM, Zi Yan wrote:
>>>>> On 22 Sep 2025, at 20:19, Wei Yang wrote:
>>>>>
>>>>>> Commit 5e27a2df03b8 ("mm/page_alloc: add alloc_contig_pages()")
>>>>>> introduced generic method for alloc_contig_pages(). But the alignment
>>>>>> calculation seems wrong.
>>>>>>
>>>>>> Since ALIGN() only accept power of two value, while nr_pages could be
>>>>>> any positive one, the result is not defined.
>>>>>
>>>>> The result would not be any value lower than zone->zone_start_pfn,
>>>>> so the worst case is getting an unaligned PFN range. I guess
>>>>> most of the time nr_pages would be power of 2.
>>>>
>>>> Agreed.
>>>>
>>>> Also as Dev had pointed out earlier, this function gives no
>>>> guarantee on alignment of non-power-of-2 requests. Hence I
>>>> don't have a strong opinion either way, but does it really
>>>> qualify for a "Fixes:" tag ?
>>>
>>> I'd say if there is nothing to fix, then this patch is not required.
>>>
>>> What likely does make sense would be that a non-power-of-2 would be aligned to the smallest contained power of 2.
>>>
>>> E.g., a 6 MiB request would be aligned to 2 MiB instead of multiples of 6 MiB.
>>>
>>> Not sure what the existing ALIGN would do with that ...
>>
>> Something like the code below:
>>
>> pfn = ALIGN(zone->zone_start_pfn, rounddown_pow_of_two(nr_pages));
> 
> Please let's not try to fix a non-existent problem here. If there is no
> alignement requirement then make sure we document it and drop the
> existing ALIGN which doesn't seem to serve any actual purpose rather
> than make the code more subtle and more obscure for no known reason.

I think we need at least one initial align because the zone_start_pfn 
might not be aligned to the desired order/nr_pages.

So I don't think we can drop it.

-- 
Cheers

David / dhildenb