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

[Shaohua Li <shli@kernel.org>]

On Thu, Sep 13, 2012 at 09:47:22AM +0900, Minchan Kim wrote:
> 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?

Thanks for catching this issue. Both looks sane, but I would vote for Andrea's.
If some pages are isolated, better we use them.

Thanks,
Shaohua

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