Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753384Ab1B1XBk (ORCPT ); Mon, 28 Feb 2011 18:01:40 -0500 Received: from mail-px0-f174.google.com ([209.85.212.174]:39682 "EHLO mail-px0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752667Ab1B1XBj (ORCPT ); Mon, 28 Feb 2011 18:01:39 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=MXFZw63X9sqTZtZgOSIcx0qfm0SrG2yhGU4bhHsf+oPL7jgq5Osg82tUmQXTYZksgU LzB1h8gs53TYdYIswCIKvZIq84Rcre0m7TKsXYh+1b8QT5iEJrn9OrnmInERFNWeKw47 KxVmV+r91On1SmZag8p4Y7E93y7HDINWJZCYU= Date: Tue, 1 Mar 2011 08:01:31 +0900 From: Minchan Kim To: Mel Gorman Cc: Andrew Morton , Arthur Marsh , Clemens Ladisch , Andrea Arcangeli , Linux-MM , Linux Kernel Mailing List Subject: Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabled while isolating pages for migration Message-ID: <20110228230131.GB1896@barrios-desktop> References: <1298664299-10270-1-git-send-email-mel@csn.ul.ie> <1298664299-10270-3-git-send-email-mel@csn.ul.ie> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1298664299-10270-3-git-send-email-mel@csn.ul.ie> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4637 Lines: 106 On Fri, Feb 25, 2011 at 08:04:59PM +0000, Mel Gorman wrote: > From: Andrea Arcangeli > > compaction_alloc() isolates pages for migration in isolate_migratepages. While > it's scanning, IRQs are disabled on the mistaken assumption the scanning > should be short. Tests show this to be true for the most part but > contention times on the LRU lock can be increased. Before this patch, > the IRQ disabled times for a simple test looked like > > Total sampled time IRQs off (not real total time): 5493 > Event shrink_inactive_list..shrink_zone 1596 us count 1 > Event shrink_inactive_list..shrink_zone 1530 us count 1 > Event shrink_inactive_list..shrink_zone 956 us count 1 > Event shrink_inactive_list..shrink_zone 541 us count 1 > Event shrink_inactive_list..shrink_zone 531 us count 1 > Event split_huge_page..add_to_swap 232 us count 1 > Event save_args..call_softirq 36 us count 1 > Event save_args..call_softirq 35 us count 2 > Event __wake_up..__wake_up 1 us count 1 > > This patch reduces the worst-case IRQs-disabled latencies by releasing the > lock every SWAP_CLUSTER_MAX pages that are scanned and releasing the CPU if > necessary. The cost of this is that the processing performing compaction will > be slower but IRQs being disabled for too long a time has worse consequences > as the following report shows; > > Total sampled time IRQs off (not real total time): 4367 > Event shrink_inactive_list..shrink_zone 881 us count 1 > Event shrink_inactive_list..shrink_zone 875 us count 1 > Event shrink_inactive_list..shrink_zone 868 us count 1 > Event shrink_inactive_list..shrink_zone 555 us count 1 > Event split_huge_page..add_to_swap 495 us count 1 > Event compact_zone..compact_zone_order 269 us count 1 > Event split_huge_page..add_to_swap 266 us count 1 > Event shrink_inactive_list..shrink_zone 85 us count 1 > Event save_args..call_softirq 36 us count 2 > Event __wake_up..__wake_up 1 us count 1 > > Signed-off-by: Andrea Arcangeli > Signed-off-by: Mel Gorman > --- > mm/compaction.c | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index 11d88a2..ec9eb0f 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -279,9 +279,27 @@ static unsigned long isolate_migratepages(struct zone *zone, > } > > /* Time to isolate some pages for migration */ > + cond_resched(); > spin_lock_irq(&zone->lru_lock); > for (; low_pfn < end_pfn; low_pfn++) { > struct page *page; > + bool unlocked = false; > + > + /* give a chance to irqs before checking need_resched() */ > + if (!((low_pfn+1) % SWAP_CLUSTER_MAX)) { > + spin_unlock_irq(&zone->lru_lock); > + unlocked = true; > + } > + if (need_resched() || spin_is_contended(&zone->lru_lock)) { I am not sure it's good if we release the lock whenever lru->lock was contended unconditionally? There are many kinds of lru_lock operations(add to lru, del from lru, isolation, reclaim, activation, deactivation and so on). Do we really need to release the lock whenever all such operations were contened? I think what we need is just spin_is_contended_irqcontext. Otherwise, please write down the comment for justifying for it. > + if (!unlocked) > + spin_unlock_irq(&zone->lru_lock); > + cond_resched(); > + spin_lock_irq(&zone->lru_lock); > + if (fatal_signal_pending(current)) > + break; This patch is for reducing for irq latency but do we have to check signal in irq hold time? > + } else if (unlocked) > + spin_lock_irq(&zone->lru_lock); > + > if (!pfn_valid_within(low_pfn)) > continue; > nr_scanned++; > -- > 1.7.2.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/