[v2 PATCH 0/2] mm: mempolicy: fix mbind()'s inconsistent behavior for unmovable pages

[v2 PATCH 0/2] mm: mempolicy: fix mbind()'s inconsistent behavior for unmovable pages

[Yang Shi]

Changelog
v2: * Fixed the inconsistent behavior by not aborting !vma_migratable()
      immediately by a separate patch (patch 1/2), and this is also the
      preparation for patch 2/2. For the details please see the commit
      log.  Per Vlastimil.
    * Not abort immediately if unmovable page is met. This should handle
      non-LRU movable pages and temporary off-LRU pages more friendly.
      Per Vlastimil and Michal Hocko.

Yang Shi (2):
      mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified
      mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

 mm/mempolicy.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 88 insertions(+), 32 deletions(-)

[v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

[Yang Shi]

When both MPOL_MF_MOVE* and MPOL_MF_STRICT was specified, mbind() should
try best to migrate misplaced pages, if some of the pages could not be
migrated, then return -EIO.

There are three different sub-cases:
1. vma is not migratable
2. vma is migratable, but there are unmovable pages
3. vma is migratable, pages are movable, but migrate_pages() fails

If #1 happens, kernel would just abort immediately, then return -EIO,
after the commit a7f40cfe3b7ada57af9b62fd28430eeb4a7cfcb7 ("mm:
mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified").

If #3 happens, kernel would set policy and migrate pages with best-effort,
but won't rollback the migrated pages and reset the policy back.

Before that commit, they behaves in the same way.  It'd better to keep
their behavior consistent.  But, rolling back the migrated pages and
resetting the policy back sounds not feasible, so just make #1 behave as
same as #3.

Userspace will know that not everything was successfully migrated (via
-EIO), and can take whatever steps it deems necessary - attempt rollback,
determine which exact page(s) are violating the policy, etc.

Make queue_pages_range() return 1 to indicate there are unmovable pages
or vma is not migratable.

The #2 is not handled correctly in the current kernel, the following
patch will fix it.

Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 mm/mempolicy.c | 86 +++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 25 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 01600d8..b50039c 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -429,11 +429,14 @@ static inline bool queue_pages_required(struct page *page,
 }
 
 /*
- * queue_pages_pmd() has three possible return values:
+ * queue_pages_pmd() has four possible return values:
+ * 2 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
+ *     specified.
  * 1 - pages are placed on the right node or queued successfully.
  * 0 - THP was split.
- * -EIO - is migration entry or MPOL_MF_STRICT was specified and an existing
- *        page was already on a node that does not follow the policy.
+ * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an
+ *        existing page was already on a node that does not follow the
+ *        policy.
  */
 static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
 				unsigned long end, struct mm_walk *walk)
@@ -463,7 +466,7 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
 	/* go to thp migration */
 	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
 		if (!vma_migratable(walk->vma)) {
-			ret = -EIO;
+			ret = 2;
 			goto unlock;
 		}
 
@@ -488,16 +491,29 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
 	struct queue_pages *qp = walk->private;
 	unsigned long flags = qp->flags;
 	int ret;
+	bool has_unmovable = false;
 	pte_t *pte;
 	spinlock_t *ptl;
 
 	ptl = pmd_trans_huge_lock(pmd, vma);
 	if (ptl) {
 		ret = queue_pages_pmd(pmd, ptl, addr, end, walk);
-		if (ret > 0)
+		switch (ret) {
+		/* THP was split, fall through to pte walk */
+		case 0:
+			break;
+		/* Pages are placed on the right node or queued successfully */
+		case 1:
 			return 0;
-		else if (ret < 0)
+		/*
+		 * Met unmovable pages, MPOL_MF_MOVE* & MPOL_MF_STRICT
+		 * were specified.
+		 */
+		case 2:
+			return 1;
+		case -EIO:
 			return ret;
+		}
 	}
 
 	if (pmd_trans_unstable(pmd))
@@ -519,14 +535,21 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
 		if (!queue_pages_required(page, qp))
 			continue;
 		if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
-			if (!vma_migratable(vma))
+			/* MPOL_MF_STRICT must be specified if we get here */
+			if (!vma_migratable(vma)) {
+				has_unmovable |= true;
 				break;
+			}
 			migrate_page_add(page, qp->pagelist, flags);
 		} else
 			break;
 	}
 	pte_unmap_unlock(pte - 1, ptl);
 	cond_resched();
+
+	if (has_unmovable)
+		return 1;
+
 	return addr != end ? -EIO : 0;
 }
 
@@ -639,7 +662,13 @@ static int queue_pages_test_walk(unsigned long start, unsigned long end,
  *
  * If pages found in a given range are on a set of nodes (determined by
  * @nodes and @flags,) it's isolated and queued to the pagelist which is
- * passed via @private.)
+ * passed via @private.
+ *
+ * queue_pages_range() has three possible return values:
+ * 1 - there is unmovable page, but MPOL_MF_MOVE* & MPOL_MF_STRICT were
+ *     specified.
+ * 0 - queue pages successfully or no misplaced page.
+ * -EIO - there is misplaced page and only MPOL_MF_STRICT was specified.
  */
 static int
 queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
@@ -1182,6 +1211,7 @@ static long do_mbind(unsigned long start, unsigned long len,
 	struct mempolicy *new;
 	unsigned long end;
 	int err;
+	int ret;
 	LIST_HEAD(pagelist);
 
 	if (flags & ~(unsigned long)MPOL_MF_VALID)
@@ -1243,26 +1273,32 @@ static long do_mbind(unsigned long start, unsigned long len,
 	if (err)
 		goto mpol_out;
 
-	err = queue_pages_range(mm, start, end, nmask,
+	ret = queue_pages_range(mm, start, end, nmask,
 			  flags | MPOL_MF_INVERT, &pagelist);
-	if (!err)
-		err = mbind_range(mm, start, end, new);
-
-	if (!err) {
-		int nr_failed = 0;
 
-		if (!list_empty(&pagelist)) {
-			WARN_ON_ONCE(flags & MPOL_MF_LAZY);
-			nr_failed = migrate_pages(&pagelist, new_page, NULL,
-				start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
-			if (nr_failed)
-				putback_movable_pages(&pagelist);
-		}
+	if (ret < 0)
+		err = -EIO;
+	else {
+		err = mbind_range(mm, start, end, new);
 
-		if (nr_failed && (flags & MPOL_MF_STRICT))
-			err = -EIO;
-	} else
-		putback_movable_pages(&pagelist);
+		if (!err) {
+			int nr_failed = 0;
+
+			if (!list_empty(&pagelist)) {
+				WARN_ON_ONCE(flags & MPOL_MF_LAZY);
+				nr_failed = migrate_pages(&pagelist, new_page,
+					NULL, start, MIGRATE_SYNC,
+					MR_MEMPOLICY_MBIND);
+				if (nr_failed)
+					putback_movable_pages(&pagelist);
+			}
+
+			if ((ret > 0) ||
+			    (nr_failed && (flags & MPOL_MF_STRICT)))
+				err = -EIO;
+		} else
+			putback_movable_pages(&pagelist);
+	}
 
 	up_write(&mm->mmap_sem);
  mpol_out:
-- 
1.8.3.1

[v2 PATCH 2/2] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

[Yang Shi]

When running syzkaller internally, we ran into the below bug on 4.9.x
kernel:

kernel BUG at mm/huge_memory.c:2124!
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
CPU: 0 PID: 1518 Comm: syz-executor107 Not tainted 4.9.168+ #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.5.1 01/01/2011
task: ffff880067b34900 task.stack: ffff880068998000
RIP: 0010:[<ffffffff81895d6b>]  [<ffffffff81895d6b>] split_huge_page_to_list+0x8fb/0x1030 mm/huge_memory.c:2124
RSP: 0018:ffff88006899f980  EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffffea00018f1700 RCX: 0000000000000000
RDX: 1ffffd400031e2e7 RSI: 0000000000000001 RDI: ffffea00018f1738
RBP: ffff88006899f9e8 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: fffffbfff0d8b13e R12: ffffea00018f1400
R13: ffffea00018f1400 R14: ffffea00018f1720 R15: ffffea00018f1401
FS:  00007fa333996740(0000) GS:ffff88006c600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000040 CR3: 0000000066b9c000 CR4: 00000000000606f0
Stack:
 0000000000000246 ffff880067b34900 0000000000000000 ffff88007ffdc000
 0000000000000000 ffff88006899f9e8 ffffffff812b4015 ffff880064c64e18
 ffffea00018f1401 dffffc0000000000 ffffea00018f1700 0000000020ffd000
Call Trace:
 [<ffffffff818490f1>] split_huge_page include/linux/huge_mm.h:100 [inline]
 [<ffffffff818490f1>] queue_pages_pte_range+0x7e1/0x1480 mm/mempolicy.c:538
 [<ffffffff817ed0da>] walk_pmd_range mm/pagewalk.c:50 [inline]
 [<ffffffff817ed0da>] walk_pud_range mm/pagewalk.c:90 [inline]
 [<ffffffff817ed0da>] walk_pgd_range mm/pagewalk.c:116 [inline]
 [<ffffffff817ed0da>] __walk_page_range+0x44a/0xdb0 mm/pagewalk.c:208
 [<ffffffff817edb94>] walk_page_range+0x154/0x370 mm/pagewalk.c:285
 [<ffffffff81844515>] queue_pages_range+0x115/0x150 mm/mempolicy.c:694
 [<ffffffff8184f493>] do_mbind mm/mempolicy.c:1241 [inline]
 [<ffffffff8184f493>] SYSC_mbind+0x3c3/0x1030 mm/mempolicy.c:1370
 [<ffffffff81850146>] SyS_mbind+0x46/0x60 mm/mempolicy.c:1352
 [<ffffffff810097e2>] do_syscall_64+0x1d2/0x600 arch/x86/entry/common.c:282
 [<ffffffff82ff6f93>] entry_SYSCALL_64_after_swapgs+0x5d/0xdb
Code: c7 80 1c 02 00 e8 26 0a 76 01 <0f> 0b 48 c7 c7 40 46 45 84 e8 4c
RIP  [<ffffffff81895d6b>] split_huge_page_to_list+0x8fb/0x1030 mm/huge_memory.c:2124
 RSP <ffff88006899f980>

with the below test:

---8<---

uint64_t r[1] = {0xffffffffffffffff};

int main(void)
{
	syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0);
				intptr_t res = 0;
	res = syscall(__NR_socket, 0x11, 3, 0x300);
	if (res != -1)
		r[0] = res;
*(uint32_t*)0x20000040 = 0x10000;
*(uint32_t*)0x20000044 = 1;
*(uint32_t*)0x20000048 = 0xc520;
*(uint32_t*)0x2000004c = 1;
	syscall(__NR_setsockopt, r[0], 0x107, 0xd, 0x20000040, 0x10);
	syscall(__NR_mmap, 0x20fed000, 0x10000, 0, 0x8811, r[0], 0);
*(uint64_t*)0x20000340 = 2;
	syscall(__NR_mbind, 0x20ff9000, 0x4000, 0x4002, 0x20000340,
0x45d4, 3);
	return 0;
}

---8<---

Actually the test does:

mmap(0x20000000, 16777216, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000
socket(AF_PACKET, SOCK_RAW, 768)        = 3
setsockopt(3, SOL_PACKET, PACKET_TX_RING, {block_size=65536, block_nr=1, frame_size=50464, frame_nr=1}, 16) = 0
mmap(0x20fed000, 65536, PROT_NONE, MAP_SHARED|MAP_FIXED|MAP_POPULATE|MAP_DENYWRITE, 3, 0) = 0x20fed000
mbind(..., MPOL_MF_STRICT|MPOL_MF_MOVE) = 0

The setsockopt() would allocate compound pages (16 pages in this test)
for packet tx ring, then the mmap() would call packet_mmap() to map the
pages into the user address space specified by the mmap() call.

When calling mbind(), it would scan the vma to queue the pages for
migration to the new node.  It would split any huge page since 4.9
doesn't support THP migration, however, the packet tx ring compound
pages are not THP and even not movable.  So, the above bug is triggered.

However, the later kernel is not hit by this issue due to the
commit d44d363f65780f2ac2 ("mm: don't assume anonymous pages have
SwapBacked flag"), which just removes the PageSwapBacked check for a
different reason.

But, there is a deeper issue.  According to the semantic of mbind(), it
should return -EIO if MPOL_MF_MOVE or MPOL_MF_MOVE_ALL was specified and
MPOL_MF_STRICT was also specified, but the kernel was unable to move
all existing pages in the range.  The tx ring of the packet socket is
definitely not movable, however, mbind() returns success for this case.

Although the most socket file associates with non-movable pages, but XDP
may have movable pages from gup.  So, it sounds not fine to just check
the underlying file type of vma in vma_migratable().

Change migrate_page_add() to check if the page is movable or not, if it
is unmovable, just return -EIO.  But do not abort pte walk immediately,
since there may be pages off LRU temporarily.  We should migrate other
pages if MPOL_MF_MOVE* is specified.  Set has_unmovable flag if some
paged could not be not moved, then return -EIO for mbind() eventually.

With this change the above test would return -EIO as expected.

Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 mm/mempolicy.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index b50039c..45d3d6e 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -403,7 +403,7 @@ void mpol_rebind_mm(struct mm_struct *mm, nodemask_t *new)
 	},
 };
 
-static void migrate_page_add(struct page *page, struct list_head *pagelist,
+static int migrate_page_add(struct page *page, struct list_head *pagelist,
 				unsigned long flags);
 
 struct queue_pages {
@@ -465,12 +465,11 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
 	flags = qp->flags;
 	/* go to thp migration */
 	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
-		if (!vma_migratable(walk->vma)) {
+		if (!vma_migratable(walk->vma) ||
+		    migrate_page_add(page, qp->pagelist, flags)) {
 			ret = 2;
 			goto unlock;
 		}
-
-		migrate_page_add(page, qp->pagelist, flags);
 	} else
 		ret = -EIO;
 unlock:
@@ -540,7 +539,14 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
 				has_unmovable |= true;
 				break;
 			}
-			migrate_page_add(page, qp->pagelist, flags);
+
+			/*
+			 * Do not abort immediately since there may be
+			 * temporary off LRU pages in the range.  Still
+			 * need migrate other LRU pages.
+			 */
+			if (migrate_page_add(page, qp->pagelist, flags))
+				has_unmovable |= true;
 		} else
 			break;
 	}
@@ -969,10 +975,21 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
 /*
  * page migration, thp tail pages can be passed.
  */
-static void migrate_page_add(struct page *page, struct list_head *pagelist,
+static int migrate_page_add(struct page *page, struct list_head *pagelist,
 				unsigned long flags)
 {
 	struct page *head = compound_head(page);
+
+	/*
+	 * Non-movable page may reach here.  And, there may be
+	 * temporaty off LRU pages or non-LRU movable pages.
+	 * Treat them as unmovable pages since they can't be
+	 * isolated, so they can't be moved at the moment.  It
+	 * should return -EIO for this case too.
+	 */
+	if (!PageLRU(head) && (flags & MPOL_MF_STRICT))
+		return -EIO;
+
 	/*
 	 * Avoid migrating a page that is shared with others.
 	 */
@@ -984,6 +1001,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 				hpage_nr_pages(head));
 		}
 	}
+
+	return 0;
 }
 
 /* page allocation callback for NUMA node migration */
@@ -1186,9 +1205,10 @@ static struct page *new_page(struct page *page, unsigned long start)
 }
 #else
 
-static void migrate_page_add(struct page *page, struct list_head *pagelist,
+static int migrate_page_add(struct page *page, struct list_head *pagelist,
 				unsigned long flags)
 {
+	return -EIO;
 }
 
 int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
-- 
1.8.3.1

Re: [v2 PATCH 0/2] mm: mempolicy: fix mbind()'s inconsistent behavior for unmovable pages

[Andrew Morton]

On Sat, 22 Jun 2019 08:20:07 +0800 Yang Shi <yang.shi@linux.alibaba.com> wrote:

> 
> Changelog
> v2: * Fixed the inconsistent behavior by not aborting !vma_migratable()
>       immediately by a separate patch (patch 1/2), and this is also the
>       preparation for patch 2/2. For the details please see the commit
>       log.  Per Vlastimil.
>     * Not abort immediately if unmovable page is met. This should handle
>       non-LRU movable pages and temporary off-LRU pages more friendly.
>       Per Vlastimil and Michal Hocko.
> 
> Yang Shi (2):
>       mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified
>       mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind
> 

I'm seeing no evidence of review on these two.  Could we please take a
look?  2/2 fixes a kernel crash so let's please also think about the
-stable situation.

I have a note here that Vlastimil had an issue with [1/2] but I seem to
hae misplaced that email :(

Re: [v2 PATCH 0/2] mm: mempolicy: fix mbind()'s inconsistent behavior for unmovable pages

[Yang Shi]

On 7/15/19 3:22 PM, Andrew Morton wrote:
> On Sat, 22 Jun 2019 08:20:07 +0800 Yang Shi <yang.shi@linux.alibaba.com> wrote:
>
>> Changelog
>> v2: * Fixed the inconsistent behavior by not aborting !vma_migratable()
>>        immediately by a separate patch (patch 1/2), and this is also the
>>        preparation for patch 2/2. For the details please see the commit
>>        log.  Per Vlastimil.
>>      * Not abort immediately if unmovable page is met. This should handle
>>        non-LRU movable pages and temporary off-LRU pages more friendly.
>>        Per Vlastimil and Michal Hocko.
>>
>> Yang Shi (2):
>>        mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified
>>        mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind
>>
> I'm seeing no evidence of review on these two.  Could we please take a
> look?  2/2 fixes a kernel crash so let's please also think about the
> -stable situation.

Thanks for following up this. It seems I have a few patches stalled due 
to lack of review.

BTW, this would not crash post-4.9 kernel since that BUG_ON had been 
removed. But, that behavior is definitely problematic as the commit log 
elaborated.

>
> I have a note here that Vlastimil had an issue with [1/2] but I seem to
> hae misplaced that email :(

Re: [v2 PATCH 0/2] mm: mempolicy: fix mbind()'s inconsistent behavior for unmovable pages

[Yang Shi]

On 7/15/19 4:51 PM, Yang Shi wrote:
>
>
> On 7/15/19 3:22 PM, Andrew Morton wrote:
>> On Sat, 22 Jun 2019 08:20:07 +0800 Yang Shi 
>> <yang.shi@linux.alibaba.com> wrote:
>>
>>> Changelog
>>> v2: * Fixed the inconsistent behavior by not aborting !vma_migratable()
>>>        immediately by a separate patch (patch 1/2), and this is also 
>>> the
>>>        preparation for patch 2/2. For the details please see the commit
>>>        log.  Per Vlastimil.
>>>      * Not abort immediately if unmovable page is met. This should 
>>> handle
>>>        non-LRU movable pages and temporary off-LRU pages more friendly.
>>>        Per Vlastimil and Michal Hocko.
>>>
>>> Yang Shi (2):
>>>        mm: mempolicy: make the behavior consistent when 
>>> MPOL_MF_MOVE* and MPOL_MF_STRICT were specified
>>>        mm: mempolicy: handle vma with unmovable pages mapped 
>>> correctly in mbind
>>>
>> I'm seeing no evidence of review on these two.  Could we please take a
>> look?  2/2 fixes a kernel crash so let's please also think about the
>> -stable situation.
>
> Thanks for following up this. It seems I have a few patches stalled 
> due to lack of review.
>
> BTW, this would not crash post-4.9 kernel since that BUG_ON had been 
> removed. But, that behavior is definitely problematic as the commit 
> log elaborated.
>
>>
>> I have a note here that Vlastimil had an issue with [1/2] but I seem to
>> hae misplaced that email :(

Vlastimil suggested something for v1, then I think his concern and 
suggestion have been solved in this version. But, the review was stalled.

Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

[Vlastimil Babka]

On 6/22/19 2:20 AM, Yang Shi wrote:
> When both MPOL_MF_MOVE* and MPOL_MF_STRICT was specified, mbind() should
> try best to migrate misplaced pages, if some of the pages could not be
> migrated, then return -EIO.
> 
> There are three different sub-cases:
> 1. vma is not migratable
> 2. vma is migratable, but there are unmovable pages
> 3. vma is migratable, pages are movable, but migrate_pages() fails
> 
> If #1 happens, kernel would just abort immediately, then return -EIO,
> after the commit a7f40cfe3b7ada57af9b62fd28430eeb4a7cfcb7 ("mm:
> mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified").
> 
> If #3 happens, kernel would set policy and migrate pages with best-effort,
> but won't rollback the migrated pages and reset the policy back.
> 
> Before that commit, they behaves in the same way.  It'd better to keep
> their behavior consistent.  But, rolling back the migrated pages and
> resetting the policy back sounds not feasible, so just make #1 behave as
> same as #3.
> 
> Userspace will know that not everything was successfully migrated (via
> -EIO), and can take whatever steps it deems necessary - attempt rollback,
> determine which exact page(s) are violating the policy, etc.
> 
> Make queue_pages_range() return 1 to indicate there are unmovable pages
> or vma is not migratable.
> 
> The #2 is not handled correctly in the current kernel, the following
> patch will fix it.
> 
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>

Agreed with the goal, but I think there's a bug, and room for improvement.

> ---
>  mm/mempolicy.c | 86 +++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 61 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 01600d8..b50039c 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -429,11 +429,14 @@ static inline bool queue_pages_required(struct page *page,
>  }
>  
>  /*
> - * queue_pages_pmd() has three possible return values:
> + * queue_pages_pmd() has four possible return values:
> + * 2 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
> + *     specified.
>   * 1 - pages are placed on the right node or queued successfully.
>   * 0 - THP was split.

I think if you renumbered these, it would be more consistent with
queue_pages_pte_range() and simplify the code there.
0 - pages on right node/queued
1 - unmovable page with right flags specified
2 - THP split

> - * -EIO - is migration entry or MPOL_MF_STRICT was specified and an existing
> - *        page was already on a node that does not follow the policy.
> + * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an
> + *        existing page was already on a node that does not follow the
> + *        policy.
>   */
>  static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
>  				unsigned long end, struct mm_walk *walk)
> @@ -463,7 +466,7 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
>  	/* go to thp migration */
>  	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
>  		if (!vma_migratable(walk->vma)) {
> -			ret = -EIO;
> +			ret = 2;
>  			goto unlock;
>  		}
>  
> @@ -488,16 +491,29 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,

Perhaps this function now also deserves a list of possible return values.

>  	struct queue_pages *qp = walk->private;
>  	unsigned long flags = qp->flags;
>  	int ret;
> +	bool has_unmovable = false;
>  	pte_t *pte;
>  	spinlock_t *ptl;
>  
>  	ptl = pmd_trans_huge_lock(pmd, vma);
>  	if (ptl) {
>  		ret = queue_pages_pmd(pmd, ptl, addr, end, walk);
> -		if (ret > 0)
> +		switch (ret) {

With renumbering suggested above, this could be:
if (ret != 2)
    return ret;

> +		/* THP was split, fall through to pte walk */
> +		case 0:
> +			break;
> +		/* Pages are placed on the right node or queued successfully */
> +		case 1:
>  			return 0;
> -		else if (ret < 0)
> +		/*
> +		 * Met unmovable pages, MPOL_MF_MOVE* & MPOL_MF_STRICT
> +		 * were specified.
> +		 */
> +		case 2:
> +			return 1;
> +		case -EIO:
>  			return ret;
> +		}
>  	}
>  
>  	if (pmd_trans_unstable(pmd))
> @@ -519,14 +535,21 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
>  		if (!queue_pages_required(page, qp))
>  			continue;
>  		if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
> -			if (!vma_migratable(vma))
> +			/* MPOL_MF_STRICT must be specified if we get here */
> +			if (!vma_migratable(vma)) {
> +				has_unmovable |= true;
>  				break;
> +			}
>  			migrate_page_add(page, qp->pagelist, flags);
>  		} else
>  			break;
>  	}
>  	pte_unmap_unlock(pte - 1, ptl);
>  	cond_resched();
> +
> +	if (has_unmovable)
> +		return 1;
> +
>  	return addr != end ? -EIO : 0;
>  }
>  
> @@ -639,7 +662,13 @@ static int queue_pages_test_walk(unsigned long start, unsigned long end,
>   *
>   * If pages found in a given range are on a set of nodes (determined by
>   * @nodes and @flags,) it's isolated and queued to the pagelist which is
> - * passed via @private.)
> + * passed via @private.
> + *
> + * queue_pages_range() has three possible return values:
> + * 1 - there is unmovable page, but MPOL_MF_MOVE* & MPOL_MF_STRICT were
> + *     specified.
> + * 0 - queue pages successfully or no misplaced page.
> + * -EIO - there is misplaced page and only MPOL_MF_STRICT was specified.
>   */
>  static int
>  queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
> @@ -1182,6 +1211,7 @@ static long do_mbind(unsigned long start, unsigned long len,
>  	struct mempolicy *new;
>  	unsigned long end;
>  	int err;
> +	int ret;
>  	LIST_HEAD(pagelist);
>  
>  	if (flags & ~(unsigned long)MPOL_MF_VALID)
> @@ -1243,26 +1273,32 @@ static long do_mbind(unsigned long start, unsigned long len,
>  	if (err)
>  		goto mpol_out;
>  
> -	err = queue_pages_range(mm, start, end, nmask,
> +	ret = queue_pages_range(mm, start, end, nmask,
>  			  flags | MPOL_MF_INVERT, &pagelist);
> -	if (!err)
> -		err = mbind_range(mm, start, end, new);
> -
> -	if (!err) {
> -		int nr_failed = 0;
>  
> -		if (!list_empty(&pagelist)) {
> -			WARN_ON_ONCE(flags & MPOL_MF_LAZY);
> -			nr_failed = migrate_pages(&pagelist, new_page, NULL,
> -				start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
> -			if (nr_failed)
> -				putback_movable_pages(&pagelist);
> -		}
> +	if (ret < 0)
> +		err = -EIO;

I think after your patch, you miss putback_movable_pages() in cases
where some were queued, and later the walk returned -EIO. The previous
code doesn't miss it, but it's also not obvious due to the multiple if
(!err) checks. I would rewrite it some thing like this:

if (ret < 0) {
    putback_movable_pages(&pagelist);
    err = ret;
    goto mmap_out; // a new label above up_write()
}

The rest can have reduced identation now.

> +	else {
> +		err = mbind_range(mm, start, end, new);
>  
> -		if (nr_failed && (flags & MPOL_MF_STRICT))
> -			err = -EIO;
> -	} else
> -		putback_movable_pages(&pagelist);
> +		if (!err) {
> +			int nr_failed = 0;
> +
> +			if (!list_empty(&pagelist)) {
> +				WARN_ON_ONCE(flags & MPOL_MF_LAZY);
> +				nr_failed = migrate_pages(&pagelist, new_page,
> +					NULL, start, MIGRATE_SYNC,
> +					MR_MEMPOLICY_MBIND);
> +				if (nr_failed)
> +					putback_movable_pages(&pagelist);
> +			}
> +
> +			if ((ret > 0) ||
> +			    (nr_failed && (flags & MPOL_MF_STRICT)))
> +				err = -EIO;
> +		} else
> +			putback_movable_pages(&pagelist);

While at it, IIRC the kernel style says that when the 'if' part uses
'{ }' then the 'else' part should as well, and it shouldn't be mixed.

Thanks,
Vlastimil

> +	}
>  
>  	up_write(&mm->mmap_sem);
>   mpol_out:
> 

Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

[Vlastimil Babka]

On 7/16/19 10:12 AM, Vlastimil Babka wrote:
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -429,11 +429,14 @@ static inline bool queue_pages_required(struct page *page,
>>  }
>>  
>>  /*
>> - * queue_pages_pmd() has three possible return values:
>> + * queue_pages_pmd() has four possible return values:
>> + * 2 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
>> + *     specified.
>>   * 1 - pages are placed on the right node or queued successfully.
>>   * 0 - THP was split.
> 
> I think if you renumbered these, it would be more consistent with
> queue_pages_pte_range() and simplify the code there.
> 0 - pages on right node/queued
> 1 - unmovable page with right flags specified
> 2 - THP split

Ah, alternatively you could add a boolean to struct queue_pages
accessible from mm_walk, set true to indicate that unmovable page has
been encountered, without propagating it back through special return values.

Re: [v2 PATCH 2/2] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

[Vlastimil Babka]

On 6/22/19 2:20 AM, Yang Shi wrote:
> @@ -969,10 +975,21 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
>  /*
>   * page migration, thp tail pages can be passed.
>   */
> -static void migrate_page_add(struct page *page, struct list_head *pagelist,
> +static int migrate_page_add(struct page *page, struct list_head *pagelist,
>  				unsigned long flags)
>  {
>  	struct page *head = compound_head(page);
> +
> +	/*
> +	 * Non-movable page may reach here.  And, there may be
> +	 * temporaty off LRU pages or non-LRU movable pages.
> +	 * Treat them as unmovable pages since they can't be
> +	 * isolated, so they can't be moved at the moment.  It
> +	 * should return -EIO for this case too.
> +	 */
> +	if (!PageLRU(head) && (flags & MPOL_MF_STRICT))
> +		return -EIO;
> +

Hm but !PageLRU() is not the only way why queueing for migration can
fail, as can be seen from the rest of the function. Shouldn't all cases
be reported?

>  	/*
>  	 * Avoid migrating a page that is shared with others.
>  	 */
> @@ -984,6 +1001,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
>  				hpage_nr_pages(head));
>  		}
>  	}
> +
> +	return 0;
>  }
>  
>  /* page allocation callback for NUMA node migration */
> @@ -1186,9 +1205,10 @@ static struct page *new_page(struct page *page, unsigned long start)
>  }
>  #else
>  
> -static void migrate_page_add(struct page *page, struct list_head *pagelist,
> +static int migrate_page_add(struct page *page, struct list_head *pagelist,
>  				unsigned long flags)
>  {
> +	return -EIO;
>  }
>  
>  int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
> 

Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

[Yang Shi]

On 7/16/19 1:12 AM, Vlastimil Babka wrote:
> On 6/22/19 2:20 AM, Yang Shi wrote:
>> When both MPOL_MF_MOVE* and MPOL_MF_STRICT was specified, mbind() should
>> try best to migrate misplaced pages, if some of the pages could not be
>> migrated, then return -EIO.
>>
>> There are three different sub-cases:
>> 1. vma is not migratable
>> 2. vma is migratable, but there are unmovable pages
>> 3. vma is migratable, pages are movable, but migrate_pages() fails
>>
>> If #1 happens, kernel would just abort immediately, then return -EIO,
>> after the commit a7f40cfe3b7ada57af9b62fd28430eeb4a7cfcb7 ("mm:
>> mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified").
>>
>> If #3 happens, kernel would set policy and migrate pages with best-effort,
>> but won't rollback the migrated pages and reset the policy back.
>>
>> Before that commit, they behaves in the same way.  It'd better to keep
>> their behavior consistent.  But, rolling back the migrated pages and
>> resetting the policy back sounds not feasible, so just make #1 behave as
>> same as #3.
>>
>> Userspace will know that not everything was successfully migrated (via
>> -EIO), and can take whatever steps it deems necessary - attempt rollback,
>> determine which exact page(s) are violating the policy, etc.
>>
>> Make queue_pages_range() return 1 to indicate there are unmovable pages
>> or vma is not migratable.
>>
>> The #2 is not handled correctly in the current kernel, the following
>> patch will fix it.
>>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> Agreed with the goal, but I think there's a bug, and room for improvement.
>
>> ---
>>   mm/mempolicy.c | 86 +++++++++++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 61 insertions(+), 25 deletions(-)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index 01600d8..b50039c 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -429,11 +429,14 @@ static inline bool queue_pages_required(struct page *page,
>>   }
>>   
>>   /*
>> - * queue_pages_pmd() has three possible return values:
>> + * queue_pages_pmd() has four possible return values:
>> + * 2 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
>> + *     specified.
>>    * 1 - pages are placed on the right node or queued successfully.
>>    * 0 - THP was split.
> I think if you renumbered these, it would be more consistent with
> queue_pages_pte_range() and simplify the code there.
> 0 - pages on right node/queued
> 1 - unmovable page with right flags specified
> 2 - THP split
>
>> - * -EIO - is migration entry or MPOL_MF_STRICT was specified and an existing
>> - *        page was already on a node that does not follow the policy.
>> + * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an
>> + *        existing page was already on a node that does not follow the
>> + *        policy.
>>    */
>>   static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
>>   				unsigned long end, struct mm_walk *walk)
>> @@ -463,7 +466,7 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
>>   	/* go to thp migration */
>>   	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
>>   		if (!vma_migratable(walk->vma)) {
>> -			ret = -EIO;
>> +			ret = 2;
>>   			goto unlock;
>>   		}
>>   
>> @@ -488,16 +491,29 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
> Perhaps this function now also deserves a list of possible return values.

Sure, will add some comments to elaborate the return values.

>
>>   	struct queue_pages *qp = walk->private;
>>   	unsigned long flags = qp->flags;
>>   	int ret;
>> +	bool has_unmovable = false;
>>   	pte_t *pte;
>>   	spinlock_t *ptl;
>>   
>>   	ptl = pmd_trans_huge_lock(pmd, vma);
>>   	if (ptl) {
>>   		ret = queue_pages_pmd(pmd, ptl, addr, end, walk);
>> -		if (ret > 0)
>> +		switch (ret) {
> With renumbering suggested above, this could be:
> if (ret != 2)
>      return ret;
>
>> +		/* THP was split, fall through to pte walk */
>> +		case 0:
>> +			break;
>> +		/* Pages are placed on the right node or queued successfully */
>> +		case 1:
>>   			return 0;
>> -		else if (ret < 0)
>> +		/*
>> +		 * Met unmovable pages, MPOL_MF_MOVE* & MPOL_MF_STRICT
>> +		 * were specified.
>> +		 */
>> +		case 2:
>> +			return 1;
>> +		case -EIO:
>>   			return ret;
>> +		}
>>   	}
>>   
>>   	if (pmd_trans_unstable(pmd))
>> @@ -519,14 +535,21 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
>>   		if (!queue_pages_required(page, qp))
>>   			continue;
>>   		if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {
>> -			if (!vma_migratable(vma))
>> +			/* MPOL_MF_STRICT must be specified if we get here */
>> +			if (!vma_migratable(vma)) {
>> +				has_unmovable |= true;
>>   				break;
>> +			}
>>   			migrate_page_add(page, qp->pagelist, flags);
>>   		} else
>>   			break;
>>   	}
>>   	pte_unmap_unlock(pte - 1, ptl);
>>   	cond_resched();
>> +
>> +	if (has_unmovable)
>> +		return 1;
>> +
>>   	return addr != end ? -EIO : 0;
>>   }
>>   
>> @@ -639,7 +662,13 @@ static int queue_pages_test_walk(unsigned long start, unsigned long end,
>>    *
>>    * If pages found in a given range are on a set of nodes (determined by
>>    * @nodes and @flags,) it's isolated and queued to the pagelist which is
>> - * passed via @private.)
>> + * passed via @private.
>> + *
>> + * queue_pages_range() has three possible return values:
>> + * 1 - there is unmovable page, but MPOL_MF_MOVE* & MPOL_MF_STRICT were
>> + *     specified.
>> + * 0 - queue pages successfully or no misplaced page.
>> + * -EIO - there is misplaced page and only MPOL_MF_STRICT was specified.
>>    */
>>   static int
>>   queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
>> @@ -1182,6 +1211,7 @@ static long do_mbind(unsigned long start, unsigned long len,
>>   	struct mempolicy *new;
>>   	unsigned long end;
>>   	int err;
>> +	int ret;
>>   	LIST_HEAD(pagelist);
>>   
>>   	if (flags & ~(unsigned long)MPOL_MF_VALID)
>> @@ -1243,26 +1273,32 @@ static long do_mbind(unsigned long start, unsigned long len,
>>   	if (err)
>>   		goto mpol_out;
>>   
>> -	err = queue_pages_range(mm, start, end, nmask,
>> +	ret = queue_pages_range(mm, start, end, nmask,
>>   			  flags | MPOL_MF_INVERT, &pagelist);
>> -	if (!err)
>> -		err = mbind_range(mm, start, end, new);
>> -
>> -	if (!err) {
>> -		int nr_failed = 0;
>>   
>> -		if (!list_empty(&pagelist)) {
>> -			WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>> -			nr_failed = migrate_pages(&pagelist, new_page, NULL,
>> -				start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
>> -			if (nr_failed)
>> -				putback_movable_pages(&pagelist);
>> -		}
>> +	if (ret < 0)
>> +		err = -EIO;
> I think after your patch, you miss putback_movable_pages() in cases
> where some were queued, and later the walk returned -EIO. The previous
> code doesn't miss it, but it's also not obvious due to the multiple if
> (!err) checks. I would rewrite it some thing like this:
>
> if (ret < 0) {
>      putback_movable_pages(&pagelist);
>      err = ret;
>      goto mmap_out; // a new label above up_write()
> }

Yes, the old code had putback_movable_pages called when !err. But, I 
think that is for error handling of mbind_range() if I understand it 
correctly since if queue_pages_range() returns -EIO (only MPOL_MF_STRICT 
was specified and there was misplaced page) that page list should be 
empty . The old code should checked whether that list is empty or not.

So, in the new code I just removed that.

>
> The rest can have reduced identation now.

Yes, the goto does eliminate the extra indentation.

>
>> +	else {
>> +		err = mbind_range(mm, start, end, new);
>>   
>> -		if (nr_failed && (flags & MPOL_MF_STRICT))
>> -			err = -EIO;
>> -	} else
>> -		putback_movable_pages(&pagelist);
>> +		if (!err) {
>> +			int nr_failed = 0;
>> +
>> +			if (!list_empty(&pagelist)) {
>> +				WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>> +				nr_failed = migrate_pages(&pagelist, new_page,
>> +					NULL, start, MIGRATE_SYNC,
>> +					MR_MEMPOLICY_MBIND);
>> +				if (nr_failed)
>> +					putback_movable_pages(&pagelist);
>> +			}
>> +
>> +			if ((ret > 0) ||
>> +			    (nr_failed && (flags & MPOL_MF_STRICT)))
>> +				err = -EIO;
>> +		} else
>> +			putback_movable_pages(&pagelist);
> While at it, IIRC the kernel style says that when the 'if' part uses
> '{ }' then the 'else' part should as well, and it shouldn't be mixed.

Really? The old code doesn't have '{ }' for else, and checkpatch doesn't 
report any error or warning.

Thanks,
Yang

>
> Thanks,
> Vlastimil
>
>> +	}
>>   
>>   	up_write(&mm->mmap_sem);
>>    mpol_out:
>>

Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

[Yang Shi]

On 7/16/19 1:47 AM, Vlastimil Babka wrote:
> On 7/16/19 10:12 AM, Vlastimil Babka wrote:
>>> --- a/mm/mempolicy.c
>>> +++ b/mm/mempolicy.c
>>> @@ -429,11 +429,14 @@ static inline bool queue_pages_required(struct page *page,
>>>   }
>>>   
>>>   /*
>>> - * queue_pages_pmd() has three possible return values:
>>> + * queue_pages_pmd() has four possible return values:
>>> + * 2 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
>>> + *     specified.
>>>    * 1 - pages are placed on the right node or queued successfully.
>>>    * 0 - THP was split.
>> I think if you renumbered these, it would be more consistent with
>> queue_pages_pte_range() and simplify the code there.
>> 0 - pages on right node/queued
>> 1 - unmovable page with right flags specified
>> 2 - THP split
> Ah, alternatively you could add a boolean to struct queue_pages
> accessible from mm_walk, set true to indicate that unmovable page has
> been encountered, without propagating it back through special return values.

I will try both to see which one (renumbering return value or use flag) 
is better.

Thanks,
Yang

Re: [v2 PATCH 2/2] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

[Yang Shi]

On 7/16/19 5:07 AM, Vlastimil Babka wrote:
> On 6/22/19 2:20 AM, Yang Shi wrote:
>> @@ -969,10 +975,21 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
>>   /*
>>    * page migration, thp tail pages can be passed.
>>    */
>> -static void migrate_page_add(struct page *page, struct list_head *pagelist,
>> +static int migrate_page_add(struct page *page, struct list_head *pagelist,
>>   				unsigned long flags)
>>   {
>>   	struct page *head = compound_head(page);
>> +
>> +	/*
>> +	 * Non-movable page may reach here.  And, there may be
>> +	 * temporaty off LRU pages or non-LRU movable pages.
>> +	 * Treat them as unmovable pages since they can't be
>> +	 * isolated, so they can't be moved at the moment.  It
>> +	 * should return -EIO for this case too.
>> +	 */
>> +	if (!PageLRU(head) && (flags & MPOL_MF_STRICT))
>> +		return -EIO;
>> +
> Hm but !PageLRU() is not the only way why queueing for migration can
> fail, as can be seen from the rest of the function. Shouldn't all cases
> be reported?

Do you mean the shared pages and isolation failed pages? I'm not sure 
whether we should consider these cases break the semantics or not, so I 
leave them as they are. But, strictly speaking they should be reported 
too, at least for the isolation failed page.

Thanks,
Yang

>
>>   	/*
>>   	 * Avoid migrating a page that is shared with others.
>>   	 */
>> @@ -984,6 +1001,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
>>   				hpage_nr_pages(head));
>>   		}
>>   	}
>> +
>> +	return 0;
>>   }
>>   
>>   /* page allocation callback for NUMA node migration */
>> @@ -1186,9 +1205,10 @@ static struct page *new_page(struct page *page, unsigned long start)
>>   }
>>   #else
>>   
>> -static void migrate_page_add(struct page *page, struct list_head *pagelist,
>> +static int migrate_page_add(struct page *page, struct list_head *pagelist,
>>   				unsigned long flags)
>>   {
>> +	return -EIO;
>>   }
>>   
>>   int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
>>

Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

[Vlastimil Babka]

On 7/16/19 7:18 PM, Yang Shi wrote:
>> I think after your patch, you miss putback_movable_pages() in cases
>> where some were queued, and later the walk returned -EIO. The previous
>> code doesn't miss it, but it's also not obvious due to the multiple if
>> (!err) checks. I would rewrite it some thing like this:
>>
>> if (ret < 0) {
>>      putback_movable_pages(&pagelist);
>>      err = ret;
>>      goto mmap_out; // a new label above up_write()
>> }
> 
> Yes, the old code had putback_movable_pages called when !err. But, I 
> think that is for error handling of mbind_range() if I understand it 
> correctly since if queue_pages_range() returns -EIO (only MPOL_MF_STRICT 
> was specified and there was misplaced page) that page list should be 
> empty . The old code should checked whether that list is empty or not.

Hm I guess you're right, returning with EIO means nothing was queued.
> So, in the new code I just removed that.
> 
>>
>> The rest can have reduced identation now.
> 
> Yes, the goto does eliminate the extra indentation.
> 
>>
>>> +	else {
>>> +		err = mbind_range(mm, start, end, new);
>>>   
>>> -		if (nr_failed && (flags & MPOL_MF_STRICT))
>>> -			err = -EIO;
>>> -	} else
>>> -		putback_movable_pages(&pagelist);
>>> +		if (!err) {
>>> +			int nr_failed = 0;
>>> +
>>> +			if (!list_empty(&pagelist)) {
>>> +				WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>>> +				nr_failed = migrate_pages(&pagelist, new_page,
>>> +					NULL, start, MIGRATE_SYNC,
>>> +					MR_MEMPOLICY_MBIND);
>>> +				if (nr_failed)
>>> +					putback_movable_pages(&pagelist);
>>> +			}
>>> +
>>> +			if ((ret > 0) ||
>>> +			    (nr_failed && (flags & MPOL_MF_STRICT)))
>>> +				err = -EIO;
>>> +		} else
>>> +			putback_movable_pages(&pagelist);
>> While at it, IIRC the kernel style says that when the 'if' part uses
>> '{ }' then the 'else' part should as well, and it shouldn't be mixed.
> 
> Really? The old code doesn't have '{ }' for else, and checkpatch doesn't 
> report any error or warning.

Checkpatch probably doesn't catch it, nor did the reviewers of the older
code. But coding-style.rst says:

Do not unnecessarily use braces where a single statement will do.

...

This does not apply if only one branch of a conditional statement is a
single
statement; in the latter case use braces in both branches:

.. code-block:: c

        if (condition) {
                do_this();
                do_that();
        } else {
                otherwise();
        }


Thanks,
Vlastimil

Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

[Yang Shi]

On 7/17/19 3:55 AM, Vlastimil Babka wrote:
> On 7/16/19 7:18 PM, Yang Shi wrote:
>>> I think after your patch, you miss putback_movable_pages() in cases
>>> where some were queued, and later the walk returned -EIO. The previous
>>> code doesn't miss it, but it's also not obvious due to the multiple if
>>> (!err) checks. I would rewrite it some thing like this:
>>>
>>> if (ret < 0) {
>>>       putback_movable_pages(&pagelist);
>>>       err = ret;
>>>       goto mmap_out; // a new label above up_write()
>>> }
>> Yes, the old code had putback_movable_pages called when !err. But, I
>> think that is for error handling of mbind_range() if I understand it
>> correctly since if queue_pages_range() returns -EIO (only MPOL_MF_STRICT
>> was specified and there was misplaced page) that page list should be
>> empty . The old code should checked whether that list is empty or not.
> Hm I guess you're right, returning with EIO means nothing was queued.
>> So, in the new code I just removed that.
>>
>>> The rest can have reduced identation now.
>> Yes, the goto does eliminate the extra indentation.
>>
>>>> +	else {
>>>> +		err = mbind_range(mm, start, end, new);
>>>>    
>>>> -		if (nr_failed && (flags & MPOL_MF_STRICT))
>>>> -			err = -EIO;
>>>> -	} else
>>>> -		putback_movable_pages(&pagelist);
>>>> +		if (!err) {
>>>> +			int nr_failed = 0;
>>>> +
>>>> +			if (!list_empty(&pagelist)) {
>>>> +				WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>>>> +				nr_failed = migrate_pages(&pagelist, new_page,
>>>> +					NULL, start, MIGRATE_SYNC,
>>>> +					MR_MEMPOLICY_MBIND);
>>>> +				if (nr_failed)
>>>> +					putback_movable_pages(&pagelist);
>>>> +			}
>>>> +
>>>> +			if ((ret > 0) ||
>>>> +			    (nr_failed && (flags & MPOL_MF_STRICT)))
>>>> +				err = -EIO;
>>>> +		} else
>>>> +			putback_movable_pages(&pagelist);
>>> While at it, IIRC the kernel style says that when the 'if' part uses
>>> '{ }' then the 'else' part should as well, and it shouldn't be mixed.
>> Really? The old code doesn't have '{ }' for else, and checkpatch doesn't
>> report any error or warning.
> Checkpatch probably doesn't catch it, nor did the reviewers of the older
> code. But coding-style.rst says:
>
> Do not unnecessarily use braces where a single statement will do.
>
> ...
>
> This does not apply if only one branch of a conditional statement is a
> single
> statement; in the latter case use braces in both branches:
>
> .. code-block:: c
>
>          if (condition) {
>                  do_this();
>                  do_that();
>          } else {
>                  otherwise();
>          }

Thanks. Good to know this. Anyway, with the "goto" suggested above, we 
don't need that "else" anymore and we could save some change of lines.

>
>
> Thanks,
> Vlastimil

Re: [v2 PATCH 2/2] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

[Yang Shi]

On 7/16/19 10:28 AM, Yang Shi wrote:
>
>
> On 7/16/19 5:07 AM, Vlastimil Babka wrote:
>> On 6/22/19 2:20 AM, Yang Shi wrote:
>>> @@ -969,10 +975,21 @@ static long do_get_mempolicy(int *policy, 
>>> nodemask_t *nmask,
>>>   /*
>>>    * page migration, thp tail pages can be passed.
>>>    */
>>> -static void migrate_page_add(struct page *page, struct list_head 
>>> *pagelist,
>>> +static int migrate_page_add(struct page *page, struct list_head 
>>> *pagelist,
>>>                   unsigned long flags)
>>>   {
>>>       struct page *head = compound_head(page);
>>> +
>>> +    /*
>>> +     * Non-movable page may reach here.  And, there may be
>>> +     * temporaty off LRU pages or non-LRU movable pages.
>>> +     * Treat them as unmovable pages since they can't be
>>> +     * isolated, so they can't be moved at the moment.  It
>>> +     * should return -EIO for this case too.
>>> +     */
>>> +    if (!PageLRU(head) && (flags & MPOL_MF_STRICT))
>>> +        return -EIO;
>>> +
>> Hm but !PageLRU() is not the only way why queueing for migration can
>> fail, as can be seen from the rest of the function. Shouldn't all cases
>> be reported?
>
> Do you mean the shared pages and isolation failed pages? I'm not sure 
> whether we should consider these cases break the semantics or not, so 
> I leave them as they are. But, strictly speaking they should be 
> reported too, at least for the isolation failed page.

By reading mbind man page, it says:

If MPOL_MF_MOVE is specified in flags, then the kernel will attempt to 
move all the existing pages in the memory range so that they follow the 
policy.  Pages that are shared with other processes will not be moved.  
If MPOL_MF_STRICT is also specified, then the call fails with the error 
EIO if some pages could not be moved.

It looks the code already handles shared page correctly, we just need 
return -EIO for isolation failed page if MPOL_MF_STRICT is specified.

>
> Thanks,
> Yang
>
>>
>>>       /*
>>>        * Avoid migrating a page that is shared with others.
>>>        */
>>> @@ -984,6 +1001,8 @@ static void migrate_page_add(struct page *page, 
>>> struct list_head *pagelist,
>>>                   hpage_nr_pages(head));
>>>           }
>>>       }
>>> +
>>> +    return 0;
>>>   }
>>>     /* page allocation callback for NUMA node migration */
>>> @@ -1186,9 +1205,10 @@ static struct page *new_page(struct page 
>>> *page, unsigned long start)
>>>   }
>>>   #else
>>>   -static void migrate_page_add(struct page *page, struct list_head 
>>> *pagelist,
>>> +static int migrate_page_add(struct page *page, struct list_head 
>>> *pagelist,
>>>                   unsigned long flags)
>>>   {
>>> +    return -EIO;
>>>   }
>>>     int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
>>>
>

Re: [v2 PATCH 2/2] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

[Yang Shi]

On 7/17/19 11:23 AM, Yang Shi wrote:
>
>
> On 7/16/19 10:28 AM, Yang Shi wrote:
>>
>>
>> On 7/16/19 5:07 AM, Vlastimil Babka wrote:
>>> On 6/22/19 2:20 AM, Yang Shi wrote:
>>>> @@ -969,10 +975,21 @@ static long do_get_mempolicy(int *policy, 
>>>> nodemask_t *nmask,
>>>>   /*
>>>>    * page migration, thp tail pages can be passed.
>>>>    */
>>>> -static void migrate_page_add(struct page *page, struct list_head 
>>>> *pagelist,
>>>> +static int migrate_page_add(struct page *page, struct list_head 
>>>> *pagelist,
>>>>                   unsigned long flags)
>>>>   {
>>>>       struct page *head = compound_head(page);
>>>> +
>>>> +    /*
>>>> +     * Non-movable page may reach here.  And, there may be
>>>> +     * temporaty off LRU pages or non-LRU movable pages.
>>>> +     * Treat them as unmovable pages since they can't be
>>>> +     * isolated, so they can't be moved at the moment.  It
>>>> +     * should return -EIO for this case too.
>>>> +     */
>>>> +    if (!PageLRU(head) && (flags & MPOL_MF_STRICT))
>>>> +        return -EIO;
>>>> +
>>> Hm but !PageLRU() is not the only way why queueing for migration can
>>> fail, as can be seen from the rest of the function. Shouldn't all cases
>>> be reported?
>>
>> Do you mean the shared pages and isolation failed pages? I'm not sure 
>> whether we should consider these cases break the semantics or not, so 
>> I leave them as they are. But, strictly speaking they should be 
>> reported too, at least for the isolation failed page.
>
> By reading mbind man page, it says:
>
> If MPOL_MF_MOVE is specified in flags, then the kernel will attempt to 
> move all the existing pages in the memory range so that they follow 
> the policy.  Pages that are shared with other processes will not be 
> moved.  If MPOL_MF_STRICT is also specified, then the call fails with 
> the error EIO if some pages could not be moved.
>
> It looks the code already handles shared page correctly, we just need 
> return -EIO for isolation failed page if MPOL_MF_STRICT is specified.

Second look shows isolate_lru_page() returns error when and only when 
the page is *not* on LRU. So, we don't need change anything to this patch.

>
>>
>> Thanks,
>> Yang
>>
>>>
>>>>       /*
>>>>        * Avoid migrating a page that is shared with others.
>>>>        */
>>>> @@ -984,6 +1001,8 @@ static void migrate_page_add(struct page 
>>>> *page, struct list_head *pagelist,
>>>>                   hpage_nr_pages(head));
>>>>           }
>>>>       }
>>>> +
>>>> +    return 0;
>>>>   }
>>>>     /* page allocation callback for NUMA node migration */
>>>> @@ -1186,9 +1205,10 @@ static struct page *new_page(struct page 
>>>> *page, unsigned long start)
>>>>   }
>>>>   #else
>>>>   -static void migrate_page_add(struct page *page, struct list_head 
>>>> *pagelist,
>>>> +static int migrate_page_add(struct page *page, struct list_head 
>>>> *pagelist,
>>>>                   unsigned long flags)
>>>>   {
>>>> +    return -EIO;
>>>>   }
>>>>     int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
>>>>
>>
>

Re: [v2 PATCH 2/2] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

[Vlastimil Babka]

On 7/17/19 8:23 PM, Yang Shi wrote:
> 
> 
> On 7/16/19 10:28 AM, Yang Shi wrote:
>>
>>
>> On 7/16/19 5:07 AM, Vlastimil Babka wrote:
>>> On 6/22/19 2:20 AM, Yang Shi wrote:
>>>> @@ -969,10 +975,21 @@ static long do_get_mempolicy(int *policy, 
>>>> nodemask_t *nmask,
>>>>   /*
>>>>    * page migration, thp tail pages can be passed.
>>>>    */
>>>> -static void migrate_page_add(struct page *page, struct list_head 
>>>> *pagelist,
>>>> +static int migrate_page_add(struct page *page, struct list_head 
>>>> *pagelist,
>>>>                   unsigned long flags)
>>>>   {
>>>>       struct page *head = compound_head(page);
>>>> +
>>>> +    /*
>>>> +     * Non-movable page may reach here.  And, there may be
>>>> +     * temporaty off LRU pages or non-LRU movable pages.
>>>> +     * Treat them as unmovable pages since they can't be
>>>> +     * isolated, so they can't be moved at the moment.  It
>>>> +     * should return -EIO for this case too.
>>>> +     */
>>>> +    if (!PageLRU(head) && (flags & MPOL_MF_STRICT))
>>>> +        return -EIO;
>>>> +
>>> Hm but !PageLRU() is not the only way why queueing for migration can
>>> fail, as can be seen from the rest of the function. Shouldn't all cases
>>> be reported?
>>
>> Do you mean the shared pages and isolation failed pages? I'm not sure 
>> whether we should consider these cases break the semantics or not, so 
>> I leave them as they are. But, strictly speaking they should be 
>> reported too, at least for the isolation failed page.

CC'd linux-api, should be done on v3 posting also.

> By reading mbind man page, it says:
> 
> If MPOL_MF_MOVE is specified in flags, then the kernel will attempt to 
> move all the existing pages in the memory range so that they follow the 
> policy.  Pages that are shared with other processes will not be moved.  
> If MPOL_MF_STRICT is also specified, then the call fails with the error 
> EIO if some pages could not be moved.

I don't think this means that for shared pages, -EIO should not be
reported. I can imagine both interpretations of the paragraph. I guess
we can be conservative and keep not reporting them, if that was always
the case - but then perhaps clarify the man page?

> It looks the code already handles shared page correctly, we just need 
> return -EIO for isolation failed page if MPOL_MF_STRICT is specified.
> 
>>
>> Thanks,
>> Yang
>>
>>>
>>>>       /*
>>>>        * Avoid migrating a page that is shared with others.
>>>>        */
>>>> @@ -984,6 +1001,8 @@ static void migrate_page_add(struct page *page, 
>>>> struct list_head *pagelist,
>>>>                   hpage_nr_pages(head));
>>>>           }
>>>>       }
>>>> +
>>>> +    return 0;
>>>>   }
>>>>     /* page allocation callback for NUMA node migration */
>>>> @@ -1186,9 +1205,10 @@ static struct page *new_page(struct page 
>>>> *page, unsigned long start)
>>>>   }
>>>>   #else
>>>>   -static void migrate_page_add(struct page *page, struct list_head 
>>>> *pagelist,
>>>> +static int migrate_page_add(struct page *page, struct list_head 
>>>> *pagelist,
>>>>                   unsigned long flags)
>>>>   {
>>>> +    return -EIO;
>>>>   }
>>>>     int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
>>>>
>>
> 

Re: [v2 PATCH 2/2] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

[Yang Shi]

On 7/17/19 11:50 AM, Vlastimil Babka wrote:
> On 7/17/19 8:23 PM, Yang Shi wrote:
>>
>> On 7/16/19 10:28 AM, Yang Shi wrote:
>>>
>>> On 7/16/19 5:07 AM, Vlastimil Babka wrote:
>>>> On 6/22/19 2:20 AM, Yang Shi wrote:
>>>>> @@ -969,10 +975,21 @@ static long do_get_mempolicy(int *policy,
>>>>> nodemask_t *nmask,
>>>>>    /*
>>>>>     * page migration, thp tail pages can be passed.
>>>>>     */
>>>>> -static void migrate_page_add(struct page *page, struct list_head
>>>>> *pagelist,
>>>>> +static int migrate_page_add(struct page *page, struct list_head
>>>>> *pagelist,
>>>>>                    unsigned long flags)
>>>>>    {
>>>>>        struct page *head = compound_head(page);
>>>>> +
>>>>> +    /*
>>>>> +     * Non-movable page may reach here.  And, there may be
>>>>> +     * temporaty off LRU pages or non-LRU movable pages.
>>>>> +     * Treat them as unmovable pages since they can't be
>>>>> +     * isolated, so they can't be moved at the moment.  It
>>>>> +     * should return -EIO for this case too.
>>>>> +     */
>>>>> +    if (!PageLRU(head) && (flags & MPOL_MF_STRICT))
>>>>> +        return -EIO;
>>>>> +
>>>> Hm but !PageLRU() is not the only way why queueing for migration can
>>>> fail, as can be seen from the rest of the function. Shouldn't all cases
>>>> be reported?
>>> Do you mean the shared pages and isolation failed pages? I'm not sure
>>> whether we should consider these cases break the semantics or not, so
>>> I leave them as they are. But, strictly speaking they should be
>>> reported too, at least for the isolation failed page.
> CC'd linux-api, should be done on v3 posting also.
>
>> By reading mbind man page, it says:
>>
>> If MPOL_MF_MOVE is specified in flags, then the kernel will attempt to
>> move all the existing pages in the memory range so that they follow the
>> policy.  Pages that are shared with other processes will not be moved.
>> If MPOL_MF_STRICT is also specified, then the call fails with the error
>> EIO if some pages could not be moved.
> I don't think this means that for shared pages, -EIO should not be
> reported. I can imagine both interpretations of the paragraph. I guess
> we can be conservative and keep not reporting them, if that was always
> the case - but then perhaps clarify the man page?

Yes, I agree the man page does looks ambiguous.  Anyway, I think we 
could add a patch later to kernel or manpage for either interpretations 
once it gets clarified.

>
>> It looks the code already handles shared page correctly, we just need
>> return -EIO for isolation failed page if MPOL_MF_STRICT is specified.
>>
>>> Thanks,
>>> Yang
>>>
>>>>>        /*
>>>>>         * Avoid migrating a page that is shared with others.
>>>>>         */
>>>>> @@ -984,6 +1001,8 @@ static void migrate_page_add(struct page *page,
>>>>> struct list_head *pagelist,
>>>>>                    hpage_nr_pages(head));
>>>>>            }
>>>>>        }
>>>>> +
>>>>> +    return 0;
>>>>>    }
>>>>>      /* page allocation callback for NUMA node migration */
>>>>> @@ -1186,9 +1205,10 @@ static struct page *new_page(struct page
>>>>> *page, unsigned long start)
>>>>>    }
>>>>>    #else
>>>>>    -static void migrate_page_add(struct page *page, struct list_head
>>>>> *pagelist,
>>>>> +static int migrate_page_add(struct page *page, struct list_head
>>>>> *pagelist,
>>>>>                    unsigned long flags)
>>>>>    {
>>>>> +    return -EIO;
>>>>>    }
>>>>>      int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
>>>>>