mm: migrate: Check page_count of THP before migrating

Hugh Dickins pointed out that migrate_misplaced_transhuge_page() does not check page_count before migrating like base page migration and khugepage. He could not see why this was safe and he is right.

The potential impact of the bug is avoided due to the limitations of NUMA balancing. The page_mapcount() check ensures that only a single address space is using this page and as THPs are typically private it should not be possible for another address space to fault it in parallel. If the address space has one associated task then it's difficult to have both a GUP pin and be referencing the page at the same time. If there are multiple tasks then a buggy scenario requires that another thread be accessing the page while the direct IO is in flight. This is dodgy behaviour as there is a possibility of corruption with or without THP migration. It would be difficult to identify the corruption as being a migration bug.

While we happen to be safe for the most part it is shoddy to depend on such "safety" so this patch checks the page count similar to anonymous pages. Note that this does not mean that the page_mapcount() check can go away. If we were to remove the page_mapcount() check the the THP would have to be unmapped from all referencing PTEs, replaced with migration PTEs and restored properly afterwards.

Reported-by Hugh Dickins <[email protected]>

Signed-off-by Mel Gorman <[email protected]>

---
 mm/migrate.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

mm/migrate.c +4 -1

index 3b676b0..f466827 100644
@@ -1679,9 +1679,18 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	page_xchg_last_nid(new_page, page_last_nid(page));
 
 	isolated = numamigrate_isolate_page(pgdat, page);
-	if (!isolated) {
+
+	/*
+	 * Failing to isolate or a GUP pin prevents migration. The expected
+	 * page count is 2. 1 for anonymous pages without a mapping and 1

As pointed out by Hugh Dickins, "mm: migrate: Check page_count of THP before migrating" can leave nr_isolated_anon elevated, correct it. This is a fix to mm-migrate-check-page_count-of-thp-before-migrating.patch

Signed-off-by Mel Gorman <[email protected]>

---
 mm/migrate.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

mm/migrate.c +0 -0

index f466827..c387786 100644
@@ -1689,8 +1689,11 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	if (!isolated || page_count(page) != 2) {

Quoted 22 lines · Mel Gorman

Hugh Dickins pointed out that migrate_misplaced_transhuge_page() does not check page_count before migrating like base page migration and khugepage. He could not see why this was safe and he is right.

The potential impact of the bug is avoided due to the limitations of NUMA balancing. The page_mapcount() check ensures that only a single address space is using this page and as THPs are typically private it should not be possible for another address space to fault it in parallel. If the address space has one associated task then it's difficult to have both a GUP pin and be referencing the page at the same time. If there are multiple tasks then a buggy scenario requires that another thread be accessing the page while the direct IO is in flight. This is dodgy behaviour as there is a possibility of corruption with or without THP migration. It would be difficult to identify the corruption as being a migration bug.

While we happen to be safe for the most part it is shoddy to depend on such "safety" so this patch checks the page count similar to anonymous pages. Note that this does not mean that the page_mapcount() check can go away. If we were to remove the page_mapcount() check the the THP would have to be unmapped from all referencing PTEs, replaced with migration PTEs and restored properly afterwards.

Quoted 2 lines

Reported-by Hugh Dickins <[email protected]>

Signed-off-by Mel Gorman <[email protected]>

Sorry, Mel, it's a NAK:

you will have expected an ack from me two weeks
or more ago; but somehow I had an intuition that if I sat on it for long enough, a worm would crawl out. Got down to looking again today, and I notice that although the putback_lru_page() is right, NR_ISOLATED_ANON is not restored on this path, so that would leak.

I expect you'll want to do something like:

	if (isolated) {
		putback_lru_page(page);
		isolated = 0;
		goto out;
	}

and that may be the appropriate fix right now.

But I do still dislike the way you always put_page in numamigrate_isolate_page():

it makes sense in the case when
isolate_lru_page() succeeds (I've long thought that weird both to insist on an existing page reference and add one of its own), but I find it very confusing on the failure paths, to have the put_page far away from the unlock_page - and I get worried when I see put_page followed by unlock_page rather than vice versa (it happens on !pmd_same
paths: if the pmd is not the same, then can we be sure that the put_page does not free the page?)

At the bottom I've put my own cleanup for this, which simplifies by doing the putback_lru_page() inside numamigrate_isolate_page(), and doesn't put_page when it doesn't isolate.

I think the only functional difference from yours (aside from fixing up NR_ISOLATED) is that migrate_misplaced_transhuge_page() doesn't have to pretend to its caller that it succeeded when actually it failed at the last hurdle (because it already did the unlock_page, which in yours the caller expects to do on failure). Oh, and I'm not holding page lock (sometimes) at clear_pmdnuma: I didn't see the reason for that, perhaps I'm missing something important there.

Maybe our tastes differ, and you won't see mine as an improvement. And I've hardly tested, so haven't signed off, and won't be surprised if its own worms crawl out.

<SNIP>

Not-signed-off-by Hugh Dickins <[email protected]>

---

 mm/huge_memory.c |   28 +++++----------
 mm/migrate.c     |   79 ++++++++++++++++++++++-----------------------
 2 files changed, 48 insertions(+), 59 deletions(-)

mm/huge_memory.c +0 -12

@@ -1298,7 +1298,6 @@ int do_huge_pmd_numa_page(struct mm_stru
 	int target_nid;
 	int current_nid = -1;
 	bool migrated;
-	bool page_locked = false;
 
 	spin_lock(&mm->page_table_lock);
 	if (unlikely(!pmd_same(pmd, *pmdp)))

@@ -1320,7 +1319,6 @@ int do_huge_pmd_numa_page(struct mm_stru
 	/* Acquire the page lock to serialise THP migrations */
 	spin_unlock(&mm->page_table_lock);
 	lock_page(page);
-	page_locked = true;
 
 	/* Confirm the PTE did not while locked */
 	spin_lock(&mm->page_table_lock);

@@ -1333,34 +1331,26 @@ int do_huge_pmd_numa_page(struct mm_stru
 
 	/* Migrate the THP to the requested node */
 	migrated = migrate_misplaced_transhuge_page(mm, vma,
-				pmdp, pmd, addr,
-				page, target_nid);
-	if (migrated)
-		current_nid = target_nid;
-	else {
-		spin_lock(&mm->page_table_lock);
-		if (unlikely(!pmd_same(pmd, *pmdp))) {
-			unlock_page(page);
-			goto out_unlock;
-		}

mm/huge_memory.c +1 -1

 out_unlock:
 	spin_unlock(&mm->page_table_lock);
 	if (current_nid != -1)
-		task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
+		task_numa_fault(current_nid, HPAGE_PMD_NR, false);
 	return 0;
 }
 

mm/migrate.c +8 -17

@@ -1555,39 +1555,38 @@ bool numamigrate_update_ratelimit(pg_dat
 
 int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
-	int ret = 0;
+	int page_lru;
 
 	/* Avoid migrating to a node that is nearly full */
-	if (migrate_balanced_pgdat(pgdat, 1)) {
-		int page_lru;
+	if (!migrate_balanced_pgdat(pgdat, 1))
+		return 0;
 
-		if (isolate_lru_page(page)) {
-			put_page(page);
-			return 0;
-		}
-
-		/* Page is isolated */
-		ret = 1;
-		page_lru = page_is_file_cache(page);
-		if (!PageTransHuge(page))
-			inc_zone_page_state(page, NR_ISOLATED_ANON + page_lru);
-		else
-			mod_zone_page_state(page_zone(page),
-					NR_ISOLATED_ANON + page_lru,
-					HPAGE_PMD_NR);
+	if (isolate_lru_page(page))
+		return 0;
+
+	/*
+	 * migrate_misplaced_transhuge_page() skips page migration's usual

mm/migrate.c +11 -15

+	page_lru = page_is_file_cache(page);
+	mod_zone_page_state(page_zone(page), NR_ISOLATED_ANON + page_lru,
+				hpage_nr_pages(page));
+
 	/*
-	 * Page is either isolated or there is not enough space on the target
-	 * node. If isolated, then it has taken a reference count and the
-	 * callers reference can be safely dropped without the page
-	 * disappearing underneath us during migration. Otherwise the page is
-	 * not to be migrated but the callers reference should still be
-	 * dropped so it does not leak.
+	 * Isolating the page has taken another reference, so the
+	 * caller's reference can be safely dropped without the page
+	 * disappearing underneath us during migration.
 	 */
 	put_page(page);
-
-	return ret;
+	return 1;
 }
 
 /*

@@ -1598,7 +1597,7 @@ int numamigrate_isolate_page(pg_data_t *
 int migrate_misplaced_page(struct page *page, int node)
 {
 	pg_data_t *pgdat = NODE_DATA(node);
-	int isolated = 0;
+	int isolated;
 	int nr_remaining;
 	LIST_HEAD(migratepages);
 

@@ -1606,20 +1605,16 @@ int migrate_misplaced_page(struct page *
 	 * Don't migrate pages that are mapped in multiple processes.
 	 * TODO: Handle false sharing detection instead of this hammer
 	 */
-	if (page_mapcount(page) != 1) {
-		put_page(page);
+	if (page_mapcount(page) != 1)
 		goto out;
-	}
 
 	/*
 	 * Rate-limit the amount of data that is being migrated to a node.
 	 * Optimal placement is no good if the memory bus is saturated and
 	 * all the time is being spent migrating!
 	 */
-	if (numamigrate_update_ratelimit(pgdat, 1)) {
-		put_page(page);
+	if (numamigrate_update_ratelimit(pgdat, 1))
 		goto out;
-	}
 
 	isolated = numamigrate_isolate_page(pgdat, page);
 	if (!isolated)

mm/migrate.c +5 -6

 #endif /* CONFIG_NUMA_BALANCING */
 

@@ -1672,17 +1670,15 @@ int migrate_misplaced_transhuge_page(str
 
 	new_page = alloc_pages_node(node,
 		(GFP_TRANSHUGE | GFP_THISNODE) & ~__GFP_WAIT, HPAGE_PMD_ORDER);
-	if (!new_page) {
-		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
-		goto out_dropref;
-	}
+	if (!new_page)
+		goto out_fail;
+
 	page_xchg_last_nid(new_page, page_last_nid(page));
 
 	isolated = numamigrate_isolate_page(pgdat, page);
 	if (!isolated) {
-		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
 		put_page(new_page);
-		goto out_keep_locked;
+		goto out_fail;
 	}
 
 	/* Prepare a page as a migration target */

@@ -1714,6 +1710,7 @@ int migrate_misplaced_transhuge_page(str
 		putback_lru_page(page);
 
 		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
+		isolated = 0;
 		goto out;
 	}
 

@@ -1758,9 +1755,11 @@ out:
 			-HPAGE_PMD_NR);
 	return isolated;