From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx170.postini.com [74.125.245.170]) by kanga.kvack.org (Postfix) with SMTP id 9203D6B0100 for ; Wed, 12 Sep 2012 17:59:03 -0400 (EDT) Date: Wed, 12 Sep 2012 14:59:02 -0700 From: Andrew Morton Subject: Re: [patch 2/2 v2]compaction: check lock contention first before taking lock Message-Id: <20120912145902.96c26c25.akpm@linux-foundation.org> In-Reply-To: <20120912105535.GM11266@suse.de> References: <20120910011850.GD3715@kernel.org> <20120911164330.4fffee4f.akpm@linux-foundation.org> <20120912105535.GM11266@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Mel Gorman Cc: Shaohua Li , linux-mm@kvack.org, aarcange@redhat.com On Wed, 12 Sep 2012 11:55:35 +0100 Mel Gorman wrote: > On Tue, Sep 11, 2012 at 04:43:30PM -0700, Andrew Morton wrote: > > On Mon, 10 Sep 2012 09:18:50 +0800 > > Shaohua Li wrote: > > > > > isolate_migratepages_range will take zone->lru_lock first and check if the lock > > > is contented, if yes, it will release the lock. This isn't efficient. If the > > > lock is truly contented, a lock/unlock pair will increase the lock contention. > > > We'd better check if the lock is contended first. compact_trylock_irqsave > > > perfectly meets the requirement. > > > > > > V2: > > > leave cond_resched() pointed out by Mel. > > > > > > Signed-off-by: Shaohua Li > > > --- > > > mm/compaction.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > Index: linux/mm/compaction.c > > > =================================================================== > > > --- linux.orig/mm/compaction.c 2012-09-10 08:49:40.377869710 +0800 > > > +++ linux/mm/compaction.c 2012-09-10 08:53:10.295230575 +0800 > > > @@ -295,8 +295,9 @@ isolate_migratepages_range(struct zone * > > > > > > /* Time to isolate some pages for migration */ > > > cond_resched(); > > > - spin_lock_irqsave(&zone->lru_lock, flags); > > > - locked = true; > > > + locked = compact_trylock_irqsave(&zone->lru_lock, &flags, cc); > > > + if (!locked) > > > + return 0; > > > for (; low_pfn < end_pfn; low_pfn++) { > > > struct page *page; > > > > Geeze that compact_checklock_irqsave stuff is naaaasty. > > > > The intention is to avoid THP allocations getting stuck in compaction.c > for ages due to spinlock contention. It's always better for those to > fail quickly. If compact_trylock_irqsave is improved it must still be > able to do this. So there's an implicit two-level prioritization here. But between what and what? It all sounds a bit hack/bandaidy? > > > There is no relationship between the concepts "user > > pressed ^C" and "this device driver or subsystem wants a high-order > > allocation". > > > > hmm, I see your point. The fatal signal check is "hidden" but this was to > preseve the existing behaviour prior to commit [c67fe375: mm: compaction: > Abort async compaction if locks are contended or taking too long]. The > fatal_signal_check could be deleted from compact_trylock_irqsave() but > then it should be checked in the isolate_migratepages_range() at the > very least. How about this? hm, well, actually, I chose ^C as an example of something which might set need_resched(). How about `There is no relationship between the concepts "this process exceeded its timeslice" and "this device driver or subsystem wants a high-order allocation"'. -- 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