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
