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

[Michal Hocko <mhocko@suse.com>]

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