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

[Michal Hocko <mhocko@suse.com>]

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