* Re: [3.7-rc6] capture_free_page() frees page without accounting for them??
2012-11-20 20:25 [3.7-rc6] capture_free_page() frees page without accounting for them?? Dave Hansen
@ 2012-11-21 0:48 ` Dave Hansen
2012-11-21 14:33 ` Mel Gorman
1 sibling, 0 replies; 3+ messages in thread
From: Dave Hansen @ 2012-11-21 0:48 UTC (permalink / raw)
To: linux-mm, Mel Gorman, Andrew Morton, LKML
[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]
I'm really evil, so I changed the loop in compact_capture_page() to
basically steal the highest-order page it can. This shouldn't _break_
anything, but it does ensure that we'll be splitting pages that we find
more often and recreating this *MUCH* faster:
- for (order = cc->order; order < MAX_ORDER; order++) {
+ for (order = MAX_ORDER - 1; order >= cc->order;order--)
I also augmented the area in capture_free_page() that I expect to be
leaking:
if (alloc_order != order) {
static int leaked_pages = 0;
leaked_pages += 1<<order;
leaked_pages -= 1<<alloc_order;
printk("%s() alloc_order(%d) != order(%d) leaked %d\n",
__func__, alloc_order, order,
leaked_pages);
expand(zone, page, alloc_order, order,
&zone->free_area[order], migratetype);
}
I add up all the fields in buddyinfo to figure out how much _should_ be
in the allocator and then compare it to MemFree to get a guess at how
much is leaked. That number correlates _really_ well with the
"leaked_pages" variable above. That pretty much seals it for me.
I'll run a stress test overnight to see if it pops up again. The patch
I'm running is attached. I'll send a properly changelogged one tomorrow
if it works.
[-- Attachment #2: leak-fix-20121120-1.patch --]
[-- Type: text/x-patch, Size: 666 bytes --]
---
linux-2.6.git-dave/mm/page_alloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN mm/page_alloc.c~leak-fix-20121120-1 mm/page_alloc.c
--- linux-2.6.git/mm/page_alloc.c~leak-fix-20121120-1 2012-11-20 19:44:09.588966346 -0500
+++ linux-2.6.git-dave/mm/page_alloc.c 2012-11-20 19:44:21.993057915 -0500
@@ -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,
_
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [3.7-rc6] capture_free_page() frees page without accounting for them??
2012-11-20 20:25 [3.7-rc6] capture_free_page() frees page without accounting for them?? Dave Hansen
2012-11-21 0:48 ` Dave Hansen
@ 2012-11-21 14:33 ` Mel Gorman
1 sibling, 0 replies; 3+ messages in thread
From: Mel Gorman @ 2012-11-21 14:33 UTC (permalink / raw)
To: Dave Hansen; +Cc: linux-mm, Andrew Morton, LKML
On Tue, Nov 20, 2012 at 12:25:37PM -0800, Dave Hansen wrote:
> Hi Mel,
>
> I'm chasing an apparent memory leak introduced post-3.6.
An accounting leak could also contribute to the kswapd bugs we've been
seeing recently.
Andrew, this is quite important and might be worth wedging in before 3.7
comes out because it'll cause serious problems if Dave is right.
> The
> interesting thing is that it appears that the pages are in the
> allocator, but not being accounted for:
>
> http://www.spinics.net/lists/linux-mm/msg46187.html
> https://bugzilla.kernel.org/show_bug.cgi?id=50181
>
Differences in the buddy allocator and reported free figures almost
always point to either per-cpu drift or NR_FREE_PAGES accounting bugs.
Usually the drift is not too bad and the drift is always within a
margin related to the number of CPUs. NR_FREE_PAGES accounting bugs get
progressively worse until the machine starts OOM killing or locks up.
> I started auditing anything that might be messing with NR_FREE_PAGES,
> and came across commit 1fb3f8ca.
It could certainly affect NR_FREE_PAGES due to its manipulating of buddy
pages. It will not result in happy and the system would potentially need
to be running a long time before it's spotted.
> 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'.
>
> I _think_ the correct fix is to just:
>
> - __mod_zone_page_state(zone, NR_FREE_PAGES, -(1UL << order));
> + __mod_zone_page_state(zone, NR_FREE_PAGES, -(1UL << alloc_order));
>
This looks correct to me but it will collide with other patches. You'll
need something like the below. If it works for you, stick a changelog on
it, feel free to put my Acked on it and get it to Andrew for ASAP
because I really think this needs to be in before 3.7 comes out or we'll
be swamped with a maze of kswapd-goes-mental bugs, all similar with
different root causes.
Thanks a million Dave!
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index fd6a073..ad99f0f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1406,7 +1406,7 @@ int capture_free_page(struct page *page, int alloc_order, int migratetype)
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 majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 3+ messages in thread