Re: [PATCH] memory-hotplug: only drain LRU when failed to offline pages

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Mon, 25 Oct 2010 13:12:02 +0800
Wu Fengguang <fengguang.wu@intel.com> wrote:

> do_migrate_range() offlines 1MB pages at one time and hence might be
> called up to 16000 times when trying to offline 16GB memory. 

But size of memory section is not such big.

> It makes sense to avoid sending the costly IPIs to drain pages on all LRU for
> the 99% cases that do_migrate_range() succeeds offlining some pages.
> 

did you test ? I think this patch should be tested by IBM guys.

> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>

Sorry, this will HUNK. Could you wait until the end of merge window ?


> ---
>  mm/memory_hotplug.c |   24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> --- linux-next.orig/mm/memory_hotplug.c	2010-10-25 11:20:47.000000000 +0800
> +++ linux-next/mm/memory_hotplug.c	2010-10-25 13:07:10.000000000 +0800
> @@ -788,7 +788,7 @@ static int offline_pages(unsigned long s
>  {
>  	unsigned long pfn, nr_pages, expire;
>  	long offlined_pages;
> -	int ret, drain, retry_max, node;
> +	int ret, retry_max, node;
>  	struct zone *zone;
>  	struct memory_notify arg;
>  
> @@ -827,7 +827,6 @@ static int offline_pages(unsigned long s
>  
>  	pfn = start_pfn;
>  	expire = jiffies + timeout;
> -	drain = 0;
>  	retry_max = 5;
>  repeat:
>  	/* start memory hot removal */
> @@ -838,34 +837,31 @@ repeat:
>  	if (signal_pending(current))
>  		goto failed_removal;
>  	ret = 0;
> -	if (drain) {
> -		lru_add_drain_all();
> -		flush_scheduled_work();

this flush_scheduled_work() is removed in recent work of Tejun Heo.

> -		cond_resched();
> -		drain_all_pages();
> -	}
> -
>  	pfn = scan_lru_pages(start_pfn, end_pfn);
>  	if (pfn) { /* We have page on LRU */
>  		ret = do_migrate_range(pfn, end_pfn);
>  		if (!ret) {
> -			drain = 1;
>  			goto repeat;
>  		} else {
>  			if (ret < 0)
>  				if (--retry_max == 0)
>  					goto failed_removal;
>  			yield();
> -			drain = 1;
> +			lru_add_drain_all();
> +			flush_scheduled_work();
This flush is unnecessary.

> +			cond_resched();
> +			drain_all_pages();

I think followin is  better order.

drain_all_pages();      # SEND IPI and asynchronous.
lru_add_drain_pages();  # call schedule_work ony by one and it's synchronous.
cond_resched();	# may not be unnecessary (lru_add_drain_pages() will sleep.)

>  			goto repeat;
>  		}
>  	}
> -	/* drain all zone's lru pagevec, this is asyncronous... */
> +
> +	/* drain all zone's lru pagevec, this is asynchronous... */
>  	lru_add_drain_all();
>  	flush_scheduled_work();
This flush() is dropped by recent works of Tejun Heo's workqueue updates.

Bye,
-Kame

--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>