mm: fix NR_ISOLATED_[ANON|FILE] mismatch

PATCH Minchan Kim 3 participants 3 messages Sep 19, 2012 – Sep 20, 2012 Memory management

Share & tools

Minchan Kim

author

2012-09-19 23:49 UTC #0 lore raw

Quoted 66 lines · Johannes Weiner

Quoted 4 lines · KOSAKI Motohiro
Quoted 3 lines · Minchan Kim
When I looked at zone stat mismatch problem, I found migrate_to_node doesn't decrease NR_ISOLATED_[ANON|FILE] if check_range fails.
This is a bit misleading. It's not that the stats would be inaccurate, it's that the pages would be leaked from the LRU, no?
Quoted 1 line
Quoted 1 line
It can make system hang out.
Did you spot this by code review only or did you actually run into this? Because...
Quoted 31 lines
Quoted 31 lines
Cc KOSAKI Motohiro <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Christoph Lameter <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 mm/mempolicy.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
mm/mempolicy.c +5 -8
index 3d64b36..6bf0860 100644
@@ -953,16 +953,16 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
 
        vma = check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
                        flags | MPOL_MF_DISCONTIG_OK, &pagelist);
-       if (IS_ERR(vma))
-               return PTR_ERR(vma);
-
-       if (!list_empty(&pagelist)) {
+       if (IS_ERR(vma)) {
+               err = PTR_ERR(vma);
+               goto out;
+       }
+       if (!list_empty(&pagelist))
                err = migrate_pages(&pagelist, new_node_page, dest,
                                                        false, MIGRATE_SYNC);
-               if (err)
-                       putback_lru_pages(&pagelist);
-       }
-
Quoted 8 lines
mm/mempolicy.c static int migrate_to_node(struct mm_struct *mm, int source, int dest, · line 965
Quoted 3 lines
mm/mempolicy.c +3 -0
+out:
+       if (err)
+               putback_lru_pages(&pagelist);
Good catch!
This is a regression since following commit. So, I doubt we need all or nothing semantics. Can we revert it instead? (and probably we need more kind comment for preventing an accident)
I think it makes sense to revert. Not because of the semantics, but I just don't see how check_range() could even fail for this callsite:
1. we pass mm->mmap->vm_start in there, so we should not fail due to find_vma()
2. we pass MPOL_MF_DISCONTIG_OK, so the discontig checks do not apply and so can not fail
3. we pass MPOL_MF_MOVE | MPOL_MF_MOVE_ALL, the page table loops will continue until addr == end, so we never fail with -EIO

Quoted 9 lines

Quoted 5 lines
commit 0def08e3acc2c9c934e4671487029aed52202d42
Author: Vasiliy Kulikov <[email protected]>
Date:   Tue Oct 26 14:21:32 2010 -0700
mm/mempolicy.c: check return code of check_range
We don't use this code to "check" the range, we use it to collect migrate pages. There is no failure case.

Here it goes.

KOSAKI Motohiro

Acked-by

2012-09-20 12:36 UTC #1 lore raw

Quoted 132 lines · Minchan Kim

Quoted 66 lines · Johannes Weiner
Quoted 4 lines · KOSAKI Motohiro
Quoted 3 lines · Minchan Kim
When I looked at zone stat mismatch problem, I found migrate_to_node doesn't decrease NR_ISOLATED_[ANON|FILE] if check_range fails.
This is a bit misleading. It's not that the stats would be inaccurate, it's that the pages would be leaked from the LRU, no?
Quoted 1 line
Quoted 1 line
It can make system hang out.
Did you spot this by code review only or did you actually run into this? Because...
Quoted 31 lines
Quoted 31 lines
Cc KOSAKI Motohiro <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Christoph Lameter <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 mm/mempolicy.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
mm/mempolicy.c +5 -8
index 3d64b36..6bf0860 100644
@@ -953,16 +953,16 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
 
        vma = check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
                        flags | MPOL_MF_DISCONTIG_OK, &pagelist);
-       if (IS_ERR(vma))
-               return PTR_ERR(vma);
-
-       if (!list_empty(&pagelist)) {
+       if (IS_ERR(vma)) {
+               err = PTR_ERR(vma);
+               goto out;
+       }
+       if (!list_empty(&pagelist))
                err = migrate_pages(&pagelist, new_node_page, dest,
                                                        false, MIGRATE_SYNC);
-               if (err)
-                       putback_lru_pages(&pagelist);
-       }
-
Quoted 8 lines
mm/mempolicy.c static int migrate_to_node(struct mm_struct *mm, int source, int dest, · line 965
Quoted 3 lines
mm/mempolicy.c +3 -0
+out:
+       if (err)
+               putback_lru_pages(&pagelist);
Good catch!
This is a regression since following commit. So, I doubt we need all or nothing semantics. Can we revert it instead? (and probably we need more kind comment for preventing an accident)
I think it makes sense to revert. Not because of the semantics, but I just don't see how check_range() could even fail for this callsite:
1. we pass mm->mmap->vm_start in there, so we should not fail due to find_vma()
2. we pass MPOL_MF_DISCONTIG_OK, so the discontig checks do not apply and so can not fail
3. we pass MPOL_MF_MOVE | MPOL_MF_MOVE_ALL, the page table loops will continue until addr == end, so we never fail with -EIO
Quoted 9 lines
Quoted 5 lines
commit 0def08e3acc2c9c934e4671487029aed52202d42
Author: Vasiliy Kulikov <[email protected]>
Date:   Tue Oct 26 14:21:32 2010 -0700
mm/mempolicy.c: check return code of check_range
We don't use this code to "check" the range, we use it to collect migrate pages. There is no failure case.
Here it goes.
From c2c21b551811e034eb0ede6806e0314b201d7e5b Mon Sep 17 00:00:00 2001
From: Minchan Kim <[email protected]>
Date: Thu, 20 Sep 2012 08:39:52 +0900
Subject: [PATCH] mm: revert 0def08e3, mm/mempolicy.c: check return code of
check_range
This patch reverts 0def08e3 because check_range can't fail in migrate_to_node with considering current usecases.
Quote from Johannes
"
I think it makes sense to revert. Not because of the semantics, but I just don't see how check_range() could even fail for this callsite:
1. we pass mm->mmap->vm_start in there, so we should not fail due to find_vma()
2. we pass MPOL_MF_DISCONTIG_OK, so the discontig checks do not apply and so can not fail
3. we pass MPOL_MF_MOVE | MPOL_MF_MOVE_ALL, the page table loops will continue until addr == end, so we never fail with -EIO
"
And I add new VM_BUG_ON for checking migrate_to_node's future usecase which might pass to MPOL_MF_STRICT.
Cc KOSAKI Motohiro <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Christoph Lameter <[email protected]>
Cc David Rientjes <[email protected]>
Cc Vasiliy Kulikov <[email protected]>
Suggested-by Johannes Weiner <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 mm/mempolicy.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
mm/mempolicy.c +1 -2
index 3d64b36..9ec87bd 100644
@@ -946,15 +946,16 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
        nodemask_t nmask;
        LIST_HEAD(pagelist);
        int err = 0;
-       struct vm_area_struct *vma;
 
        nodes_clear(nmask);
        node_set(source, nmask);
 
-       vma = check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
+       /*

mm/mempolicy.c static int migrate_to_node(struct mm_struct *mm, int source, int dest, · line 961

Quoted 10 lines

mm/mempolicy.c +4 -2

+        * Collect migrate pages and it shoudn't be failed.
+        */
+       VM_BUG_ON(flags & MPOL_MF_STRICT);
+       check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
                        flags | MPOL_MF_DISCONTIG_OK, &pagelist);
-       if (IS_ERR(vma))
-               return PTR_ERR(vma);
 
        if (!list_empty(&pagelist)) {
                err = migrate_pages(&pagelist, new_node_page, dest,

Looks good. thank you.

Acked-by KOSAKI Motohiro <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Johannes Weiner

2012-09-20 15:41 UTC #2 lore raw

In reply to Minchan Kim

Quoted 45 lines · Minchan Kim

From: Minchan Kim <[email protected]>
Date: Thu, 20 Sep 2012 08:39:52 +0900
Subject: [PATCH] mm: revert 0def08e3, mm/mempolicy.c: check return code of
check_range
This patch reverts 0def08e3 because check_range can't fail in migrate_to_node with considering current usecases.
Quote from Johannes
"
I think it makes sense to revert. Not because of the semantics, but I just don't see how check_range() could even fail for this callsite:
1. we pass mm->mmap->vm_start in there, so we should not fail due to find_vma()
2. we pass MPOL_MF_DISCONTIG_OK, so the discontig checks do not apply and so can not fail
3. we pass MPOL_MF_MOVE | MPOL_MF_MOVE_ALL, the page table loops will continue until addr == end, so we never fail with -EIO
"
And I add new VM_BUG_ON for checking migrate_to_node's future usecase which might pass to MPOL_MF_STRICT.
Cc KOSAKI Motohiro <[email protected]>
Cc Mel Gorman <[email protected]>
Cc Christoph Lameter <[email protected]>
Cc David Rientjes <[email protected]>
Cc Vasiliy Kulikov <[email protected]>
Suggested-by Johannes Weiner <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 mm/mempolicy.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
mm/mempolicy.c +0 -0
index 3d64b36..9ec87bd 100644
@@ -946,15 +946,16 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
 	nodemask_t nmask;
 	LIST_HEAD(pagelist);
 	int err = 0;

mm/mempolicy.c static int migrate_to_node(struct mm_struct *mm, int source, int dest, · line 956

Quoted 10 lines

mm/mempolicy.c +4 -2

-	struct vm_area_struct *vma;
 
 	nodes_clear(nmask);
 	node_set(source, nmask);
 
-	vma = check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
+	/*
+	 * Collect migrate pages and it shoudn't be failed.
+	 */
+	VM_BUG_ON(flags & MPOL_MF_STRICT);

Adding a check and a comment is a good idea, but I'm not a big fan of checking for MPOL_MF_STRICT in particular because it's one of the invalid inputs, and so you need to extend this check when somebody extends the spectrum of invalid inputs. I would much prefer checking directly for !(flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) instead, which would also make the possible inputs apparent without having to chase up the call chain to find out what is usually passed in.

And how about

/*
* This does not "check" the range but isolates all pages that
* need migration. Between passing in the full user address
* space range and MPOL_MF_DISCONTIG_OK, this call can not fail. */

Quoted 13 lines

mm/mempolicy.c +1 -2

+	check_range(mm, mm->mmap->vm_start, mm->task_size, &nmask,
 			flags | MPOL_MF_DISCONTIG_OK, &pagelist);
-	if (IS_ERR(vma))
-		return PTR_ERR(vma);
 
 	if (!list_empty(&pagelist)) {
 		err = migrate_pages(&pagelist, new_node_page, dest,

Signature · 5 lines

1.7.9.5

-- 
Kind regards,
Minchan Kim