From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx205.postini.com [74.125.245.205]) by kanga.kvack.org (Postfix) with SMTP id 356906B005A for ; Thu, 6 Sep 2012 04:16:43 -0400 (EDT) Date: Thu, 6 Sep 2012 17:18:18 +0900 From: Minchan Kim Subject: Re: [RFC v2] memory-hotplug: remove MIGRATE_ISOLATE from free_area->free_list Message-ID: <20120906081818.GC16231@bbox> References: <1346900018-14759-1-git-send-email-minchan@kernel.org> <50485B7B.3030201@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50485B7B.3030201@cn.fujitsu.com> Sender: owner-linux-mm@kvack.org List-ID: To: Lai Jiangshan Cc: Kamezawa Hiroyuki , Yasuaki Ishimatsu , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Michal Nazarewicz , Mel Gorman , Wen Congyang , Konrad Rzeszutek Wilk Hello Lai, On Thu, Sep 06, 2012 at 04:14:51PM +0800, Lai Jiangshan wrote: > On 09/06/2012 10:53 AM, Minchan Kim wrote: > > Normally, MIGRATE_ISOLATE type is used for memory-hotplug. > > But it's irony type because the pages isolated would exist > > as free page in free_area->free_list[MIGRATE_ISOLATE] so people > > can think of it as allocatable pages but it is *never* allocatable. > > It ends up confusing NR_FREE_PAGES vmstat so it would be > > totally not accurate so some of place which depend on such vmstat > > could reach wrong decision by the context. > > > > There were already report about it.[1] > > [1] 702d1a6e, memory-hotplug: fix kswapd looping forever problem > > > > Then, there was other report which is other problem.[2] > > [2] http://www.spinics.net/lists/linux-mm/msg41251.html > > > > I believe it can make problems in future, too. > > So I hope removing such irony type by another design. > > > > I hope this patch solves it and let's revert [1] and doesn't need [2]. > > > > * Changelog v1 > > * Fix from Michal's many suggestion > > > > Cc: Michal Nazarewicz > > Cc: Mel Gorman > > Cc: Kamezawa Hiroyuki > > Cc: Yasuaki Ishimatsu > > Cc: Wen Congyang > > Cc: Konrad Rzeszutek Wilk > > Signed-off-by: Minchan Kim > > --- > > > @@ -180,30 +287,35 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, > > * all pages in [start_pfn...end_pfn) must be in the same zone. > > * zone->lock must be held before call this. > > * > > - * Returns 1 if all pages in the range are isolated. > > + * Returns true if all pages in the range are isolated. > > */ > > -static int > > -__test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn) > > +static bool > > +__test_page_isolated_in_pageblock(unsigned long start_pfn, unsigned long end_pfn) > > { > > + unsigned long pfn, next_pfn; > > struct page *page; > > > > - while (pfn < end_pfn) { > > - if (!pfn_valid_within(pfn)) { > > - pfn++; > > - continue; > > - } > > - page = pfn_to_page(pfn); > > - if (PageBuddy(page)) > > - pfn += 1 << page_order(page); > > - else if (page_count(page) == 0 && > > - page_private(page) == MIGRATE_ISOLATE) > > - pfn += 1; > > - else > > - break; > > + list_for_each_entry(page, &isolated_pages, lru) { > > > + if (&page->lru == &isolated_pages) > > + return false; > > what's the mean of this line? I just copied it from Michal's code but It seem to be not needed. I will remove it in next spin. > > > + pfn = page_to_pfn(page); > > + if (pfn >= end_pfn) > > + return false; > > + if (pfn >= start_pfn) > > + goto found; > > + } > > + return false; > > + > > + list_for_each_entry_continue(page, &isolated_pages, lru) { > > + if (page_to_pfn(page) != next_pfn) > > + return false; > > where is next_pfn init-ed? by "goto found" > > > +found: > > + pfn = page_to_pfn(page); > > + next_pfn = pfn + (1UL << page_order(page)); > > + if (next_pfn >= end_pfn) > > + return true; > > } > > - if (pfn < end_pfn) > > - return 0; > > - return 1; > > + return false; > > } > > > > int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn) > > @@ -211,7 +323,7 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn) > > unsigned long pfn, flags; > > struct page *page; > > struct zone *zone; > > - int ret; > > + bool ret; > > > > /* > > * Note: pageblock_nr_page != MAX_ORDER. Then, chunks of free page > > diff --git a/mm/vmstat.c b/mm/vmstat.c > > index df7a674..bb59ff7 100644 > > --- a/mm/vmstat.c > > +++ b/mm/vmstat.c > > @@ -616,7 +616,6 @@ static char * const migratetype_names[MIGRATE_TYPES] = { > > #ifdef CONFIG_CMA > > "CMA", > > #endif > > - "Isolate", > > }; > > > > static void *frag_start(struct seq_file *m, loff_t *pos) > > -- > 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 -- Kind regards, Minchan Kim -- 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