On 2024/8/14 22:53, Zi Yan wrote:
On 13 Aug 2024, at 22:01, Kefeng Wang wrote:
On 2024/8/13 22:59, Zi Yan wrote:
On 13 Aug 2024, at 10:46, Kefeng Wang wrote:
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@xxxxxxxx/
Probably we can isolate the hugetlb page and use migrate_page() instead of
__alloc_contig_migrate_range() in the section below, since we are targeting
only hugetlb pages here. It should solve the issue.
For contig allocation, I think we must isolate/migrate page in
__alloc_contig_migrate_range(), but for memory offline,(especially for
gigantic hugepage)as mentioned above, we already have own path to
isolate/migrate used page and dissolve the free pages,the
start_isolate_page_range() only need to mark page range MIGRATE_ISOLATE,
that is what we did before b2c9e2fbba32,
start_isolate_page_range
scan_movable_pages
do_migrate_range
dissolve_free_hugetlb_folios
Do we really need isolate/migrate the hugetlb page and for memory
offline path?
For memory offline path, there is do_migrate_range() to move the pages.
For contig allocation, there is __alloc_contig_migrate_range() after
isolation to migrate the pages.
The migration code in isolate_single_pageblock() is not needed.
Something like this would be OK, just skip the page and let either
do_migrate_range() or __alloc_contig_migrate_range() to handle it:
Oh, right, for alloc_contig_range(), we do have another
__alloc_contig_migrate_range() after start_isolate_page_range(), then we
could drop the following code,
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 042937d5abe4..587d723711c5 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -402,23 +402,6 @@ static int isolate_single_pageblock(unsigned long boundary_pfn, int flags,
#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;
pfn = head_pfn + nr_pages;
continue;
}
But we need to remove the CONFIG_COMPACTION/CMA too, thought?
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 042937d5abe4..785c2d320631 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -395,30 +395,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;
+ if (head_pfn + nr_pages <= boundary_pfn ||
+ PageHuge(page))
pfn = head_pfn + nr_pages;
continue;
}
@@ -432,7 +410,6 @@ static int isolate_single_pageblock(unsigned long
boundary_pfn, int flags,
*/
VM_WARN_ON_ONCE_PAGE(PageLRU(page), page);
VM_WARN_ON_ONCE_PAGE(__PageMovable(page), page);
-#endif
goto failed;
}
