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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/8/16 18:11, David Hildenbrand wrote:
> 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).
> 

Yes, this is what I did in rfc, only skip migration for offline path.
but Zi Yan suggested to remove migration totally[1]

[1] 
https://lore.kernel.org/linux-mm/50FEEE33-49CA-48B5-B4C5-964F1BE25D43@nvidia.com/

> 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 :)
>