| Message ID | 20240327141034.3712697-3-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: remove isolate_lru_page() and isolate_movable_page() | expand |
On 27 Mar 2024, at 10:10, Kefeng Wang wrote: > With isolate_movable_folio() and folio_isolate_lru(), let's use > more folio in do_migrate_range() to save compound_head() calls. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/memory_hotplug.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index a444e2d7dd2b..bd207772c619 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1774,14 +1774,14 @@ static int scan_movable_pages(unsigned long start, unsigned long end, > > static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > { > + struct folio *folio; > unsigned long pfn; > - struct page *page, *head; > LIST_HEAD(source); > static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > > for (pfn = start_pfn; pfn < end_pfn; pfn++) { > - struct folio *folio; > + struct page *page, *head; You could get rid of head too. It is only used to calculate next pfn, so pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1 would work. And the PageHuge(page) and PageTransHuge(page) can be simplified, since their pfn calculations are the same. Something like: if (folio_test_large(folio)) { pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1; if (folio_test_hugetlb(folio)) { isolate_hugetlb(folio, &source); continue; } } -- Best Regards, Yan, Zi
On Wed, Mar 27, 2024 at 10:45:42AM -0400, Zi Yan wrote: > > for (pfn = start_pfn; pfn < end_pfn; pfn++) { > > - struct folio *folio; > > + struct page *page, *head; > > You could get rid of head too. It is only used to calculate next pfn, > so pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1 would work. > > And the PageHuge(page) and PageTransHuge(page) can be simplified, since > their pfn calculations are the same. Something like: > > if (folio_test_large(folio)) { > pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1; > if (folio_test_hugetlb(folio)) { > isolate_hugetlb(folio, &source); > continue; > } > } How much of this is safe without a refcount on the folio?
On 27 Mar 2024, at 10:54, Matthew Wilcox wrote: > On Wed, Mar 27, 2024 at 10:45:42AM -0400, Zi Yan wrote: >>> for (pfn = start_pfn; pfn < end_pfn; pfn++) { >>> - struct folio *folio; >>> + struct page *page, *head; >> >> You could get rid of head too. It is only used to calculate next pfn, >> so pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1 would work. >> >> And the PageHuge(page) and PageTransHuge(page) can be simplified, since >> their pfn calculations are the same. Something like: >> >> if (folio_test_large(folio)) { >> pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1; >> if (folio_test_hugetlb(folio)) { >> isolate_hugetlb(folio, &source); >> continue; >> } >> } > > How much of this is safe without a refcount on the folio? folio_to_pfn() should be fine, isolate_hugetlb() checks the folio under hugetlb_lock, but folio_nr_pages() might return a bogus number that makes pfn go beyond end_pfn and ends for loop early. The code below increases the refcount, so it might be better to move this part of the code after refcount is increased. -- Best Regards, Yan, Zi
On Wed, Mar 27, 2024 at 11:10:48AM -0400, Zi Yan wrote: > On 27 Mar 2024, at 10:54, Matthew Wilcox wrote: > > How much of this is safe without a refcount on the folio? > > folio_to_pfn() should be fine, isolate_hugetlb() checks the folio > under hugetlb_lock, but folio_nr_pages() might return a bogus number > that makes pfn go beyond end_pfn and ends for loop early. The code > below increases the refcount, so it might be better to move this > part of the code after refcount is increased. I really want to instill a little bit of fear in Kefeng. This is all really subtle code because it's running without a refcount. I've mostly stayed away from it because I know that I don't know what I'm doing. Kefeng has no idea that he doesn't know what he's doing. And so we get these patches, and they're sometimes subtly wrong, and it takes a lot of arguing and revision and thinking and Kefeng is doing very little of the thinking part! Kefeng, please stick to working on code that you understand. Or take the time to learn code you don't understand before sending patches to it. This kind of Leeroy Jenkins approach to development is not good.
On 2024/3/27 23:10, Zi Yan wrote: > On 27 Mar 2024, at 10:54, Matthew Wilcox wrote: > >> On Wed, Mar 27, 2024 at 10:45:42AM -0400, Zi Yan wrote: >>>> for (pfn = start_pfn; pfn < end_pfn; pfn++) { >>>> - struct folio *folio; >>>> + struct page *page, *head; >>> >>> You could get rid of head too. It is only used to calculate next pfn, >>> so pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1 would work. >>> >>> And the PageHuge(page) and PageTransHuge(page) can be simplified, since >>> their pfn calculations are the same. Something like: >>> >>> if (folio_test_large(folio)) { >>> pfn = folio_to_pfn(folio) + folio_nr_pages(folio) - 1; >>> if (folio_test_hugetlb(folio)) { >>> isolate_hugetlb(folio, &source); >>> continue; >>> } >>> } >> >> How much of this is safe without a refcount on the folio? > > folio_to_pfn() should be fine, isolate_hugetlb() checks the folio > under hugetlb_lock, but folio_nr_pages() might return a bogus number > that makes pfn go beyond end_pfn and ends for loop early. The code > below increases the refcount, so it might be better to move this > part of the code after refcount is increased. The PageHWPoison() is per-page check, for hugetlb, it will directly try to isolate and ignore the PageHWPoison check, I'm not sure about moveing PageHWPoison ahead, we need to take the i_mmap_lock_write and add TTU_RMAP_LOCKED for for hugetlb pages in shared mappings when try_to_unmap(), but now hugetlb pages won't unmap if there is posion page, if the get_page_unless_zero() is moved ahead, the logical need be changed a lot, this minimize changes aim to remove isolate_lru/movable_page, so could we keep it simple, but as your suggested, we could do more optimization about do_migrate_range() in the next. Thanks. > > -- > Best Regards, > Yan, Zi
On 2024/3/27 23:58, Matthew Wilcox wrote: > On Wed, Mar 27, 2024 at 11:10:48AM -0400, Zi Yan wrote: >> On 27 Mar 2024, at 10:54, Matthew Wilcox wrote: >>> How much of this is safe without a refcount on the folio? >> >> folio_to_pfn() should be fine, isolate_hugetlb() checks the folio >> under hugetlb_lock, but folio_nr_pages() might return a bogus number >> that makes pfn go beyond end_pfn and ends for loop early. The code >> below increases the refcount, so it might be better to move this >> part of the code after refcount is increased. > > I really want to instill a little bit of fear in Kefeng. > > This is all really subtle code because it's running without a refcount. > I've mostly stayed away from it because I know that I don't know what > I'm doing. Kefeng has no idea that he doesn't know what he's doing. > > And so we get these patches, and they're sometimes subtly wrong, and it > takes a lot of arguing and revision and thinking and Kefeng is doing > very little of the thinking part! > > Kefeng, please stick to working on code that you understand. Or take > the time to learn code you don't understand before sending patches to > it. This kind of Leeroy Jenkins approach to development is not good. Oh, I remember your reminder and be in awe of changes and try to think more about changes, for this one, I only convert page to folio after refcount increased with get_page_unless_zero(), and as replied to Zi, PageHWPoison part need more consideration and this one only aims to remove isolate_lru/movable_page, so don't touch the page before get_page_unless_zero(). Thanks for your review and help all the time.
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index a444e2d7dd2b..bd207772c619 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1774,14 +1774,14 @@ static int scan_movable_pages(unsigned long start, unsigned long end, static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) { + struct folio *folio; unsigned long pfn; - struct page *page, *head; LIST_HEAD(source); static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); for (pfn = start_pfn; pfn < end_pfn; pfn++) { - struct folio *folio; + struct page *page, *head; bool isolated; if (!pfn_valid(pfn)) @@ -1818,15 +1818,15 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) * 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); + if (folio_test_lru(folio)) + isolated = folio_isolate_lru(folio); else - isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE); + isolated = isolate_movable_folio(folio, 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)); + list_add_tail(&folio->lru, &source); + if (!__folio_test_movable(folio)) + node_stat_add_folio(folio, NR_ISOLATED_ANON + + folio_is_file_lru(folio)); } else { if (__ratelimit(&migrate_rs)) { @@ -1834,7 +1834,7 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) dump_page(page, "isolation failed"); } } - put_page(page); + folio_put(folio); } if (!list_empty(&source)) { nodemask_t nmask = node_states[N_MEMORY]; @@ -1846,9 +1846,9 @@ 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. + * we can use the nid of the first folio 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 @@ -1861,11 +1861,11 @@ 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);
With isolate_movable_folio() and folio_isolate_lru(), let's use more folio in do_migrate_range() to save compound_head() calls. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/memory_hotplug.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)