Some cleanups for page isolation
Subsystems (8)
Baolin Wang
reviewercover Some cleanups for page isolation
Hi,
The page isolation functions did not return a boolean to indicate success or not, instead it will return a negative error when failed to isolate a page. So it's better to check the negative error explicitly for isolation to make the code more clear per Linus's suggestion in [1].
No functional changes intended in this patch series.
Baolin Wang (3):
mm: check negative error of folio_isolate_lru() when failed to isolate
a folio
mm: check negative error of isolate_lru_page() when failed to isolate
a page
mm: mempolicy: check negative error of isolate_hugetlb() when failed
to isolate a hugetlb
mm/damon/paddr.c | 2 +- mm/gup.c | 2 +- mm/khugepaged.c | 4 ++-- mm/memcontrol.c | 2 +- mm/mempolicy.c | 2 +- mm/migrate.c | 4 ++-- mm/migrate_device.c | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-)
Signature · 1 line
2.27.0
Baolin Wang
reviewerPATCH 1/3 mm: check negative error of folio_isolate_lru() when failed to isolate a folio
The folio_isolate_lru() will return a negative error if failed to isolate a folio from its LRU list, thus better to check the negative error to make the code more clear per Linus's suggestion[1].
No functional changes.
--- mm/damon/paddr.c | 2 +- mm/gup.c | 2 +- mm/khugepaged.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
mm/damon/paddr.c +1 -1
@@ -246,7 +246,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)folio_clear_referenced (folio);folio_test_clear_young (folio); -if (folio_isolate_lru (folio)) { +if (folio_isolate_lru (folio) <0 ) {folio_put (folio);continue ; }
mm/gup.c +1 -1
@@ -1939,7 +1939,7 @@ static unsigned long collect_longterm_unpinnable_pages( drain_allow = false; } -if (folio_isolate_lru (folio)) +if (folio_isolate_lru (folio) <0 )continue ;list_add_tail (&folio->lru, movable_page_list);
mm/khugepaged.c +1 -1
@@ -2047,7 +2047,7 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, goto out_unlock; } -if (folio_isolate_lru (folio)) { +if (folio_isolate_lru (folio) <0 ) { result =SCAN_DEL_PAGE_LRU ; goto out_unlock; }
Signature · 1 line
2.27.0
Baolin Wang
reviewerPATCH 2/3 mm: check negative error of isolate_lru_page() when failed to isolate a page
In reply to Baolin WangBetter to check the negative error of isolate_lru_page() when failed to isolate a page from its LRU, which makes the code more clear.
No functional changes.
--- mm/khugepaged.c | 2 +- mm/memcontrol.c | 2 +- mm/migrate.c | 4 ++-- mm/migrate_device.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
mm/khugepaged.c +1 -1
@@ -659,7 +659,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,* Isolate the page to avoid collapsing an hugepage * currently in use by the VM. */ -if (isolate_lru_page (page)) { +if (isolate_lru_page (page) <0 ) {unlock_page (page); result =SCAN_DEL_PAGE_LRU ; goto out;
mm/memcontrol.c +1 -1
@@ -6226,7 +6226,7 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,*/ if (PageTransCompound (page)) goto put; -if (!device &&isolate_lru_page (page)) +if (!device &&isolate_lru_page (page) <0 ) goto put;if (!mem_cgroup_move_account (page, false, mc.from, mc.to)) {
mm/migrate.c +2 -2
@@ -2135,7 +2135,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, head =compound_head (page); err =isolate_lru_page (head); -if (err) +if (err <0 ) goto out_putpage; err =1 ;
@@ -2541,7 +2541,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)return 0 ; } -if (isolate_lru_page (page)) +if (isolate_lru_page (page) <0 )return 0 ; mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_is_file_lru(page),
mm/migrate_device.c +1 -1
@@ -388,7 +388,7 @@ static unsigned long migrate_device_unmap(unsigned long *src_pfns, allow_drain = false; } -if (isolate_lru_page (page)) { +if (isolate_lru_page (page) <0 ) { src_pfns[i] &= ~MIGRATE_PFN_MIGRATE ; restore++;continue ;
Signature · 1 line
2.27.0
Baolin Wang
reviewerPATCH 3/3 mm: mempolicy: check negative error of isolate_hugetlb() when failed to isolate a hugetlb
In reply to Baolin WangBetter to check the negative error of isolate_hugetlb() when failed to isolate a hugetlb page, which makes the code more clear.
No functional changes.
--- mm/mempolicy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/mempolicy.c +1 -1
@@ -609,7 +609,7 @@ static int queue_folios_hugetlb(pte_t *pte, unsigned long hmask,if (flags & (MPOL_MF_MOVE_ALL ) || (flags &MPOL_MF_MOVE &&folio_estimated_sharers (folio) ==1 && !hugetlb_pmd_shared (pte))) { -if (isolate_hugetlb (folio, qp->pagelist) && +if (isolate_hugetlb (folio, qp->pagelist) <0 && (flags &MPOL_MF_STRICT ))/* * Failed to isolate folio but allow migrating pages
Signature · 1 line
2.27.0
Quoted 4 lines · Baolin Wang
The page isolation functions did not return a boolean to indicate success or not, instead it will return a negative error when failed to isolate a page. So it's better to check the negative error explicitly for isolation to make the code more clear per Linus's suggestion in [1].
Only one caller of isolate_lru_page() or folio_isolate_lru() actually uses the errno. And the errno can only be 0 or -EBUSY. It'd be better to change the three functions to return bool and fix add_page_for_migration() to set the errno to -EBUSY itself.
Baolin Wang
reviewerQuoted 10 lines · Matthew Wilcox
Quoted 4 lines · Baolin Wang
The page isolation functions did not return a boolean to indicate success or not, instead it will return a negative error when failed to isolate a page. So it's better to check the negative error explicitly for isolation to make the code more clear per Linus's suggestion in [1].
Only one caller of
isolate_lru_page()orfolio_isolate_lru()actually uses the errno. And the errno can only be 0 or-EBUSY. It'd be better to change the three functions to return bool and fixadd_page_for_migration()to set the errno to-EBUSYitself.
Sounds reasonable to me, and I can change them to return bool in next version. Thanks.