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

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

[Wei Yang]

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.

Use roundup() to calculate the correct alignment.

Fixes: 5e27a2df03b8 ("mm/page_alloc: add alloc_contig_pages()")
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Brendan Jackman <jackmanb@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Zi Yan <ziy@nvidia.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a1bcc1e003c7..a17a6014e3db 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7095,7 +7095,7 @@ struct page *alloc_contig_pages_noprof(unsigned long nr_pages, gfp_t gfp_mask,
 					gfp_zone(gfp_mask), nodemask) {
 		spin_lock_irqsave(&zone->lock, flags);
 
-		pfn = ALIGN(zone->zone_start_pfn, nr_pages);
+		pfn = roundup(zone->zone_start_pfn, nr_pages);
 		while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
 			if (pfn_range_valid_contig(pfn, nr_pages)) {
 				/*
-- 
2.34.1

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

[Zi Yan]

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.

>
> Use roundup() to calculate the correct alignment.
>
> Fixes: 5e27a2df03b8 ("mm/page_alloc: add alloc_contig_pages()")
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Brendan Jackman <jackmanb@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Cc: Zi Yan <ziy@nvidia.com>
> ---
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a1bcc1e003c7..a17a6014e3db 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7095,7 +7095,7 @@ struct page *alloc_contig_pages_noprof(unsigned long nr_pages, gfp_t gfp_mask,
>  					gfp_zone(gfp_mask), nodemask) {
>  		spin_lock_irqsave(&zone->lock, flags);
>
> -		pfn = ALIGN(zone->zone_start_pfn, nr_pages);
> +		pfn = roundup(zone->zone_start_pfn, nr_pages);
>  		while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
>  			if (pfn_range_valid_contig(pfn, nr_pages)) {
>  				/*
> -- 
> 2.34.1

LGTM. Reviewed-by: Zi Yan <ziy@nvidia.com>

Best Regards,
Yan, Zi

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

[Anshuman Khandual]

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 ?
> 
>>
>> Use roundup() to calculate the correct alignment.
>>
>> Fixes: 5e27a2df03b8 ("mm/page_alloc: add alloc_contig_pages()")
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>> Cc: Oscar Salvador <osalvador@suse.de>
>> Cc: Brendan Jackman <jackmanb@google.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Suren Baghdasaryan <surenb@google.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Wei Yang <richard.weiyang@gmail.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> ---
>>  mm/page_alloc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index a1bcc1e003c7..a17a6014e3db 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -7095,7 +7095,7 @@ struct page *alloc_contig_pages_noprof(unsigned long nr_pages, gfp_t gfp_mask,
>>  					gfp_zone(gfp_mask), nodemask) {
>>  		spin_lock_irqsave(&zone->lock, flags);
>>
>> -		pfn = ALIGN(zone->zone_start_pfn, nr_pages);
>> +		pfn = roundup(zone->zone_start_pfn, nr_pages);
>>  		while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
>>  			if (pfn_range_valid_contig(pfn, nr_pages)) {
>>  				/*
>> -- 
>> 2.34.1
> 
> LGTM. Reviewed-by: Zi Yan <ziy@nvidia.com>
> 
> Best Regards,
> Yan, Zi

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

[David Hildenbrand]

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

-- 
Cheers

David / dhildenb

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

[Zi Yan]

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

Best Regards,
Yan, Zi

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

[Michal Hocko]

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.
-- 
Michal Hocko
SUSE Labs

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

[David Hildenbrand]

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

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

[Michal Hocko]

On Wed 24-09-25 13:40:58, David Hildenbrand wrote:
> 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.

I thought we have concluded that there is no user expecting a specific
alignment. Or have I misunderstood that?
-- 
Michal Hocko
SUSE Labs

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

[David Hildenbrand]

On 24.09.25 14:01, Michal Hocko wrote:
> On Wed 24-09-25 13:40:58, David Hildenbrand wrote:
>> 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.
> 
> I thought we have concluded that there is no user expecting a specific
> alignment. Or have I misunderstood that?

We concluded that for users that don't pass in non-power-2 requests.

hugetlb->folio_alloc_gigantic()->alloc_contig_pages_noprof() certainly 
expects a suitable alignment :)

I'll note that if no alignment is required, the whole loop in 
alloc_contig_pages_noprof() is not optimal, because we could be skipping 
suitable ranges. But I guess nobody cares about that right now -- and at 
least it's simple.

-- 
Cheers

David / dhildenb

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

[Michal Hocko]

On Wed 24-09-25 14:19:37, David Hildenbrand wrote:
> On 24.09.25 14:01, Michal Hocko wrote:
[...]
> > I thought we have concluded that there is no user expecting a specific
> > alignment. Or have I misunderstood that?
> 
> We concluded that for users that don't pass in non-power-2 requests.

Ok, my misreading of the discussion then.

> hugetlb->folio_alloc_gigantic()->alloc_contig_pages_noprof() certainly
> expects a suitable alignment :)

True that.

So effectivelly there is nothing to be done here right. Those that
provide power-two requests will get what they have been getting and
those non-existent others will get whatever as we do not provide any
guarantee in that case. Maybe something we could add as a comment.

Makes sense?
-- 
Michal Hocko
SUSE Labs

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

[David Hildenbrand]

On 25.09.25 10:14, Michal Hocko wrote:
> On Wed 24-09-25 14:19:37, David Hildenbrand wrote:
>> On 24.09.25 14:01, Michal Hocko wrote:
> [...]
>>> I thought we have concluded that there is no user expecting a specific
>>> alignment. Or have I misunderstood that?
>>
>> We concluded that for users that don't pass in non-power-2 requests.
> 
> Ok, my misreading of the discussion then.
> 
>> hugetlb->folio_alloc_gigantic()->alloc_contig_pages_noprof() certainly
>> expects a suitable alignment :)
> 
> True that.
> 
> So effectivelly there is nothing to be done here right. Those that
> provide power-two requests will get what they have been getting and
> those non-existent others

I think kfence might be doing a non-power-of-two allocation. TDX maybe 
as well.

But neither seems to depend on some alignment, so all good.

> will get whatever as we do not provide any
> guarantee in that case. Maybe something we could add as a comment.

We currently have

"The allocated memory is always aligned to a page boundary. If nr_pages 
is a power of two, then allocated range is also guaranteed to be aligned 
to same nr_pages (e.g. 1GB request would be aligned to 1GB)."

What would be your suggestion?

-- 
Cheers

David / dhildenb

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

[Michal Hocko]

On Thu 25-09-25 11:22:26, David Hildenbrand wrote:
> On 25.09.25 10:14, Michal Hocko wrote:
> > On Wed 24-09-25 14:19:37, David Hildenbrand wrote:
> > > On 24.09.25 14:01, Michal Hocko wrote:
> > [...]
> > > > I thought we have concluded that there is no user expecting a specific
> > > > alignment. Or have I misunderstood that?
> > > 
> > > We concluded that for users that don't pass in non-power-2 requests.
> > 
> > Ok, my misreading of the discussion then.
> > 
> > > hugetlb->folio_alloc_gigantic()->alloc_contig_pages_noprof() certainly
> > > expects a suitable alignment :)
> > 
> > True that.
> > 
> > So effectivelly there is nothing to be done here right. Those that
> > provide power-two requests will get what they have been getting and
> > those non-existent others
> 
> I think kfence might be doing a non-power-of-two allocation. TDX maybe as
> well.
> 
> But neither seems to depend on some alignment, so all good.
> 
> > will get whatever as we do not provide any
> > guarantee in that case. Maybe something we could add as a comment.
> 
> We currently have
> 
> "The allocated memory is always aligned to a page boundary. If nr_pages is a
> power of two, then allocated range is also guaranteed to be aligned to same
> nr_pages (e.g. 1GB request would be aligned to 1GB)."
> 
> What would be your suggestion?

Suggestion to self to read through the doc before suggesting editings
and suggestion to the patch to not do anything.

Thanks for the patience with me.
-- 
Michal Hocko
SUSE Labs

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

[Dev Jain]

On 23/09/25 5:49 am, 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.
>
> Use roundup() to calculate the correct alignment.
>
> Fixes: 5e27a2df03b8 ("mm/page_alloc: add alloc_contig_pages()")

The ALIGN() statement was even before this, but not sure if a potential
caller with non-power-of-2 nr_pages was present then.

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Brendan Jackman <jackmanb@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Cc: Zi Yan <ziy@nvidia.com>
> ---
>   mm/page_alloc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a1bcc1e003c7..a17a6014e3db 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7095,7 +7095,7 @@ struct page *alloc_contig_pages_noprof(unsigned long nr_pages, gfp_t gfp_mask,
>   					gfp_zone(gfp_mask), nodemask) {
>   		spin_lock_irqsave(&zone->lock, flags);
>   
> -		pfn = ALIGN(zone->zone_start_pfn, nr_pages);
> +		pfn = roundup(zone->zone_start_pfn, nr_pages);
>   		while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
>   			if (pfn_range_valid_contig(pfn, nr_pages)) {
>   				/*

The kernel-doc above alloc_contig_pages_noprof reads:

"If nr_pages is a power of two, then allocated range is always guaranteed
to be aligned to same nr_pages." This means that the function gives no
guarantee on alignment of non-power-of-2 requests - and why should it?
It doesn't make any sense to align pfn to 7 if nr_pages is 7.

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

[Zi Yan]

On 23 Sep 2025, at 2:47, Dev Jain wrote:

> On 23/09/25 5:49 am, 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.
>>
>> Use roundup() to calculate the correct alignment.
>>
>> Fixes: 5e27a2df03b8 ("mm/page_alloc: add alloc_contig_pages()")
>
> The ALIGN() statement was even before this, but not sure if a potential
> caller with non-power-of-2 nr_pages was present then.
>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>> Cc: Oscar Salvador <osalvador@suse.de>
>> Cc: Brendan Jackman <jackmanb@google.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Suren Baghdasaryan <surenb@google.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Wei Yang <richard.weiyang@gmail.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> ---
>>   mm/page_alloc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index a1bcc1e003c7..a17a6014e3db 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -7095,7 +7095,7 @@ struct page *alloc_contig_pages_noprof(unsigned long nr_pages, gfp_t gfp_mask,
>>   					gfp_zone(gfp_mask), nodemask) {
>>   		spin_lock_irqsave(&zone->lock, flags);
>>  -		pfn = ALIGN(zone->zone_start_pfn, nr_pages);
>> +		pfn = roundup(zone->zone_start_pfn, nr_pages);
>>   		while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
>>   			if (pfn_range_valid_contig(pfn, nr_pages)) {
>>   				/*
>
> The kernel-doc above alloc_contig_pages_noprof reads:
>
> "If nr_pages is a power of two, then allocated range is always guaranteed
> to be aligned to same nr_pages." This means that the function gives no
> guarantee on alignment of non-power-of-2 requests - and why should it?
> It doesn't make any sense to align pfn to 7 if nr_pages is 7.

I agree with this. But giving a non power of 2 to ALIGN is unexpected, right?
So maybe only use ALIGN for power of 2 nr_pages and leave other cases alone?
Or like David suggested in another email, align nr_pages down to power of 2.

Something like:

pfn = ALIGN(zone->zone_start_pfn, rounddown_pow_of_two(nr_pages));

Best Regards,
Yan, Zi

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

[Michal Hocko]

On Tue 23-09-25 00:19:43, 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.
> 
> Use roundup() to calculate the correct alignment.

What is the problem you are trying to fix here? The ALIGN is certainly
not well defined for nr_pages that is not power of two but does any
caller of alloc_contig_pages assumes any specific alignment, especially
when nr_pages is not power of two?

> Fixes: 5e27a2df03b8 ("mm/page_alloc: add alloc_contig_pages()")
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Brendan Jackman <jackmanb@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Cc: Zi Yan <ziy@nvidia.com>
> ---
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a1bcc1e003c7..a17a6014e3db 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7095,7 +7095,7 @@ struct page *alloc_contig_pages_noprof(unsigned long nr_pages, gfp_t gfp_mask,
>  					gfp_zone(gfp_mask), nodemask) {
>  		spin_lock_irqsave(&zone->lock, flags);
>  
> -		pfn = ALIGN(zone->zone_start_pfn, nr_pages);
> +		pfn = roundup(zone->zone_start_pfn, nr_pages);
>  		while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
>  			if (pfn_range_valid_contig(pfn, nr_pages)) {
>  				/*
> -- 
> 2.34.1

-- 
Michal Hocko
SUSE Labs

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

[Wei Yang]

On Tue, Sep 23, 2025 at 09:29:12AM +0200, Michal Hocko wrote:
>On Tue 23-09-25 00:19:43, 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.
>> 
>> Use roundup() to calculate the correct alignment.
>
>What is the problem you are trying to fix here? The ALIGN is certainly
>not well defined for nr_pages that is not power of two but does any
>caller of alloc_contig_pages assumes any specific alignment, especially
>when nr_pages is not power of two?
>

There is no real problem here. The initial thought is ALIGN() don't expect
non-power_of_two value.

The discussion above sounds reasonable, we don't need alignment for
non-power_of_two nr_pages.

So this looks we can start iteration from zone->zone_start_pfn for
non-power_of_two nr_pages.

IMHO, How about the change below?

    pfn = zone->zone_start_pfn;
    if (is_power_of_2(nr_pages))
        pfn = ALIGN(pfn, nr_pages)

And it looks making code consist with the doc above.

-- 
Wei Yang
Help you, Help me

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

[Michal Hocko]

On Wed 24-09-25 00:05:28, Wei Yang wrote:
> On Tue, Sep 23, 2025 at 09:29:12AM +0200, Michal Hocko wrote:
> >On Tue 23-09-25 00:19:43, 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.
> >> 
> >> Use roundup() to calculate the correct alignment.
> >
> >What is the problem you are trying to fix here? The ALIGN is certainly
> >not well defined for nr_pages that is not power of two but does any
> >caller of alloc_contig_pages assumes any specific alignment, especially
> >when nr_pages is not power of two?
> >
> 
> There is no real problem here. The initial thought is ALIGN() don't expect
> non-power_of_two value.
> 
> The discussion above sounds reasonable, we don't need alignment for
> non-power_of_two nr_pages.
> 
> So this looks we can start iteration from zone->zone_start_pfn for
> non-power_of_two nr_pages.
> 
> IMHO, How about the change below?
> 
>     pfn = zone->zone_start_pfn;
>     if (is_power_of_2(nr_pages))
>         pfn = ALIGN(pfn, nr_pages)

Why should we do that if we claim there is no alignment enforced and
therefore it shouldn't be expected. Something like this would merely
create a confusion and subtle bugs in the worst case.
-- 
Michal Hocko
SUSE Labs