Re: [PATCH V2] mm/page_alloc: Add alloc_contig_pages()

[David Hildenbrand]

On 17.10.19 09:34, Michal Hocko wrote:
> On Thu 17-10-19 09:21:24, David Hildenbrand wrote:
> > On 17.10.19 09:11, Michal Hocko wrote:
> > > On Thu 17-10-19 10:44:41, Anshuman Khandual wrote:
> > > [...]
> > > > Does this add-on documentation look okay ? Should we also mention about the
> > > > possible reduction in chances of success during pfn block search for the
> > > > non-power-of-two cases as the implicit alignment will probably turn out to
> > > > be bigger than nr_pages itself ?
> > > >
> > > >    * Requested nr_pages may or may not be power of two. The search for suitable
> > > >    * memory range in a zone happens in nr_pages aligned pfn blocks. But in case
> > > >    * when nr_pages is not power of two, an implicitly aligned pfn block search
> > > >    * will happen which in turn will impact allocated memory block's alignment.
> > > >    * In these cases, the size (i.e nr_pages) and the alignment of the allocated
> > > >    * memory will be different. This problem does not exist when nr_pages is power
> > > >    * of two where the size and the alignment of the allocated memory will always
> > > >    * be nr_pages.
> > >
> > > I dunno, it sounds more complicated than really necessary IMHO. Callers
> > > shouldn't really be bothered by memory blocks and other really deep
> > > implementation details.. Wouldn't be the below sufficient?
> > >
> > > The allocated memory is always aligned to a page boundary. If nr_pages
> > > is a power of two then the alignement is guaranteed to be to the given
> >
> > s/alignement/alignment/
> >
> > and "the PFN is guaranteed to be aligned to nr_pages" (the address is
> > aligned to nr_pages*PAGE_SIZE)
>
> thx for the correction.
>
> > > nr_pages (e.g. 1GB request would be aligned to 1GB).
> >
> > I'd probably add "This function will miss allocation opportunities if
> > nr_pages is not a power of two (and the implicit alignment is bogus)."
>
> This is again an implementation detail and quite a confusing one to
> whoever not familiar with the MM internals. And to be fair even a proper
> alignment doesn't give you any stronger guarantee as long as the
> allocation operates on non movable zones anyway.

To be honest, I'd not suggest to anyone to use this function with 
nr_pages not being a power of two, and I already explained why. I prefer 
to spill that out than having people complain afterwards. Yes it's an 
implementation detail users should be aware of until reworked.

But I think we talked about this here for way too long, so I am fine 
with either.

--

Thanks,

David / dhildenb