Hi Andrea,
On Thu, Sep 13, 2012 at 01:48:08AM +0200, Andrea Arcangeli wrote:
> On Wed, Sep 12, 2012 at 02:20:19PM -0700, Andrew Morton wrote:
> > OK, I'll slip this in there:
> >
> > --- a/mm/compaction.c~mm-compaction-abort-compaction-loop-if-lock-is-contended-or-run-too-long-fix
> > +++ a/mm/compaction.c
> > @@ -909,8 +909,7 @@ static unsigned long compact_zone_order(
> > INIT_LIST_HEAD(&cc.migratepages);
> >
> > ret = compact_zone(zone, &cc);
> > - if (contended)
> > - *contended = cc.contended;
> > + *contended = cc.contended;
> > return ret;
> > }
>
> Ack the above, thanks.
>
> One more thing, today a bug tripped while building cyanogenmod10 (it
> swaps despite so much ram) after I added the cc->contended loop break
> patch. The original version of the fix from Shaohua didn't have this
> problem because it would only abort compaction if the low_pfn didn't
> advance and in turn the list would be guaranteed empty.
Nice catch!
>
> Verifying the list is empty before aborting compaction (which takes a
> path that ignores the cc->migratelist) should be enough to fix it and
> it makes it really equivalent to the previous fix. Both cachelines
> should be cache hot so it should be practically zero cost to check it.
>
> Only lightly tested so far.
>
> ===
> >From b2a50e49d65596d3920773316ad9b7dd54e4acaf Mon Sep 17 00:00:00 2001
> From: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> Date: Thu, 13 Sep 2012 01:22:03 +0200
> Subject: [PATCH] mm: compaction: fix leak in cc->contended loop breaking
> logic
>
> We cannot return ISOLATE_ABORT when cc->contended is true, if we have
> some pages already successfully isolated in the cc->migratepages
> list, or they will be leaked.
>
> The bug was highlighted by a nice VM_BUG_ON in the async compaction in
> kswapd. So I also added the symmetric VM_BUG_ON to the other caller of
> the function considering it looks a worthwhile VM_BUG_ON.
Fair enough.
>
> ------------[ cut here ]------------
> kernel BUG at mm/compaction.c:934!
> invalid opcode: 0000 [#1] SMP
> Modules linked in: tun usbhid kvm_intel xhci_hcd kvm snd_hda_codec_realtek ehci_hcd usbcore snd_hda_intel sn
> er crc32c_intel psmouse ghash_clmulni_intel sr_mod snd sg cdrom snd_page_alloc usb_common pcspkr [last unloa
>
> CPU 0
> Pid: 513, comm: kswapd0 Not tainted 3.6.0-rc4+ #17 /DH61BE
> RIP: 0010:[<ffffffff8111302c>] [<ffffffff8111302c>] __compact_pgdat+0x1ac/0x1b0
> RSP: 0018:ffff880216fa5cb0 EFLAGS: 00010283
> RAX: 0000000000000003 RBX: ffff880216fa5d00 RCX: 0000000000000002
> RDX: 00000000000008d7 RSI: 0000000000000002 RDI: ffffffff8195b058
> RBP: ffffffff8195b000 R08: 0000000000000be4 R09: ffffffff8195a9c0
> R10: ffffffff8195b400 R11: ffffffff8195b570 R12: 0000000000000001
> R13: 0000000000000001 R14: ffff880216fa5d10 R15: 0000000000000003
> FS: 0000000000000000(0000) GS:ffff88021fa00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 00007f14d4167000 CR3: 00000000018f1000 CR4: 00000000000407f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process kswapd0 (pid: 513, threadinfo ffff880216fa4000, task ffff880216cfef20)
> Stack:
> ffffffff8195a9c0 ffffffff8195b000 0000000000000320 0000000000000003
> ffffffff8195a9c0 ffffffff8195b640 0000000000000002 0000000000000c80
> 0000000000000001 ffffffff811132f3 ffff880216fa5d00 ffff880216fa5d00
> Call Trace:
> [<ffffffff811132f3>] ? compact_pgdat+0x23/0x30
> [<ffffffff8110503f>] ? kswapd+0x89f/0xac0
> [<ffffffff8106f450>] ? wake_up_bit+0x40/0x40
> [<ffffffff811047a0>] ? shrink_lruvec+0x510/0x510
> [<ffffffff811047a0>] ? shrink_lruvec+0x510/0x510
> [<ffffffff8106ef1e>] ? kthread+0x9e/0xb0
>
> Signed-off-by: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> ---
> mm/compaction.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 6066a35..0292984 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -633,7 +633,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
>
> /* Perform the isolation */
> low_pfn = isolate_migratepages_range(zone, cc, low_pfn, end_pfn);
> - if (!low_pfn || cc->contended)
> + if (!low_pfn || (cc->contended && !cc->nr_migratepages))
> return ISOLATE_ABORT;
I'm not sure it's best.
As you mentioned, it's same with first version of Shaohua.
But it could mitigate the goal of the patch if lock contention or
need_resched happens in the middle of loop once we isolate a
migratable page.
What do you think about this?
diff --git a/mm/compaction.c b/mm/compaction.c
index 0fbc6b7..7a009dd 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -848,6 +848,10 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
switch (isolate_migratepages(zone, cc)) {
case ISOLATE_ABORT:
ret = COMPACT_PARTIAL;
+ if (!list_empty(&cc->migratepages)) {
+ putback_lru_pages(&cc->migratepages);
+ cc->nr_migratepages = 0;
+ }
goto out;
case ISOLATE_NONE:
continue;
>
> cc->migrate_pfn = low_pfn;
> @@ -843,6 +843,10 @@ static unsigned long compact_zone_order(struct zone *zone,
> INIT_LIST_HEAD(&cc.migratepages);
>
> ret = compact_zone(zone, &cc);
> +
> + VM_BUG_ON(!list_empty(&cc.freepages));
> + VM_BUG_ON(!list_empty(&cc.migratepages));
> +
> *contended = cc.contended;
> return ret;
> }
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>