Re: [patch 1/2 v2]compaction: abort compaction loop if lock is contended or run too long

[Minchan Kim <minchan@kernel.org>]

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@redhat.com>
> 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@redhat.com>
> ---
>  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@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>

-- 
Kind regards,
Minchan Kim

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