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

[Michal Hocko <mhocko@suse.com>]

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