fix incorrect NR_FREE_PAGES accounting (appears like memory leak)

PATCH New Dave Hansen 2 participants 4 messages Nov 21, 2012 – Nov 27, 2012

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Dave Hansen

author

2012-11-21 19:21 UTC #0 lore raw

PATCH 1/1 fix incorrect NR_FREE_PAGES accounting (appears like memory leak)

This needs to make it in before 3.7 is released.

There have been some 3.7-rc reports of vm issues, including some kswapd bugs and, more importantly, some memory "leaks":

	http://www.spinics.net/lists/linux-mm/msg46187.html
	https://bugzilla.kernel.org/show_bug.cgi?id=50181

The post-3.6 commit 1fb3f8ca took split_free_page() and reused it for the compaction code. It does something curious with capture_free_page() (previously known as split_free_page()):

int capture_free_page(struct page *page, int alloc_order,
...

        __mod_zone_page_state(zone, NR_FREE_PAGES, -(1UL << order));

-       /* Split into individual pages */
-       set_page_refcounted(page);
-       split_page(page, order);
+       if (alloc_order != order)

+ expand(zone, page, alloc_order, order,
+ &zone->free_area[order], migratetype);

Note that expand() puts the pages _back_ in the allocator, but it does not bump NR_FREE_PAGES. We "return" 'alloc_order' worth of pages, but we accounted for removing 'order' in the __mod_zone_page_state() call. For the old split_page()-style use (order==alloc_order) the bug will not trigger. But, when called from the compaction code where we occasionally get a larger page out of the buddy allocator than we need, we will run in to this.

This patch simply changes the NR_FREE_PAGES manipulation to the correct 'alloc_order' instead of 'order'.

I've been able to repeatedly trigger this in my testing environment. The amount "leaked" very closely tracks the imbalance I see in buddy pages vs. NR_FREE_PAGES. I have confirmed that this patch fixes the imbalance

Signed-off-by Dave Hansen <[email protected]>

Acked-by Mel Gorman <[email protected]>

---

 linux-2.6.git-dave/mm/page_alloc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/page_alloc.c renamed from mm/page_alloc.c~leak-fix-20121120-2 +1 -1

diff -puN mm/page_alloc.c~leak-fix-20121120-2 mm/page_alloc.c
@@ -1405,7 +1405,7 @@ int capture_free_page(struct page *page,
 
 	mt = get_pageblock_migratetype(page);
 	if (unlikely(mt != MIGRATE_ISOLATE))
-		__mod_zone_freepage_state(zone, -(1UL << order), mt);
+		__mod_zone_freepage_state(zone, -(1UL << alloc_order), mt);
 
 	if (alloc_order != order)
 		expand(zone, page, alloc_order, order,

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

Mel Gorman

2012-11-26 11:23 UTC #1 lore raw

PATCH 1/1 mm: compaction: Fix return value of capture_free_page

Quoted 3 lines · Dave Hansen

This needs to make it in before 3.7 is released.

This is also required. Dave, can you double check? The surprise is that this does not blow up very obviously.

---8<---
From: Mel Gorman <[email protected]>
Subject: [PATCH] mm: compaction: Fix return value of capture_free_page

Commit ef6c5be6 (fix incorrect NR_FREE_PAGES accounting (appears like memory leak)) fixes a NR_FREE_PAGE accounting leak but missed the return value which was also missed by this reviewer until today. That return value is used by compaction when adding pages to a list of isolated free pages and without this follow-up fix, there is a risk of free list corruption.

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

---
 mm/page_alloc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/page_alloc.c +1 -1

index bcb72c6..8193809 100644
@@ -1422,7 +1422,7 @@ int capture_free_page(struct page *page, int alloc_order, int migratetype)
 		}
 	}
 
-	return 1UL << order;
+	return 1UL << alloc_order;
 }
 
 /*

Dave Hansen

author

2012-11-26 15:07 UTC #2 lore raw

Re: [PATCH] mm: compaction: Fix return value of capture_free_page

Quoted 7 lines · Mel Gorman

Quoted 3 lines · Dave Hansen
This needs to make it in before 3.7 is released.
This is also required. Dave, can you double check? The surprise is that this does not blow up very obviously.

...

mm/page_alloc.c int capture_free_page(struct page *page, int alloc_order, int migratetype) · line 1426

Quoted 7 lines

mm/page_alloc.c +1 -1

@@ -1422,7 +1422,7 @@ int capture_free_page(struct page *page, int alloc_order, int migratetype)
 		}
 	}
 
-	return 1UL << order;
+	return 1UL << alloc_order;
 }

compact_capture_page() only looks at the boolean return value out of capture_free_page(), so it wouldn't notice. split_free_page() does. But, when it calls capture_free_page(), order==alloc_order, so it wouldn't make a difference. So, there's probably no actual bug here, but it's certainly a wrong return value.

We should probably also fix the set_pageblock_migratetype() loop in there while we're at it. I think it's potentially trampling on the migration type of pages currently in the allocator. I _think_ that completes the list of things that need to get audited in there.

Mel Gorman

2012-11-27 11:07 UTC #3 lore raw

Re: [PATCH] mm: compaction: Fix return value of capture_free_page

Quoted 13 lines · Dave Hansen

Quoted 7 lines · Mel Gorman
Quoted 3 lines · Dave Hansen
This needs to make it in before 3.7 is released.
This is also required. Dave, can you double check? The surprise is that this does not blow up very obviously.
...
Quoted 4 lines
(quoted diff) +0 -0
@@ -1422,7 +1422,7 @@ int capture_free_page(struct page *page, int alloc_order, int migratetype)
 		}
 	}
 

Quoted 10 lines

mm/page_alloc.c int capture_free_page(struct page *page, int alloc_order, int migratetype) · line 1426
Quoted 3 lines
mm/page_alloc.c +1 -1
-	return 1UL << order;
+	return 1UL << alloc_order;
 }
compact_capture_page() only looks at the boolean return value out of capture_free_page(), so it wouldn't notice. split_free_page() does. But, when it calls capture_free_page(), order==alloc_order, so it wouldn't make a difference. So, there's probably no actual bug here, but it's certainly a wrong return value.

I don't think it is fine in this case.

isolate_freepages_block
isolated = split_free_page(page);
-> split_free_page

     nr_pages = capture_free_page(page, order, 0);
     -> capture_free_page (returns wrong value of too many pages)
     return nr_pages;

so now isolate_freepages_block has the wrong value with nr_pages holding a value for a larger number of pages than are really isolated and does this

                for (i = 0; i < isolated; i++) {
                        list_add(&page->lru, freelist);
                        page++;
                }

so potentially that is now adding pages that are already on the buddy list to the local free list and "fun" ensues.

Signature · 8 lines

Mel Gorman
SUSE Labs

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