Re: + mm-hugetlb-make-alloc_gigantic_page-available-for-general-use.patch added to -mm tree

[Michal Hocko <mhocko@kernel.org>]

On Mon 14-10-19 18:38:52, Anshuman Khandual wrote:
> 
> 
> On 10/14/2019 06:30 PM, Michal Hocko wrote:
> > On Mon 14-10-19 18:23:00, Anshuman Khandual wrote:
> >>
> >>
> >> On 10/14/2019 05:47 PM, Michal Hocko wrote:
> >>> On Fri 11-10-19 13:29:32, Andrew Morton wrote:
> >>>> alloc_gigantic_page() implements an allocation method where it scans over
> >>>> various zones looking for a large contiguous memory block which could not
> >>>> have been allocated through the buddy allocator.  A subsequent patch which
> >>>> tests arch page table helpers needs such a method to allocate PUD_SIZE
> >>>> sized memory block.  In the future such methods might have other use cases
> >>>> as well.  So alloc_gigantic_page() has been split carving out actual
> >>>> memory allocation method and made available via new
> >>>> alloc_gigantic_page_order().
> >>>
> >>> You are exporting a helper used for hugetlb internally. Is this really
> >>
> >> Right, because the helper i.e alloc_gigantic_page() is generic enough which
> >> scans over various zones to allocate a page order which could not be allocated
> >> through the buddy. Only thing which is HugeTLB specific in there, is struct
> >> hstate from where the order is derived with huge_page_order(). Otherwise it
> >> is very generic.
> >>
> >>> what is needed? I haven't followed this patchset but don't you simply
> >>
> >> Originally I had just implemented similar allocator inside the test itself
> >> but then figured that alloc_gigantic_page() can be factored out to create
> >> a generic enough allocator helper.
> >>
> >>> need a generic 1GB allocator? If yes then you should be looking at
> >>
> >> The test needs a PUD_SIZE allocator.
> >>
> >>> alloc_contig_range.
> >>
> >> IIUC alloc_contig_range() requires (start pfn, end pfn) for the region to be
> >> allocated. But before that all applicable zones need to be scanned to figure
> >> out any available and suitable pfn range for alloc_contig_range() to try. In
> >> this case pfn_range_valid_gigantic() check seemed reasonable while scanning
> >> the zones.
> >>
> >> If pfn_range_valid_gigantic() is good enough or could be made more generic,
> >> then the new factored alloc_gigantic_page_order() could be made a helper in
> >> mm/page_alloc.c
> > 
> > OK, thanks for the clarification. This all means that this patch is not
> > the right approach. If you need a more generic alloc_contig_range then
> > add it to page_alloc.c and make it completely independent on the hugetlb
> > config and the code. Hugetlb allocator can reuse that helper.
> 
> Sure. Just to confirm again, the checks on pfns in pfn_range_valid_gigantic()
> while looking for contiguous memory is generic enough in it's current form ?
> 
> static bool pfn_range_valid_gigantic(struct zone *z,
>                         unsigned long start_pfn, unsigned long nr_pages)
> {
>         unsigned long i, end_pfn = start_pfn + nr_pages;
>         struct page *page;
> 
>         for (i = start_pfn; i < end_pfn; i++) {
>                 if (!pfn_valid(i))
>                         return false;
> 
>                 page = pfn_to_page(i);
> 
>                 if (page_zone(page) != z)
>                         return false;
> 
>                 if (PageReserved(page))
>                         return false;
> 
>                 if (page_count(page) > 0)
>                         return false;
> 
>                 if (PageHuge(page))
>                         return false;
>         }
> 
>         return true;
> }

Yes, I do not think we want to allow allocating cross zones or other
used memory. This is more of a quick check than a correctness one. The
allocator has to make sure that all that is properly done while
isolating memory.

-- 
Michal Hocko
SUSE Labs