mm, migrate: Check return value of try_to_unmap()
Subsystems (2)
PATCH 1/1 mm, migrate: Check return value of try_to_unmap()
From: Huang Ying <[email protected]>
During the page migration, try_to_unmap() is called to replace the page table entries with the migration entries. Now its return value is ignored, that is generally OK in most cases. But in theory, it's possible that try_to_unmap() return false (failure) for the page migration after arch_unmap_one() is called in unmap code. Even if without arch_unmap_one(), it makes code more robust for the future code changing to check the return value.
Known issue: I don't know what is the appropriate error code for try_to_unmap() failure. Whether EIO is OK?
--- mm/migrate.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
mm/migrate.c +12 -6
@@ -1116,8 +1116,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage,/* Establish migration ptes */ VM_BUG_ON_PAGE (PageAnon (page) && !PageKsm (page) && !anon_vma, page); -try_to_unmap (page, -TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS ); +if (!try_to_unmap (page, +TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS )) { + rc = -EIO ; + goto out_unlock_both; + } page_was_mapped =1 ; }
@@ -1337,8 +1340,11 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, goto put_anon;if (page_mapped (hpage)) { -try_to_unmap (hpage, -TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS ); +if (!try_to_unmap (hpage, +TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS )) { + rc = -EIO ; + goto unlock_both; + } page_was_mapped =1 ; }
@@ -1349,6 +1355,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,remove_migration_ptes (hpage, rc ==MIGRATEPAGE_SUCCESS ? new_hpage : hpage, false); +unlock_both:unlock_page (new_hpage); put_anon:
@@ -2558,8 +2565,7 @@ static void migrate_vma_unmap(struct migrate_vma *migrate)continue ;if (page_mapped (page)) { -try_to_unmap (page, flags); -if (page_mapped (page)) +if (!try_to_unmap (page, flags)) goto restore; }
Signature · 1 line
2.25.0
Quoted 9 lines · Ying
From: Huang Ying <[email protected]>
During the page migration,
try_to_unmap()is called to replace the page table entries with the migration entries. Now its return value is ignored, that is generally OK in most cases. But in theory, it's possible thattry_to_unmap()return false (failure) for the page migration afterarch_unmap_one()is called in unmap code. Even if withoutarch_unmap_one(), it makes code more robust for the future code changing to check the return value.
No. This patch serves no purpose, and introduces a bug.
More robust? Robustness is provided by the later expected_count checks, with freezing where necessary.
Saving time by not going further if try_to_unmap() failed? That's done by the page_mapped() checks immediately following the try_to_unmap() calls (just out of sight in two cases).
Quoted 3 lines
Known issue: I don't know what is the appropriate error code for
try_to_unmap()failure. Whether EIO is OK?
-EAGAIN is the rc already being used for this case, and which indicates that retries may be appropriate (but they're often scary overkill).
Quoted 20 lines
Signed-off-by "Huang, Ying" <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Mel Gorman <[email protected]>Cc Vlastimil Babka <[email protected]>Cc Zi Yan <[email protected]>Cc Michal Hocko <[email protected]>Cc Minchan Kim <[email protected]>Cc Johannes Weiner <[email protected]>Cc Hugh Dickins <[email protected]>--- mm/migrate.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)mm/migrate.c +0
-0
mm/migrate.c static int __unmap_and_move(struct page *page, struct page *newpage, · line 1122
Quoted 9 lines
mm/migrate.c +4
-2@@ -1116,8 +1116,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage,/* Establish migration ptes */ VM_BUG_ON_PAGE (PageAnon (page) && !PageKsm (page) && !anon_vma, page); -try_to_unmap (page, -TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS ); +if (!try_to_unmap (page, +TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS )) { + rc = -EIO ; + goto out_unlock_both;
No: even if try_to_unmap() says that it did not entirely succeed, it may have unmapped some ptes, inserting migration entries in their place. Those need to be replaced by proper ptes before the page is unlocked, which page_was_mapped 1 and remove_migration_ptes() do; but this goto skips those.
Quoted 5 lines
mm/migrate.c +1
-0+ } page_was_mapped =1 ; }
mm/migrate.c static int unmap_and_move_huge_page(new_page_t get_new_page, · line 1346
Quoted 9 lines
mm/migrate.c +4
-2@@ -1337,8 +1340,11 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, goto put_anon;if (page_mapped (hpage)) { -try_to_unmap (hpage, -TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS ); +if (!try_to_unmap (hpage, +TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS )) { + rc = -EIO ; + goto unlock_both;
Same as above.
Quoted 24 lines
mm/migrate.c +3
-2+ } page_was_mapped =1 ; }@@ -1349,6 +1355,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,remove_migration_ptes (hpage, rc ==MIGRATEPAGE_SUCCESS ? new_hpage : hpage, false); +unlock_both:unlock_page (new_hpage); put_anon:@@ -2558,8 +2565,7 @@ static void migrate_vma_unmap(struct migrate_vma *migrate)continue ;if (page_mapped (page)) { -try_to_unmap (page, flags); -if (page_mapped (page)) +if (!try_to_unmap (page, flags)) goto restore; }Signature · 1 line
2.25.0
Hi, Hugh,
Quoted 12 lines · Hugh Dickins
Quoted 12 lines
mm/migrate.c +2
-2@@ -1116,8 +1116,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage,/* Establish migration ptes */ VM_BUG_ON_PAGE (PageAnon (page) && !PageKsm (page) && !anon_vma, page); -try_to_unmap (page, -TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS ); +if (!try_to_unmap (page, +TTU_MIGRATION |TTU_IGNORE_MLOCK |TTU_IGNORE_ACCESS )) {
Quoted 8 lines
mm/migrate.c static int __unmap_and_move(struct page *page, struct page *newpage, · line 1122
Quoted 2 lines
mm/migrate.c +2
-0+ rc = -EIO ; + goto out_unlock_both;No: even if
try_to_unmap()says that it did not entirely succeed, it may have unmapped some ptes, inserting migration entries in their place. Those need to be replaced by proper ptes before the page is unlocked, whichpage_was_mapped1 andremove_migration_ptes()do; but this goto skips those.
Yes. You are right. I misunderstand the original code. Please ignore this patch.
Best Regards,
Huang, Ying