From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755477AbaFKLlo (ORCPT ); Wed, 11 Jun 2014 07:41:44 -0400 Received: from cantor2.suse.de ([195.135.220.15]:53319 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751049AbaFKLln (ORCPT ); Wed, 11 Jun 2014 07:41:43 -0400 Message-ID: <53984074.8010000@suse.cz> Date: Wed, 11 Jun 2014 13:41:40 +0200 From: Vlastimil Babka User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Joonsoo Kim , Minchan Kim CC: David Rientjes , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Greg Thelen , Mel Gorman , Michal Nazarewicz , Naoya Horiguchi , Christoph Lameter , Rik van Riel Subject: Re: [PATCH 05/10] mm, compaction: remember position within pageblock in free pages scanner References: <1402305982-6928-1-git-send-email-vbabka@suse.cz> <1402305982-6928-5-git-send-email-vbabka@suse.cz> <20140611021213.GF15630@bbox> <20140611081606.GB28258@js1304-P5Q-DELUXE> In-Reply-To: <20140611081606.GB28258@js1304-P5Q-DELUXE> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/11/2014 10:16 AM, Joonsoo Kim wrote: > On Wed, Jun 11, 2014 at 11:12:13AM +0900, Minchan Kim wrote: >> On Mon, Jun 09, 2014 at 11:26:17AM +0200, Vlastimil Babka wrote: >>> Unlike the migration scanner, the free scanner remembers the beginning of the >>> last scanned pageblock in cc->free_pfn. It might be therefore rescanning pages >>> uselessly when called several times during single compaction. This might have >>> been useful when pages were returned to the buddy allocator after a failed >>> migration, but this is no longer the case. >>> >>> This patch changes the meaning of cc->free_pfn so that if it points to a >>> middle of a pageblock, that pageblock is scanned only from cc->free_pfn to the >>> end. isolate_freepages_block() will record the pfn of the last page it looked >>> at, which is then used to update cc->free_pfn. >>> >>> In the mmtests stress-highalloc benchmark, this has resulted in lowering the >>> ratio between pages scanned by both scanners, from 2.5 free pages per migrate >>> page, to 2.25 free pages per migrate page, without affecting success rates. >>> >>> Signed-off-by: Vlastimil Babka >> Reviewed-by: Minchan Kim >> >> Below is a nitpick. >> >>> Cc: Minchan Kim >>> Cc: Mel Gorman >>> Cc: Joonsoo Kim >>> Cc: Michal Nazarewicz >>> Cc: Naoya Horiguchi >>> Cc: Christoph Lameter >>> Cc: Rik van Riel >>> Cc: David Rientjes >>> --- >>> mm/compaction.c | 33 ++++++++++++++++++++++++++++----- >>> 1 file changed, 28 insertions(+), 5 deletions(-) >>> >>> diff --git a/mm/compaction.c b/mm/compaction.c >>> index 83f72bd..58dfaaa 100644 >>> --- a/mm/compaction.c >>> +++ b/mm/compaction.c >>> @@ -297,7 +297,7 @@ static bool suitable_migration_target(struct page *page) >>> * (even though it may still end up isolating some pages). >>> */ >>> static unsigned long isolate_freepages_block(struct compact_control *cc, >>> - unsigned long blockpfn, >>> + unsigned long *start_pfn, >>> unsigned long end_pfn, >>> struct list_head *freelist, >>> bool strict) >>> @@ -306,6 +306,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, >>> struct page *cursor, *valid_page = NULL; >>> unsigned long flags; >>> bool locked = false; >>> + unsigned long blockpfn = *start_pfn; >>> >>> cursor = pfn_to_page(blockpfn); >>> >>> @@ -314,6 +315,9 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, >>> int isolated, i; >>> struct page *page = cursor; >>> >>> + /* Record how far we have got within the block */ >>> + *start_pfn = blockpfn; >>> + >> >> Couldn't we move this out of the loop for just one store? Ah, I get it now. Ignore my previous reply. > Hello, Vlastimil. > > Moreover, start_pfn can't be updated to end pfn with this approach. > Is it okay? That's intentional, as end_pfn means the scanner would restart at the beginning of next pageblock. So I want to record last pfn *inside* the pageblock that was fully scanned. Note that there's a high change that fully scanning pageblock means that I haven't isolated enough and isolate_freepages() will advance to the previous pageblock anyway, and the recorded value will be overwritten. But still it's better to prevent this corner case. So outside the loop, I would need to do: *start_pfn = max(blockpfn, end_pfn - 1); It looks a bit tricky but probably better than multiple assignments. Thanks. > Thanks. > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f172.google.com (mail-we0-f172.google.com [74.125.82.172]) by kanga.kvack.org (Postfix) with ESMTP id 08AF36B014F for ; Wed, 11 Jun 2014 07:41:43 -0400 (EDT) Received: by mail-we0-f172.google.com with SMTP id u57so479871wes.17 for ; Wed, 11 Jun 2014 04:41:43 -0700 (PDT) Received: from mx2.suse.de (cantor2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id hj5si21505994wib.52.2014.06.11.04.41.41 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 11 Jun 2014 04:41:42 -0700 (PDT) Message-ID: <53984074.8010000@suse.cz> Date: Wed, 11 Jun 2014 13:41:40 +0200 From: Vlastimil Babka MIME-Version: 1.0 Subject: Re: [PATCH 05/10] mm, compaction: remember position within pageblock in free pages scanner References: <1402305982-6928-1-git-send-email-vbabka@suse.cz> <1402305982-6928-5-git-send-email-vbabka@suse.cz> <20140611021213.GF15630@bbox> <20140611081606.GB28258@js1304-P5Q-DELUXE> In-Reply-To: <20140611081606.GB28258@js1304-P5Q-DELUXE> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Joonsoo Kim , Minchan Kim Cc: David Rientjes , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Greg Thelen , Mel Gorman , Michal Nazarewicz , Naoya Horiguchi , Christoph Lameter , Rik van Riel On 06/11/2014 10:16 AM, Joonsoo Kim wrote: > On Wed, Jun 11, 2014 at 11:12:13AM +0900, Minchan Kim wrote: >> On Mon, Jun 09, 2014 at 11:26:17AM +0200, Vlastimil Babka wrote: >>> Unlike the migration scanner, the free scanner remembers the beginning of the >>> last scanned pageblock in cc->free_pfn. It might be therefore rescanning pages >>> uselessly when called several times during single compaction. This might have >>> been useful when pages were returned to the buddy allocator after a failed >>> migration, but this is no longer the case. >>> >>> This patch changes the meaning of cc->free_pfn so that if it points to a >>> middle of a pageblock, that pageblock is scanned only from cc->free_pfn to the >>> end. isolate_freepages_block() will record the pfn of the last page it looked >>> at, which is then used to update cc->free_pfn. >>> >>> In the mmtests stress-highalloc benchmark, this has resulted in lowering the >>> ratio between pages scanned by both scanners, from 2.5 free pages per migrate >>> page, to 2.25 free pages per migrate page, without affecting success rates. >>> >>> Signed-off-by: Vlastimil Babka >> Reviewed-by: Minchan Kim >> >> Below is a nitpick. >> >>> Cc: Minchan Kim >>> Cc: Mel Gorman >>> Cc: Joonsoo Kim >>> Cc: Michal Nazarewicz >>> Cc: Naoya Horiguchi >>> Cc: Christoph Lameter >>> Cc: Rik van Riel >>> Cc: David Rientjes >>> --- >>> mm/compaction.c | 33 ++++++++++++++++++++++++++++----- >>> 1 file changed, 28 insertions(+), 5 deletions(-) >>> >>> diff --git a/mm/compaction.c b/mm/compaction.c >>> index 83f72bd..58dfaaa 100644 >>> --- a/mm/compaction.c >>> +++ b/mm/compaction.c >>> @@ -297,7 +297,7 @@ static bool suitable_migration_target(struct page *page) >>> * (even though it may still end up isolating some pages). >>> */ >>> static unsigned long isolate_freepages_block(struct compact_control *cc, >>> - unsigned long blockpfn, >>> + unsigned long *start_pfn, >>> unsigned long end_pfn, >>> struct list_head *freelist, >>> bool strict) >>> @@ -306,6 +306,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, >>> struct page *cursor, *valid_page = NULL; >>> unsigned long flags; >>> bool locked = false; >>> + unsigned long blockpfn = *start_pfn; >>> >>> cursor = pfn_to_page(blockpfn); >>> >>> @@ -314,6 +315,9 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, >>> int isolated, i; >>> struct page *page = cursor; >>> >>> + /* Record how far we have got within the block */ >>> + *start_pfn = blockpfn; >>> + >> >> Couldn't we move this out of the loop for just one store? Ah, I get it now. Ignore my previous reply. > Hello, Vlastimil. > > Moreover, start_pfn can't be updated to end pfn with this approach. > Is it okay? That's intentional, as end_pfn means the scanner would restart at the beginning of next pageblock. So I want to record last pfn *inside* the pageblock that was fully scanned. Note that there's a high change that fully scanning pageblock means that I haven't isolated enough and isolate_freepages() will advance to the previous pageblock anyway, and the recorded value will be overwritten. But still it's better to prevent this corner case. So outside the loop, I would need to do: *start_pfn = max(blockpfn, end_pfn - 1); It looks a bit tricky but probably better than multiple assignments. Thanks. > Thanks. > -- 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