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

[David Hildenbrand <david@redhat.com>]

On 16.08.24 13:30, Kefeng Wang wrote:
> 
> 
> 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]

Please distill some of that in the patch description. Right now you only 
talk about memory offlining and don't cover why alloc_contig_range() is 
fine as well with this change.

Let me explore the details in the meantime ... :)

-- 
Cheers,

David / dhildenb