| Message ID | 20240725011647.1306045-5-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: memory_hotplug: improve do_migrate_range() | expand |
On 25.07.24 03:16, Kefeng Wang wrote: > Move isolate_hugetlb() after grab a reference, and use the > isolate_folio_to_list() to unify hugetlb/LRU/non-LRU folio > isolation, which cleanup code a bit and save a few calls to > compound_head(). Will this work with free hugetlb folios that have a refcount of 0 and get_page_unless_zero() would fail?
On 2024/7/30 18:31, David Hildenbrand wrote: > On 25.07.24 03:16, Kefeng Wang wrote: >> Move isolate_hugetlb() after grab a reference, and use the >> isolate_folio_to_list() to unify hugetlb/LRU/non-LRU folio >> isolation, which cleanup code a bit and save a few calls to >> compound_head(). > > Will this work with free hugetlb folios that have a refcount of 0 and > get_page_unless_zero() would fail? Before this changes, isolate_hugetlb() will fail to isolate a ref=0 folio(call folio_try_get()) and now get_page_unless_zero() will fail too, so no behavior change, maybe I miss something?
On 31.07.24 07:13, Kefeng Wang wrote: > > > On 2024/7/30 18:31, David Hildenbrand wrote: >> On 25.07.24 03:16, Kefeng Wang wrote: >>> Move isolate_hugetlb() after grab a reference, and use the >>> isolate_folio_to_list() to unify hugetlb/LRU/non-LRU folio >>> isolation, which cleanup code a bit and save a few calls to >>> compound_head(). >> >> Will this work with free hugetlb folios that have a refcount of 0 and >> get_page_unless_zero() would fail? > > Before this changes, isolate_hugetlb() will fail to isolate a ref=0 > folio(call folio_try_get()) and now get_page_unless_zero() will fail > too, so no behavior change, maybe I miss something? Good point, isolate_hugetlb() will simply do nothing and we skip the folio for this round. Let me take a closer look.
On 25.07.24 03:16, Kefeng Wang wrote: > Move isolate_hugetlb() after grab a reference, and use the > isolate_folio_to_list() to unify hugetlb/LRU/non-LRU folio > isolation, which cleanup code a bit and save a few calls to > compound_head(). > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/memory_hotplug.c | 48 +++++++++++++++------------------------------ > 1 file changed, 16 insertions(+), 32 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index ccaf4c480aed..057037766efa 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1773,20 +1773,17 @@ static int scan_movable_pages(unsigned long start, unsigned long end, > static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > { > unsigned long pfn; > - struct page *page, *head; > LIST_HEAD(source); > + struct folio *folio; > static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > > for (pfn = start_pfn; pfn < end_pfn; pfn++) { > - struct folio *folio; > - bool isolated; > + struct page *page; > > if (!pfn_valid(pfn)) > continue; > page = pfn_to_page(pfn); > - folio = page_folio(page); > - head = &folio->page; > > /* > * HWPoison pages have elevated reference counts so the migration would > @@ -1808,36 +1805,22 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > continue; > } > > - if (PageHuge(page)) { > - pfn = page_to_pfn(head) + compound_nr(head) - 1; > - isolate_hugetlb(folio, &source); > + folio = folio_get_nontail_page(page); > + if (!folio) > continue; There is one interesting case: 1 GiB hugetlb folios can span multiple memory blocks (e.g., 128 MiB). Offlining individual blocks must work. If you do the folio_get_nontail_page() we'd not be able to offline a memory block in the middle anymore, because we'd never try even isolating it. So likely we have to try getting the head page of a large folio instead (as a fallback if this fails?) and continue from there. In case of an free hugetlb tail page we would now iterate each individual page instead of simply jumping forward like the old code would have done. I think we want to maintain that behavior as well? > - } else if (PageTransHuge(page)) > - pfn = page_to_pfn(head) + thp_nr_pages(page) - 1; > - > - if (!get_page_unless_zero(page)) > - continue; > - /* > - * We can skip free pages. And we can deal with pages on > - * LRU and non-lru movable pages. > - */ > - if (PageLRU(page)) > - isolated = isolate_lru_page(page); > - else > - isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE); > - if (isolated) { > - list_add_tail(&page->lru, &source); > - if (!__PageMovable(page)) > - inc_node_page_state(page, NR_ISOLATED_ANON + > - page_is_file_lru(page)); > > - } else { > + /* Skip free folios, deal with hugetlb, LRU and non-lru movable folios. */ Can you clarify what "skip free folios" means? For free folios the folio_get_nontail_page() shouldn't have succeeded. Did you mean if the folio got freed in the meantime? > + if (!isolate_folio_to_list(folio, &source)) { > if (__ratelimit(&migrate_rs)) { > pr_warn("failed to isolate pfn %lx\n", pfn); > dump_page(page, "isolation failed"); > } > }
On 2024/8/2 4:23, David Hildenbrand wrote: > On 25.07.24 03:16, Kefeng Wang wrote: >> Move isolate_hugetlb() after grab a reference, and use the >> isolate_folio_to_list() to unify hugetlb/LRU/non-LRU folio >> isolation, which cleanup code a bit and save a few calls to >> compound_head(). >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> mm/memory_hotplug.c | 48 +++++++++++++++------------------------------ >> 1 file changed, 16 insertions(+), 32 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index ccaf4c480aed..057037766efa 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -1773,20 +1773,17 @@ static int scan_movable_pages(unsigned long >> start, unsigned long end, >> static void do_migrate_range(unsigned long start_pfn, unsigned long >> end_pfn) >> { >> unsigned long pfn; >> - struct page *page, *head; >> LIST_HEAD(source); >> + struct folio *folio; >> static DEFINE_RATELIMIT_STATE(migrate_rs, >> DEFAULT_RATELIMIT_INTERVAL, >> DEFAULT_RATELIMIT_BURST); >> for (pfn = start_pfn; pfn < end_pfn; pfn++) { >> - struct folio *folio; >> - bool isolated; >> + struct page *page; >> if (!pfn_valid(pfn)) >> continue; >> page = pfn_to_page(pfn); >> - folio = page_folio(page); >> - head = &folio->page; >> /* >> * HWPoison pages have elevated reference counts so the >> migration would >> @@ -1808,36 +1805,22 @@ static void do_migrate_range(unsigned long >> start_pfn, unsigned long end_pfn) >> continue; >> } >> - if (PageHuge(page)) { >> - pfn = page_to_pfn(head) + compound_nr(head) - 1; >> - isolate_hugetlb(folio, &source); >> + folio = folio_get_nontail_page(page); >> + if (!folio) >> continue; > > There is one interesting case: 1 GiB hugetlb folios can span multiple > memory blocks (e.g., 128 MiB). Offlining individual blocks must work. > > If you do the folio_get_nontail_page() we'd not be able to offline a > memory block in the middle anymore, because we'd never try even > isolating it. Indeed, will test this case. > > So likely we have to try getting the head page of a large folio instead > (as a fallback if this fails?) and continue from there. > > In case of an free hugetlb tail page we would now iterate each > individual page instead of simply jumping forward like the old code > would have done. I think we want to maintain that behavior as well? Yes, only can occur when the first hugetlb page if start_pfn is not head page, will reconsider of this part > >> - } else if (PageTransHuge(page)) >> - pfn = page_to_pfn(head) + thp_nr_pages(page) - 1; >> - >> - if (!get_page_unless_zero(page)) >> - continue; >> - /* >> - * We can skip free pages. And we can deal with pages on >> - * LRU and non-lru movable pages. >> - */ >> - if (PageLRU(page)) >> - isolated = isolate_lru_page(page); >> - else >> - isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE); >> - if (isolated) { >> - list_add_tail(&page->lru, &source); >> - if (!__PageMovable(page)) >> - inc_node_page_state(page, NR_ISOLATED_ANON + >> - page_is_file_lru(page)); >> - } else { >> + /* Skip free folios, deal with hugetlb, LRU and non-lru >> movable folios. */ > > Can you clarify what "skip free folios" means? For free folios the > folio_get_nontail_page() shouldn't have succeeded. Did you mean if the > folio got freed in the meantime? I think we could drop the comments here, the original comment is added by commit 0c0e61958965 ("memory unplug: page offline"), and there is no increase the reference to page, but with commit 700c2a46e882 ("mem- hotplug: call isolate_lru_page with elevated refcount"), the comment could be dropped since the folio can't be freed here. > >> + if (!isolate_folio_to_list(folio, &source)) { >> if (__ratelimit(&migrate_rs)) { >> pr_warn("failed to isolate pfn %lx\n", pfn); >> dump_page(page, "isolation failed"); >> } >> } > >
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index ccaf4c480aed..057037766efa 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1773,20 +1773,17 @@ static int scan_movable_pages(unsigned long start, unsigned long end, static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) { unsigned long pfn; - struct page *page, *head; LIST_HEAD(source); + struct folio *folio; static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); for (pfn = start_pfn; pfn < end_pfn; pfn++) { - struct folio *folio; - bool isolated; + struct page *page; if (!pfn_valid(pfn)) continue; page = pfn_to_page(pfn); - folio = page_folio(page); - head = &folio->page; /* * HWPoison pages have elevated reference counts so the migration would @@ -1808,36 +1805,22 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) continue; } - if (PageHuge(page)) { - pfn = page_to_pfn(head) + compound_nr(head) - 1; - isolate_hugetlb(folio, &source); + folio = folio_get_nontail_page(page); + if (!folio) continue; - } else if (PageTransHuge(page)) - pfn = page_to_pfn(head) + thp_nr_pages(page) - 1; - - if (!get_page_unless_zero(page)) - continue; - /* - * We can skip free pages. And we can deal with pages on - * LRU and non-lru movable pages. - */ - if (PageLRU(page)) - isolated = isolate_lru_page(page); - else - isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE); - if (isolated) { - list_add_tail(&page->lru, &source); - if (!__PageMovable(page)) - inc_node_page_state(page, NR_ISOLATED_ANON + - page_is_file_lru(page)); - } else { + /* Skip free folios, deal with hugetlb, LRU and non-lru movable folios. */ + if (!isolate_folio_to_list(folio, &source)) { if (__ratelimit(&migrate_rs)) { pr_warn("failed to isolate pfn %lx\n", pfn); dump_page(page, "isolation failed"); } } - put_page(page); + + if (folio_test_large(folio)) + pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1; + + folio_put(folio); } if (!list_empty(&source)) { nodemask_t nmask = node_states[N_MEMORY]; @@ -1852,7 +1835,7 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) * We have checked that migration range is on a single zone so * we can use the nid of the first page to all the others. */ - mtc.nid = page_to_nid(list_first_entry(&source, struct page, lru)); + mtc.nid = folio_nid(list_first_entry(&source, struct folio, lru)); /* * try to allocate from a different node but reuse this node @@ -1865,11 +1848,12 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) ret = migrate_pages(&source, alloc_migration_target, NULL, (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG, NULL); if (ret) { - list_for_each_entry(page, &source, lru) { + list_for_each_entry(folio, &source, lru) { if (__ratelimit(&migrate_rs)) { pr_warn("migrating pfn %lx failed ret:%d\n", - page_to_pfn(page), ret); - dump_page(page, "migration failure"); + folio_pfn(folio), ret); + dump_page(&folio->page, + "migration failure"); } } putback_movable_pages(&source);
Move isolate_hugetlb() after grab a reference, and use the isolate_folio_to_list() to unify hugetlb/LRU/non-LRU folio isolation, which cleanup code a bit and save a few calls to compound_head(). Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/memory_hotplug.c | 48 +++++++++++++++------------------------------ 1 file changed, 16 insertions(+), 32 deletions(-)