Re: [PATCH] mm: remove migration for HugePage in isolate_single_pageblock()

[David Hildenbrand <david@redhat.com>]

On 16.08.24 06:06, Kefeng Wang wrote:
> The gigantic page size may larger than memory block size, so memory
> offline always fails in this case after commit b2c9e2fbba32 ("mm: make
> alloc_contig_range work at pageblock granularity"),
> 
> offline_pages
>    start_isolate_page_range
>      start_isolate_page_range(isolate_before=true)
>        isolate [isolate_start, isolate_start + pageblock_nr_pages)
>      start_isolate_page_range(isolate_before=false)
>        isolate [isolate_end - pageblock_nr_pages, isolate_end) pageblock
>         	__alloc_contig_migrate_range
>            isolate_migratepages_range
>              isolate_migratepages_block
>                isolate_or_dissolve_huge_page
>                  if (hstate_is_gigantic(h))
>                      return -ENOMEM;
> 
> In fact, we don't need to migrate page in page range isolation, for
> memory offline path, there is do_migrate_range() to move the pages.
> For contig allocation, there is another __alloc_contig_migrate_range()
> after isolation to migrate the pages. So fix issue by skipping the
> __alloc_contig_migrate_range() in isolate_single_pageblock().
> 
> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/page_isolation.c | 28 +++-------------------------
>   1 file changed, 3 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 39fb8c07aeb7..7e04047977cf 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -403,30 +403,8 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>   			unsigned long head_pfn = page_to_pfn(head);
>   			unsigned long nr_pages = compound_nr(head);
>   
> -			if (head_pfn + nr_pages <= boundary_pfn) {
> -				pfn = head_pfn + nr_pages;
> -				continue;
> -			}
> -
> -#if defined CONFIG_COMPACTION || defined CONFIG_CMA
> -			if (PageHuge(page)) {
> -				int page_mt = get_pageblock_migratetype(page);
> -				struct compact_control cc = {
> -					.nr_migratepages = 0,
> -					.order = -1,
> -					.zone = page_zone(pfn_to_page(head_pfn)),
> -					.mode = MIGRATE_SYNC,
> -					.ignore_skip_hint = true,
> -					.no_set_skip_hint = true,
> -					.gfp_mask = gfp_flags,
> -					.alloc_contig = true,
> -				};
> -				INIT_LIST_HEAD(&cc.migratepages);
> -
> -				ret = __alloc_contig_migrate_range(&cc, head_pfn,
> -							head_pfn + nr_pages, page_mt);
> -				if (ret)
> -					goto failed;

But won't this break alloc_contig_range() then? I would have expected 
that you have to special-case here on the migration reason (MEMORY_OFFLINE).

I remember some dirty details when we're trying to allcoate with a 
single pageblock for alloc_contig_range().

Note that memory offlining always covers pageblocks large than MAX_ORDER 
chunks (which implies full pageblocks) but alloc_contig_range() + CMA 
might only cover (parts of) single pageblocks.

Hoping Zi Yan can review :)

-- 
Cheers,

David / dhildenb