mm: fix NR_ISOLATED_[ANON|FILE] mismatch

PATCH In review Minchan Kim 3 participants 4 messages Sep 19, 2012

Subsystems (2)

Memory management MM NUMA placement

lore

Share & tools

lore

Minchan Kim

author

2012-09-19 07:43 UTC #0 lore raw

PATCH 1/1 mm: fix NR_ISOLATED_[ANON|FILE] mismatch

When I looked at zone stat mismatch problem, I found migrate_to_node doesn't decrease NR_ISOLATED_[ANON|FILE] if check_range fails.

It can make system hang out.

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 +8 -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);
-	}
-
+out:
+	if (err)
+		putback_lru_pages(&pagelist);
 	return err;
 }

Signature · 7 lines

1.7.9.5

--
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>

KOSAKI Motohiro

2012-09-19 17:05 UTC #1 lore raw

Quoted 30 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.
It can make system hang out.
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 +4 -4
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;
+       }

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

Quoted 10 lines

mm/mempolicy.c +4 -4

+       if (!list_empty(&pagelist))
                err = migrate_pages(&pagelist, new_node_page, dest,
                                                        false, MIGRATE_SYNC);
-               if (err)
-                       putback_lru_pages(&pagelist);
-       }
-
+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)

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

--
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-19 18:28 UTC #2 lore raw

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

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 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.

Minchan Kim

author

2012-09-19 20:38 UTC #3 lore raw

Hi Hannes,

Quoted 6 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.

Quoted 7 lines

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...

Just by code review during the bug of memory-hotplug.

Quoted 53 lines

Quoted 32 lines
Quoted 32 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 8 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.

Right. I will send revert patch when I go to office. Thanks for the pointing out.

Signature · 8 lines

Kind Regards,
Minchan Kim

--
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>