Re: [PATCH RFC] mm: skip gigantic pages in isolate_single_pageblock() when mem offline

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

On 2024/8/13 22:03, Matthew Wilcox wrote:
> On Tue, Aug 13, 2024 at 08:52:26PM +0800, 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;
>>
>> [   15.815756] memory offlining [mem 0x3c0000000-0x3c7ffffff] failed due to failure to isolate range
>>
>> Fix it by skipping the __alloc_contig_migrate_range() if met gigantic
>> pages when memory offline, which return back to the original logic to
>> handle the gigantic pages.
> 
> This seems like the wrong way to fix this.  The logic in the next
> PageHuge() section seems like it's specifically supposed to handle
> gigantic pages.  So you've just made that dead code, but instead of
> removing it, you've left it there to confuse everyone?

isolate_single_pageblock() in start_isolate_page_range() will be called
from memory offline and contig allocation (alloc_contig_pages()), this
changes only restore the behavior from memory offline code, but we still
fail in contig allocation.

 From memory offline, we has own path to isolate/migrate page or dissolve
free hugetlb folios, so I think we don't depends on the 
__alloc_contig_migrate_range().
> 
> I admit to not understanding this code terribly well.
> 
A quick search from [1], the isolate_single_pageblock() is added for
contig allocation, but it has negative effects on memory hotplug,
Zi Yan, could you give some comments?

[1] 
https://lore.kernel.org/linux-mm/20220425143118.2850746-1-zi.yan@sent.com/

>> Fixes: b2c9e2fbba32 ("mm: make alloc_contig_range work at pageblock granularity")
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/page_isolation.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>> index 042937d5abe4..25db4040e70a 100644
>> --- a/mm/page_isolation.c
>> +++ b/mm/page_isolation.c
>> @@ -400,6 +400,16 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
>>   				continue;
>>   			}
>>   
>> +			if ((flags & MEMORY_OFFLINE) && PageHuge(page)) {
>> +				struct hstate *h;
>> +
>> +				h = size_to_hstate(nr_pages << PAGE_SHIFT);
>> +				if (hstate_is_gigantic(h)) {
>> +					pfn = head_pfn + nr_pages;
>> +					continue;
>> +				}
>> +			}
>> +
>>   #if defined CONFIG_COMPACTION || defined CONFIG_CMA
>>   			if (PageHuge(page)) {
>>   				int page_mt = get_pageblock_migratetype(page);
>> -- 
>> 2.27.0
>>
>>
>