From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx147.postini.com [74.125.245.147]) by kanga.kvack.org (Postfix) with SMTP id EB4556B00FB for ; Wed, 12 Sep 2012 17:20:20 -0400 (EDT) Date: Wed, 12 Sep 2012 14:20:19 -0700 From: Andrew Morton Subject: Re: [patch 1/2 v2]compaction: abort compaction loop if lock is contended or run too long Message-Id: <20120912142019.0e06bf52.akpm@linux-foundation.org> In-Reply-To: <20120912004840.GI27078@redhat.com> References: <20120910011830.GC3715@kernel.org> <20120911163455.bb249a3c.akpm@linux-foundation.org> <20120912004840.GI27078@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Andrea Arcangeli Cc: Shaohua Li , linux-mm@kvack.org, mgorman@suse.de On Wed, 12 Sep 2012 02:48:40 +0200 Andrea Arcangeli wrote: > On Tue, Sep 11, 2012 at 04:34:55PM -0700, Andrew Morton wrote: > > On Mon, 10 Sep 2012 09:18:30 +0800 > > Shaohua Li wrote: > > > > > isolate_migratepages_range() might isolate none pages, for example, when > > > zone->lru_lock is contended and compaction is async. In this case, we should > > > abort compaction, otherwise, compact_zone will run a useless loop and make > > > zone->lru_lock is even contended. > > > > > > ... > > > > > > @@ -838,12 +838,14 @@ static unsigned long compact_zone_order( > > > .migratetype = allocflags_to_migratetype(gfp_mask), > > > .zone = zone, > > > .sync = sync, > > > - .contended = contended, > > > }; > > > INIT_LIST_HEAD(&cc.freepages); > > > INIT_LIST_HEAD(&cc.migratepages); > > > > > > - return compact_zone(zone, &cc); > > > + ret = compact_zone(zone, &cc); > > > + if (contended) > > > + *contended = cc.contended; > > > + return ret; > > > } > > > > > > > From a quick read, `contended' is never NULL here. And defining the > > "contended" pointer can be null with some caller so the if is > needed. The inner code was checking it before. This is also why we > couldn't use *contended for the loop break bugfix, because contended > could have been null at times. Confused. I can see only two call sites: __alloc_pages_slowpath ->__alloc_pages_direct_compact ->try_to_compact_pages ->compact_zone_order and in both cases, `contended' points at valid storage. > Now cc.contended is always available so > we can use that to fix the bug and it's self documenting as well. > > > interface so that `contended' must be a valid pointer is a good change, > > IMO - it results in simpler and faster code. > > Agreed. 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; } > > Alas, try_to_compact_pages()'s kerneldoc altogether forgets to describe > > this argument. Mel's > > mm-compaction-capture-a-suitable-high-order-page-immediately-when-it-is-made-available.patch > > adds a `pages' arg and forgets to document that as well. poke poke -- 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: email@kvack.org