mm: mempolicy: keep VMA walk if both MPOL_MF_STRICT and MPOL_MF_MOVE are specified

PATCH Applied Yang Shi 3 participants 7 messages Sep 20, 2023 – Sep 28, 2023

Subsystems (3)

Stable branch Memory management MM NUMA placement

lore

Share & tools

lore

Yang Shi

author

2023-09-20 22:34 UTC #0 lore raw

PATCH 1/1 mm: mempolicy: keep VMA walk if both MPOL_MF_STRICT and MPOL_MF_MOVE are specified

When calling mbind() with MPOL_MF_{MOVE|MOVEALL} | MPOL_MF_STRICT, kernel should attempt to migrate all existing pages, and return -EIO if there is misplaced or unmovable page. Then commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()") messed up the return value and didn't break VMA scan early ianymore when MPOL_MF_STRICT alone. The return value problem was fixed by commit a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified"), but it broke the VMA walk early if unmovable page is met, it may cause some pages are not migrated as expected.

The code should conceptually do:

if (MPOL_MF_MOVE|MOVEALL)
scan all vmas
try to migrate the existing pages
return success
else if (MPOL_MF_MOVE* | MPOL_MF_STRICT)
scan all vmas
try to migrate the existing pages
return -EIO if unmovable or migration failed
else /* MPOL_MF_STRICT alone */
break early if meets unmovable and don't call mbind_range() at all
else /* none of those flags */
check the ranges in test_walk, EFAULT without mbind_range() if discontig.

Fixed the behavior.

Cc Hugh Dickins <[email protected]>

Cc Suren Baghdasaryan <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vlastimil Babka <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Rafael Aquini <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Cc David Rientjes <[email protected]>

Cc <[email protected]> v4.9+

Signed-off-by Yang Shi <[email protected]>

---
 mm/mempolicy.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

mm/mempolicy.c +19 ~~-20~~

index 42b5567e3773..f1b00d6ac7ee 100644
@@ -426,6 +426,7 @@ struct queue_pages {
 	unsigned long start;
 	unsigned long end;
 	struct vm_area_struct *first;
+	bool has_unmovable;
 };
 
 /*

@@ -446,9 +447,8 @@ static inline bool queue_folio_required(struct folio *folio,
 /*
  * queue_folios_pmd() has three possible return values:
  * 0 - folios are placed on the right node or queued successfully, or
- *     special page is met, i.e. huge zero page.
- * 1 - there is unmovable folio, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
- *     specified.
+ *     special page is met, i.e. zero page, or unmovable page is found
+ *     but continue walking (indicated by queue_pages.has_unmovable).
  * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an
  *        existing folio was already on a node that does not follow the
  *        policy.

@@ -479,7 +479,7 @@ static int queue_folios_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
 	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
 		if (!vma_migratable(walk->vma) ||
 		    migrate_folio_add(folio, qp->pagelist, flags)) {
-			ret = 1;
+			qp->has_unmovable = true;
 			goto unlock;
 		}
 	} else

@@ -495,9 +495,8 @@ static int queue_folios_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
  *
  * queue_folios_pte_range() has three possible return values:
  * 0 - folios are placed on the right node or queued successfully, or
- *     special page is met, i.e. zero page.
- * 1 - there is unmovable folio, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
- *     specified.
+ *     special page is met, i.e. zero page, or unmovable page is found
+ *     but continue walking (indicated by queue_pages.has_unmovable).
  * -EIO - only MPOL_MF_STRICT was specified and an existing folio was already
  *        on a node that does not follow the policy.
  */

@@ -508,7 +507,6 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
 	struct folio *folio;
 	struct queue_pages *qp = walk->private;
 	unsigned long flags = qp->flags;
-	bool has_unmovable = false;
 	pte_t *pte, *mapped_pte;
 	pte_t ptent;
 	spinlock_t *ptl;

@@ -538,11 +536,12 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
 		if (!queue_folio_required(folio, qp))
 			continue;
 		if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
-			/* MPOL_MF_STRICT must be specified if we get here */
-			if (!vma_migratable(vma)) {
-				has_unmovable = true;
-				break;
-			}
+			/*
+			 * MPOL_MF_STRICT must be specified if we get here.
+			 * Continue walking vmas due to MPOL_MF_MOVE* flags.
+			 */
+			if (!vma_migratable(vma))
+				qp->has_unmovable = true;
 
 			/*
 			 * Do not abort immediately since there may be

@@ -550,16 +549,13 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
 			 * need migrate other LRU pages.
 			 */
 			if (migrate_folio_add(folio, qp->pagelist, flags))
-				has_unmovable = true;
+				qp->has_unmovable = true;
 		} else
 			break;
 	}
 	pte_unmap_unlock(mapped_pte, ptl);
 	cond_resched();
 
-	if (has_unmovable)
-		return 1;
-
 	return addr != end ? -EIO : 0;
 }

@@ -599,7 +595,7 @@ static int queue_folios_hugetlb(pte_t *pte, unsigned long hmask,
 		 * Detecting misplaced folio but allow migrating folios which
 		 * have been queued.
 		 */
-		ret = 1;
+		qp->has_unmovable = true;
 		goto unlock;
 	}

@@ -620,7 +616,7 @@ static int queue_folios_hugetlb(pte_t *pte, unsigned long hmask,
 			 * Failed to isolate folio but allow migrating pages
 			 * which have been queued.
 			 */
-			ret = 1;
+			qp->has_unmovable = true;
 	}
 unlock:
 	spin_unlock(ptl);

@@ -756,12 +752,15 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
 		.start = start,
 		.end = end,
 		.first = NULL,
+		.has_unmovable = false,
 	};
 	const struct mm_walk_ops *ops = lock_vma ?
 			&queue_pages_lock_vma_walk_ops : &queue_pages_walk_ops;
 
 	err = walk_page_range(mm, start, end, ops, &qp);
 
+	if (qp.has_unmovable)
+		err = 1;
 	if (!qp.first)
 		/* whole range in hole */
 		err = -EFAULT;

@@ -1358,7 +1357,7 @@ static long do_mbind(unsigned long start, unsigned long len,
 				putback_movable_pages(&pagelist);
 		}
 
-		if ((ret > 0) || (nr_failed && (flags & MPOL_MF_STRICT)))
+		if (((ret > 0) || nr_failed) && (flags & MPOL_MF_STRICT))
 			err = -EIO;
 	} else {
 up_out:

Signature · 1 line

2.39.0

Yang Shi

author

2023-09-20 23:36 UTC #1 lore raw

Quoted 25 lines · Yang Shi

When calling mbind() with MPOL_MF_{MOVE|MOVEALL} | MPOL_MF_STRICT, kernel should attempt to migrate all existing pages, and return -EIO if there is misplaced or unmovable page. Then commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()") messed up the return value and didn't break VMA scan early ianymore when MPOL_MF_STRICT alone. The return value problem was fixed by commit a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified"), but it broke the VMA walk early if unmovable page is met, it may cause some pages are not migrated as expected.
The code should conceptually do:
if (MPOL_MF_MOVE|MOVEALL)
scan all vmas
try to migrate the existing pages
return success
else if (MPOL_MF_MOVE* | MPOL_MF_STRICT)
scan all vmas
try to migrate the existing pages
return -EIO if unmovable or migration failed
else /* MPOL_MF_STRICT alone */
break early if meets unmovable and don't call mbind_range() at all
else /* none of those flags */
check the ranges in test_walk, EFAULT without mbind_range() if discontig.

Quoted 1 line

Fixed the behavior.

Forgot the fixes.

Fixes a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified")

Quoted 148 lines

Cc Hugh Dickins <[email protected]>

Cc Suren Baghdasaryan <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Vlastimil Babka <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Rafael Aquini <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Cc David Rientjes <[email protected]>

Cc <[email protected]> v4.9+

Signed-off-by Yang Shi <[email protected]>

---
  mm/mempolicy.c | 39 +++++++++++++++++++--------------------
  1 file changed, 19 insertions(+), 20 deletions(-)

mm/mempolicy.c +19 ~~-20~~

index 42b5567e3773..f1b00d6ac7ee 100644
@@ -426,6 +426,7 @@ struct queue_pages {
  	unsigned long start;
  	unsigned long end;
  	struct vm_area_struct *first;
+	bool has_unmovable;
  };
 
  /*

@@ -446,9 +447,8 @@ static inline bool queue_folio_required(struct folio *folio,
  /*
   * queue_folios_pmd() has three possible return values:
   * 0 - folios are placed on the right node or queued successfully, or
- *     special page is met, i.e. huge zero page.
- * 1 - there is unmovable folio, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
- *     specified.
+ *     special page is met, i.e. zero page, or unmovable page is found
+ *     but continue walking (indicated by queue_pages.has_unmovable).
   * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an
   *        existing folio was already on a node that does not follow the
   *        policy.

@@ -479,7 +479,7 @@ static int queue_folios_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
  	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
  		if (!vma_migratable(walk->vma) ||
  		    migrate_folio_add(folio, qp->pagelist, flags)) {
-			ret = 1;
+			qp->has_unmovable = true;
  			goto unlock;
  		}
  	} else

@@ -495,9 +495,8 @@ static int queue_folios_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
   *
   * queue_folios_pte_range() has three possible return values:
   * 0 - folios are placed on the right node or queued successfully, or
- *     special page is met, i.e. zero page.
- * 1 - there is unmovable folio, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
- *     specified.
+ *     special page is met, i.e. zero page, or unmovable page is found
+ *     but continue walking (indicated by queue_pages.has_unmovable).
   * -EIO - only MPOL_MF_STRICT was specified and an existing folio was already
   *        on a node that does not follow the policy.
   */

@@ -508,7 +507,6 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
  	struct folio *folio;
  	struct queue_pages *qp = walk->private;
  	unsigned long flags = qp->flags;
-	bool has_unmovable = false;
  	pte_t *pte, *mapped_pte;
  	pte_t ptent;
  	spinlock_t *ptl;

@@ -538,11 +536,12 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
  		if (!queue_folio_required(folio, qp))
  			continue;
  		if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
-			/* MPOL_MF_STRICT must be specified if we get here */
-			if (!vma_migratable(vma)) {
-				has_unmovable = true;
-				break;
-			}
+			/*
+			 * MPOL_MF_STRICT must be specified if we get here.
+			 * Continue walking vmas due to MPOL_MF_MOVE* flags.
+			 */
+			if (!vma_migratable(vma))
+				qp->has_unmovable = true;
 
  			/*
  			 * Do not abort immediately since there may be

@@ -550,16 +549,13 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
  			 * need migrate other LRU pages.
  			 */
  			if (migrate_folio_add(folio, qp->pagelist, flags))
-				has_unmovable = true;
+				qp->has_unmovable = true;
  		} else
  			break;
  	}
  	pte_unmap_unlock(mapped_pte, ptl);
  	cond_resched();
 
-	if (has_unmovable)
-		return 1;
-
  	return addr != end ? -EIO : 0;
  }

@@ -599,7 +595,7 @@ static int queue_folios_hugetlb(pte_t *pte, unsigned long hmask,
  		 * Detecting misplaced folio but allow migrating folios which
  		 * have been queued.
  		 */
-		ret = 1;
+		qp->has_unmovable = true;
  		goto unlock;
  	}

@@ -620,7 +616,7 @@ static int queue_folios_hugetlb(pte_t *pte, unsigned long hmask,
  			 * Failed to isolate folio but allow migrating pages
  			 * which have been queued.
  			 */
-			ret = 1;
+			qp->has_unmovable = true;
  	}
  unlock:
  	spin_unlock(ptl);

@@ -756,12 +752,15 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
  		.start = start,
  		.end = end,
  		.first = NULL,
+		.has_unmovable = false,
  	};
  	const struct mm_walk_ops *ops = lock_vma ?
  			&queue_pages_lock_vma_walk_ops : &queue_pages_walk_ops;
 
  	err = walk_page_range(mm, start, end, ops, &qp);
 
+	if (qp.has_unmovable)
+		err = 1;
  	if (!qp.first)
  		/* whole range in hole */
  		err = -EFAULT;

@@ -1358,7 +1357,7 @@ static long do_mbind(unsigned long start, unsigned long len,
  				putback_movable_pages(&pagelist);
  		}
 
-		if ((ret > 0) || (nr_failed && (flags & MPOL_MF_STRICT)))
+		if (((ret > 0) || nr_failed) && (flags & MPOL_MF_STRICT))
  			err = -EIO;
  	} else {
  up_out:

Andrew Morton

maintainer

2023-09-25 15:48 UTC #2 lore raw

In reply to Yang Shi

Quoted 9 lines · On Wed, 20 Sep 2023 15:32:42 -0700 Yang Shi <[email protected]> wrote:

When calling mbind() with MPOL_MF_{MOVE|MOVEALL} | MPOL_MF_STRICT, kernel should attempt to migrate all existing pages, and return -EIO if there is misplaced or unmovable page. Then commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()") messed up the return value and didn't break VMA scan early ianymore when MPOL_MF_STRICT alone. The return value problem was fixed by commit a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified"), but it broke the VMA walk early if unmovable page is met, it may cause some pages are not migrated as expected.

So I'm thinking that a7f40cfe3b7a is the suitable Fixes: target?

Quoted 17 lines

The code should conceptually do:
if (MPOL_MF_MOVE|MOVEALL)
scan all vmas
try to migrate the existing pages
return success
else if (MPOL_MF_MOVE* | MPOL_MF_STRICT)
scan all vmas
try to migrate the existing pages
return -EIO if unmovable or migration failed
else /* MPOL_MF_STRICT alone */
break early if meets unmovable and don't call mbind_range() at all
else /* none of those flags */
check the ranges in test_walk, EFAULT without mbind_range() if discontig.
Fixed the behavior.

Yang Shi

author

2023-09-25 17:16 UTC #3 lore raw

Quoted 11 lines · Andrew Morton

Quoted 9 lines · On Wed, 20 Sep 2023 15:32:42 -0700 Yang Shi <[email protected]> wrote:
When calling mbind() with MPOL_MF_{MOVE|MOVEALL} | MPOL_MF_STRICT, kernel should attempt to migrate all existing pages, and return -EIO if there is misplaced or unmovable page. Then commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()") messed up the return value and didn't break VMA scan early ianymore when MPOL_MF_STRICT alone. The return value problem was fixed by commit a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified"), but it broke the VMA walk early if unmovable page is met, it may cause some pages are not migrated as expected.

Quoted 1 line

So I'm thinking that a7f40cfe3b7a is the suitable Fixes: target?

Yes, thanks. My follow-up email also added this.

Quoted 18 lines

Quoted 18 lines
The code should conceptually do:
if (MPOL_MF_MOVE|MOVEALL)
scan all vmas
try to migrate the existing pages
return success
else if (MPOL_MF_MOVE* | MPOL_MF_STRICT)
scan all vmas
try to migrate the existing pages
return -EIO if unmovable or migration failed
else /* MPOL_MF_STRICT alone */
break early if meets unmovable and don't call mbind_range() at all
else /* none of those flags */
check the ranges in test_walk, EFAULT without mbind_range() if discontig.
Fixed the behavior.

Suren Baghdasaryan

2023-09-27 21:39 UTC #4 lore raw

Quoted 15 lines · Yang Shi

Quoted 11 lines · Andrew Morton
Quoted 9 lines · On Wed, 20 Sep 2023 15:32:42 -0700 Yang Shi <[email protected]> wrote:
When calling mbind() with MPOL_MF_{MOVE|MOVEALL} | MPOL_MF_STRICT, kernel should attempt to migrate all existing pages, and return -EIO if there is misplaced or unmovable page. Then commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()") messed up the return value and didn't break VMA scan early ianymore when MPOL_MF_STRICT alone. The return value problem was fixed by commit a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified"), but it broke the VMA walk early if unmovable page is met, it may cause some pages are not migrated as expected.

Quoted 19 lines

Quoted 1 line
So I'm thinking that a7f40cfe3b7a is the suitable Fixes: target?
Yes, thanks. My follow-up email also added this.
Quoted 15 lines
Quoted 15 lines
The code should conceptually do:
if (MPOL_MF_MOVE|MOVEALL)
scan all vmas
try to migrate the existing pages
return success
else if (MPOL_MF_MOVE* | MPOL_MF_STRICT)
scan all vmas
try to migrate the existing pages
return -EIO if unmovable or migration failed
else /* MPOL_MF_STRICT alone */
break early if meets unmovable and don't call mbind_range() at all
else /* none of those flags */
check the ranges in test_walk, EFAULT without mbind_range() if discontig.

With this change I think my temporary fix at
https://lore.kernel.org/all/[email protected]/
can be removed because we either scan all vmas (which means we locked them all) or we break early and do not call mbind_range() at all (in which case we don't need vmas to be locked).

Quoted 4 lines

Quoted 3 lines
Quoted 3 lines
Fixed the behavior.

Andrew Morton

maintainer

2023-09-28 16:38 UTC #5 lore raw

Quoted 16 lines · On Wed, 27 Sep 2023 14:39:21 -0700 Suren Baghdasaryan <[email protected]> wrote:

Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
The code should conceptually do:
if (MPOL_MF_MOVE|MOVEALL)
scan all vmas
try to migrate the existing pages
return success
else if (MPOL_MF_MOVE* | MPOL_MF_STRICT)
scan all vmas
try to migrate the existing pages
return -EIO if unmovable or migration failed
else /* MPOL_MF_STRICT alone */
break early if meets unmovable and don't call mbind_range() at all
else /* none of those flags */
check the ranges in test_walk, EFAULT without mbind_range() if discontig.

Quoted 5 lines

With this change I think my temporary fix at
https://lore.kernel.org/all/[email protected]/
can be removed because we either scan all vmas (which means we locked them all) or we break early and do not call mbind_range() at all (in which case we don't need vmas to be locked).

Thanks, I dropped "mm: lock VMAs skipped by a failed queue_pages_range()"

Yang Shi

author

2023-09-28 17:35 UTC #6 lore raw

Quoted 16 lines · Andrew Morton

Quoted 14 lines · On Wed, 27 Sep 2023 14:39:21 -0700 Suren Baghdasaryan <[email protected]> wrote:
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
The code should conceptually do:
if (MPOL_MF_MOVE|MOVEALL)
scan all vmas
try to migrate the existing pages
return success
else if (MPOL_MF_MOVE* | MPOL_MF_STRICT)
scan all vmas
try to migrate the existing pages
return -EIO if unmovable or migration failed
else /* MPOL_MF_STRICT alone */
break early if meets unmovable and don't call mbind_range() at all
else /* none of those flags */
check the ranges in test_walk, EFAULT without mbind_range() if discontig.

Quoted 5 lines

Quoted 5 lines
With this change I think my temporary fix at
https://lore.kernel.org/all/[email protected]/
can be removed because we either scan all vmas (which means we locked them all) or we break early and do not call mbind_range() at all (in which case we don't need vmas to be locked).

Yes, we could just drop it. Keep the code not depend on the subtle behavior of queue_pages_range() by keeping it is ok to me either. I don't have strong preference.

Quoted 1 line

Thanks, I dropped "mm: lock VMAs skipped by a failed queue_pages_range()"

Thanks, Andrew.