Re: PROBLEM: __offline_isolated_pages may offline too many pages

[Mel Gorman <mel@csn.ul.ie>]

On Thu, Dec 16, 2010 at 09:06:57AM +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 15 Dec 2010 10:25:51 +0100
> Ingo Korb <ingo@akana.de> wrote:
> 
> > On 15.12.2010 01:21, KAMEZAWA Hiroyuki wrote:
> > 
> > > It's designed for offline memory section>  MAX_ORDER. pageblock_nr_pages
> > > is tend to be smaller than that.
> > >
> > > Do you see the problem with _exsisting_ user interface of memory hotplug ?
> > > I think we have no control other than memory section.
> > 
> > The existing, exported interface (remove_memory() - the check itself is 
> > in offline_pages()) only checks if both start and end of the 
> > to-be-removed block are aligned to pageblock_nr_pages. As you noted the 
> > actual size and alignment requirements in __offline_isolated_pages can 
> > be larger that that, so I think the checks in offline_pages() should be 
> > changed (if 1<<MAX_ORDER is always >= pageblock_nr_pages) or extended 
> > (if there can be any relation between the two).
> > 
> 
> Ok, maybe my mistake. This is a fix. Thank you for reporting.
> ==
> 
> offline_pages()'s sanity check of given range is wrong. It should
> be aligned to MAX_ORDER. Current exsiting caller uses SECTION_SIZE
> alignment, so this change has no influence to exsisting callers.
> 
> Reported-by: Ingo Korb <ingo@akana.de>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Other than the spelling mistakes in the changelog and the lack of a
subject;

Acked-by: Mel Gorman <mel@csn.ul.ie>

> ---
>  mm/memory_hotplug.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.37-rc5/mm/memory_hotplug.c
> ===================================================================
> --- linux-2.6.37-rc5.orig/mm/memory_hotplug.c
> +++ linux-2.6.37-rc5/mm/memory_hotplug.c
> @@ -798,10 +798,14 @@ static int offline_pages(unsigned long s
>  	struct memory_notify arg;
>  
>  	BUG_ON(start_pfn >= end_pfn);
> -	/* at least, alignment against pageblock is necessary */
> -	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
> +	/*
> +	 * Considering buddy allocator which joins nearby pages, the range
> +	 * in offline should be aligned to MAX_ORDER. If not, isolated
> +	 * page will be joined to other (not isolated) pages.
> +	 */
> +	if (!IS_ALIGNED(start_pfn, MAX_ORDER_NR_PAGES))
>  		return -EINVAL;
> -	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
> +	if (!IS_ALIGNED(end_pfn, MAX_ORDER_NR_PAGES))
>  		return -EINVAL;
>  	/* This makes hotplug much easier...and readable.
>  	   we assume this for now. .*/
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>