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

[Michal Hocko]

On Wed 16-10-19 21:01:19, Anshuman Khandual wrote:
> 
> 
> On 10/16/2019 06:11 PM, Michal Hocko wrote:
> > On Wed 16-10-19 14:29:05, David Hildenbrand wrote:
> >> On 16.10.19 13:51, Michal Hocko wrote:
> >>> On Wed 16-10-19 16:43:57, Anshuman Khandual wrote:
> >>>>
> >>>>
> >>>> On 10/16/2019 04:39 PM, David Hildenbrand wrote:
> >>> [...]
> >>>>> Just to make sure, you ignored my comment regarding alignment
> >>>>> although I explicitly mentioned it a second time? Thanks.
> >>>>
> >>>> I had asked Michal explicitly what to be included for the respin. Anyways
> >>>> seems like the previous thread is active again. I am happy to incorporate
> >>>> anything new getting agreed on there.
> >>>
> >>> Your patch is using the same alignment as the original code would do. If
> >>> an explicit alignement is needed then this can be added on top, right?
> >>>
> >>
> >> Again, the "issue" I see here is that we could now pass in numbers that are
> >> not a power of two. For gigantic pages it was clear that we always have a
> >> number of two. The alignment does not make any sense otherwise.
> 
> ALIGN() does expect nr_pages two be power of two otherwise the mask
> value might not be correct, affecting start pfn value for a zone.
> 
> #define ALIGN(x, a)             	__ALIGN_KERNEL((x), (a))
> #define __ALIGN_KERNEL(x, a)            __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
> #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))

Yes it doesn't really provide a sensible result but does it matter?
 
> >> What I'm asking for is
> >>
> >> a) Document "The resulting PFN is aligned to nr_pages" and "nr_pages should
> >> be a power of two".
> > 
> > OK, this makes sense.
> Sure, will add this to the alloc_contig_pages() helper description and
> in the commit message as well.
> 
> > 
> >> b) Eventually adding something like
> >>
> >> if (WARN_ON_ONCE(!is_power_of_2(nr_pages)))
> >> 	return NULL;
> > 
> > I am not sure this is really needed.
> > 
> Just wondering why not ? Ideally if we are documenting that nr_pages should be
> power of two, then we should abort erring allocation request with an warning. Is
> it because allocation can still succeed for non-power-of-two requests despite
> possible problem with mask and alignment ? Hence there is no need to abort it.

What would we do about the warning? There are only three ways to go
about this a) reguire power of two nr_pages and always align to that b)
do not restrict nr_pages and align implicitly to what makes sense (power
of two on proper size and arbitrary page aligned otherwise) and c) allow
an explicit alignment.

The simplest way forward is b) because that doesn't really require any
code changes. And I thought the main point of this patch was to provide
something as simple as possible. a) would be only slightly more
complicated but I am wondering why should be restrict the size of the
allocation when there is nothing inherent to do so.
-- 
Michal Hocko
SUSE Labs