[PATCH 1/3] page_isolation: codeclean fix comment and rm unneeded val init

[PATCH 1/3] page_isolation: codeclean fix comment and rm unneeded val init

[Bob Liu]

function __test_page_isolated_in_pageblock() return 1 if all pages
in the range is isolated, so fix the comment.
value pfn will be init in the following loop so rm it.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/page_isolation.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 5e0ffd9..4ae42bb 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -86,7 +86,7 @@ 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 0 if all pages in the range is isolated.
+ * Returns 1 if all pages in the range is isolated.
  */
 static int
 __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
@@ -119,7 +119,6 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)
 	struct zone *zone;
 	int ret;
 
-	pfn = start_pfn;
 	/*
 	 * Note: pageblock_nr_page != MAX_ORDER. Then, chunks of free page
 	 * is not aligned to pageblock_nr_pages.
-- 
1.5.6.3

--
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>

[PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Bob Liu]

If not_managed is true all pages will be putback to lru, so
break the loop earlier to skip other pages isolate.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/memory_hotplug.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index d4e940a..4f72184 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 					    page_is_file_cache(page));
 
 		} else {
-			/* Becasue we don't have big zone->lock. we should
-			   check this again here. */
-			if (page_count(page))
-				not_managed++;
 #ifdef CONFIG_DEBUG_VM
 			printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
 			       pfn);
 			dump_page(page);
 #endif
+			/* Becasue we don't have big zone->lock. we should
+			   check this again here. */
+			if (page_count(page)) {
+				not_managed++;
+				break;
+			}
 		}
 	}
 	ret = -EBUSY;
-- 
1.5.6.3

--
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>

[PATCH 3/3] do_migrate_range: reduce list_empty() check.

[Bob Liu]

simple code for reducing list_empty(&source) check.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/memory_hotplug.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 4f72184..b6ffcfe 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -718,22 +718,19 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 			   check this again here. */
 			if (page_count(page)) {
 				not_managed++;
+				ret = -EBUSY;
 				break;
 			}
 		}
 	}
-	ret = -EBUSY;
-	if (not_managed) {
-		if (!list_empty(&source))
+	if (!list_empty(&source)) {
+		if (not_managed) {
 			putback_lru_pages(&source);
-		goto out;
+			goto out;
+		}
+		/* this function returns # of failed pages */
+		ret = migrate_pages(&source, hotremove_migrate_alloc, 0, 1);
 	}
-	ret = 0;
-	if (list_empty(&source))
-		goto out;
-	/* this function returns # of failed pages */
-	ret = migrate_pages(&source, hotremove_migrate_alloc, 0, 1);
-
 out:
 	return ret;
 }
-- 
1.5.6.3

--
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>

Re: [PATCH 3/3] do_migrate_range: reduce list_empty() check.

[KAMEZAWA Hiroyuki]

On Thu, 21 Oct 2010 21:28:21 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> simple code for reducing list_empty(&source) check.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

--
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>

Re: [PATCH 3/3] do_migrate_range: reduce list_empty() check.

[Wu Fengguang]

On Thu, Oct 21, 2010 at 09:28:21PM +0800, Bob Liu wrote:
> simple code for reducing list_empty(&source) check.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

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

--
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>

Re: [PATCH 3/3] do_migrate_range: reduce list_empty() check.

[Andrew Morton]

It's not completely clear to me that these three patches are finalised.
If updates are needed, lease send them ASAP.

On Thu, 21 Oct 2010 21:28:21 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> simple code for reducing list_empty(&source) check.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/memory_hotplug.c |   17 +++++++----------
>  1 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4f72184..b6ffcfe 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -718,22 +718,19 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  			   check this again here. */
>  			if (page_count(page)) {
>  				not_managed++;
> +				ret = -EBUSY;
>  				break;
>  			}
>  		}
>  	}
> -	ret = -EBUSY;
> -	if (not_managed) {
> -		if (!list_empty(&source))
> +	if (!list_empty(&source)) {
> +		if (not_managed) {
>  			putback_lru_pages(&source);
> -		goto out;
> +			goto out;
> +		}
> +		/* this function returns # of failed pages */
> +		ret = migrate_pages(&source, hotremove_migrate_alloc, 0, 1);
>  	}
> -	ret = 0;
> -	if (list_empty(&source))
> -		goto out;
> -	/* this function returns # of failed pages */
> -	ret = migrate_pages(&source, hotremove_migrate_alloc, 0, 1);
> -
>  out:
>  	return ret;

The code you're patching has changed a bit in -mm.  Here's what I ended
up with:

	static int
	do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
	{
		unsigned long pfn;
		struct page *page;
		int move_pages = NR_OFFLINE_AT_ONCE_PAGES;
		int not_managed = 0;
		int ret = 0;
		LIST_HEAD(source);
	
		for (pfn = start_pfn; pfn < end_pfn && move_pages > 0; pfn++) {
			if (!pfn_valid(pfn))
				continue;
			page = pfn_to_page(pfn);
			if (!page_count(page))
				continue;
			/*
			 * We can skip free pages. And we can only deal with pages on
			 * LRU.
			 */
			ret = isolate_lru_page(page);
			if (!ret) { /* Success */
				list_add_tail(&page->lru, &source);
				move_pages--;
				inc_zone_page_state(page, NR_ISOLATED_ANON +
						    page_is_file_cache(page));
	
			} else {
	#ifdef CONFIG_DEBUG_VM
				printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
				       pfn);
				dump_page(page);
	#endif
				/* Becasue we don't have big zone->lock. we should
				   check this again here. */
				if (page_count(page)) {
					not_managed++;
					ret = -EBUSY;
					break;
				}
			}
		}
		if (!list_empty(&source)) {
			if (not_managed) {
				putback_lru_pages(&source);
				goto out;
			}
			/* this function returns # of failed pages */
			ret = migrate_pages(&source, hotremove_migrate_alloc, 0, 1);
-->>			if (ret)
-->>				putback_lru_pages(&source);
		}
	out:
		return ret;
	}
	

--
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>

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Wu Fengguang]

On Thu, Oct 21, 2010 at 09:28:20PM +0800, Bob Liu wrote:
> If not_managed is true all pages will be putback to lru, so
> break the loop earlier to skip other pages isolate.

It's good fix in itself. However it's normal for isolate_lru_page() to
fail at times (when there are active reclaimers). The failures are
typically temporal and may well go away when offline_pages() retries
the call. So it seems more reasonable to migrate as much as possible
to increase the chance of complete success in next retry.

> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/memory_hotplug.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index d4e940a..4f72184 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  					    page_is_file_cache(page));
>  
>  		} else {
> -			/* Becasue we don't have big zone->lock. we should
> -			   check this again here. */
> -			if (page_count(page))
> -				not_managed++;
>  #ifdef CONFIG_DEBUG_VM
>  			printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
>  			       pfn);
>  			dump_page(page);
>  #endif
> +			/* Becasue we don't have big zone->lock. we should
> +			   check this again here. */
> +			if (page_count(page)) {
> +				not_managed++;
> +				break;
> +			}
>  		}
>  	}
>  	ret = -EBUSY;
> -- 
> 1.5.6.3

--
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>

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Bob Liu]

On Thu, Oct 21, 2010 at 10:25 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> On Thu, Oct 21, 2010 at 09:28:20PM +0800, Bob Liu wrote:
>> If not_managed is true all pages will be putback to lru, so
>> break the loop earlier to skip other pages isolate.
>
> It's good fix in itself. However it's normal for isolate_lru_page() to
> fail at times (when there are active reclaimers). The failures are
> typically temporal and may well go away when offline_pages() retries
> the call. So it seems more reasonable to migrate as much as possible
> to increase the chance of complete success in next retry.
>

Hi, Wu

The original code will try to migrate pages as much as possible except
page_count(page) is true.
If page_count(page) is true, isolate more pages is mean-less, because
all of them will
be put back after the loop.

Or maybe we can skip the page_count() check?  It seems unreasonable,
if isolate one page failed and
that page was in use why it needs to put back the whole isolated list?

Thanks

>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> ---
>>  mm/memory_hotplug.c |   10 ++++++----
>>  1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index d4e940a..4f72184 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>                                           page_is_file_cache(page));
>>
>>               } else {
>> -                     /* Becasue we don't have big zone->lock. we should
>> -                        check this again here. */
>> -                     if (page_count(page))
>> -                             not_managed++;
>>  #ifdef CONFIG_DEBUG_VM
>>                       printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
>>                              pfn);
>>                       dump_page(page);
>>  #endif
>> +                     /* Becasue we don't have big zone->lock. we should
>> +                        check this again here. */
>> +                     if (page_count(page)) {
>> +                             not_managed++;
>> +                             break;
>> +                     }
>>               }
>>       }
>>       ret = -EBUSY;
>> --
>> 1.5.6.3
>
-- 
Regards,
--Bob

--
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>

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Wu Fengguang]

On Fri, Oct 22, 2010 at 10:48:51AM +0800, Bob Liu wrote:
> On Thu, Oct 21, 2010 at 10:25 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> > On Thu, Oct 21, 2010 at 09:28:20PM +0800, Bob Liu wrote:
> >> If not_managed is true all pages will be putback to lru, so
> >> break the loop earlier to skip other pages isolate.
> >
> > It's good fix in itself. However it's normal for isolate_lru_page() to
> > fail at times (when there are active reclaimers). The failures are
> > typically temporal and may well go away when offline_pages() retries
> > the call. So it seems more reasonable to migrate as much as possible
> > to increase the chance of complete success in next retry.
> >
> 
> Hi, Wu
> 
> The original code will try to migrate pages as much as possible except
> page_count(page) is true.
> If page_count(page) is true, isolate more pages is mean-less, because
> all of them will
> be put back after the loop.
> 
> Or maybe we can skip the page_count() check?  It seems unreasonable,
> if isolate one page failed and
> that page was in use why it needs to put back the whole isolated list?

My suggestion was to keep the page_count() check and remove
putback_lru_pages() and call migrate_pages() regardless of
not_managed.

Does that make sense for typical memory hot remove scenarios?
That will increase the possibility of success at the cost of some more
migrated pages in case memory offline fails.

Thanks,
Fengguang

> >> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> >> ---
> >> A mm/memory_hotplug.c | A  10 ++++++----
> >> A 1 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >> index d4e940a..4f72184 100644
> >> --- a/mm/memory_hotplug.c
> >> +++ b/mm/memory_hotplug.c
> >> @@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >> A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  page_is_file_cache(page));
> >>
> >> A  A  A  A  A  A  A  } else {
> >> - A  A  A  A  A  A  A  A  A  A  /* Becasue we don't have big zone->lock. we should
> >> - A  A  A  A  A  A  A  A  A  A  A  A check this again here. */
> >> - A  A  A  A  A  A  A  A  A  A  if (page_count(page))
> >> - A  A  A  A  A  A  A  A  A  A  A  A  A  A  not_managed++;
> >> A #ifdef CONFIG_DEBUG_VM
> >> A  A  A  A  A  A  A  A  A  A  A  printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
> >> A  A  A  A  A  A  A  A  A  A  A  A  A  A  A pfn);
> >> A  A  A  A  A  A  A  A  A  A  A  dump_page(page);
> >> A #endif
> >> + A  A  A  A  A  A  A  A  A  A  /* Becasue we don't have big zone->lock. we should
> >> + A  A  A  A  A  A  A  A  A  A  A  A check this again here. */
> >> + A  A  A  A  A  A  A  A  A  A  if (page_count(page)) {
> >> + A  A  A  A  A  A  A  A  A  A  A  A  A  A  not_managed++;
> >> + A  A  A  A  A  A  A  A  A  A  A  A  A  A  break;
> >> + A  A  A  A  A  A  A  A  A  A  }
> >> A  A  A  A  A  A  A  }
> >> A  A  A  }
> >> A  A  A  ret = -EBUSY;
> >> --
> >> 1.5.6.3
> >
> -- 
> Regards,
> --Bob

--
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>

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Bob Liu]

On Fri, Oct 22, 2010 at 11:22 AM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> On Fri, Oct 22, 2010 at 10:48:51AM +0800, Bob Liu wrote:
>> On Thu, Oct 21, 2010 at 10:25 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
>> > On Thu, Oct 21, 2010 at 09:28:20PM +0800, Bob Liu wrote:
>> >> If not_managed is true all pages will be putback to lru, so
>> >> break the loop earlier to skip other pages isolate.
>> >
>> > It's good fix in itself. However it's normal for isolate_lru_page() to
>> > fail at times (when there are active reclaimers). The failures are
>> > typically temporal and may well go away when offline_pages() retries
>> > the call. So it seems more reasonable to migrate as much as possible
>> > to increase the chance of complete success in next retry.
>> >
>>
>> Hi, Wu
>>
>> The original code will try to migrate pages as much as possible except
>> page_count(page) is true.
>> If page_count(page) is true, isolate more pages is mean-less, because
>> all of them will
>> be put back after the loop.
>>
>> Or maybe we can skip the page_count() check?  It seems unreasonable,
>> if isolate one page failed and
>> that page was in use why it needs to put back the whole isolated list?
>
> My suggestion was to keep the page_count() check and remove
> putback_lru_pages() and call migrate_pages() regardless of
> not_managed.
>

If not_managed is no more used, page_count() will also meanless.
You mean patch like this:
==
@@ -687,7 +687,6 @@
 	unsigned long pfn;
 	struct page *page;
 	int move_pages = NR_OFFLINE_AT_ONCE_PAGES;
-	int not_managed = 0;
 	int ret = 0;
 	LIST_HEAD(source);

@@ -709,10 +708,6 @@
 					    page_is_file_cache(page));

 		} else {
-			/* Becasue we don't have big zone->lock. we should
-			   check this again here. */
-			if (page_count(page))
-				not_managed++;
 #ifdef CONFIG_DEBUG_VM
 			printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
 			       pfn);
@@ -720,12 +715,6 @@
 #endif
 		}
 	}
-	ret = -EBUSY;
-	if (not_managed) {
-		if (!list_empty(&source))
-			putback_lru_pages(&source);
-		goto out;
-	}
 	ret = 0;
 	if (list_empty(&source))
 		goto out;
==
Thanks,

> Does that make sense for typical memory hot remove scenarios?
> That will increase the possibility of success at the cost of some more
> migrated pages in case memory offline fails.
>
> Thanks,
> Fengguang
>
>> >> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> >> ---
>> >>  mm/memory_hotplug.c |   10 ++++++----
>> >>  1 files changed, 6 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> >> index d4e940a..4f72184 100644
>> >> --- a/mm/memory_hotplug.c
>> >> +++ b/mm/memory_hotplug.c
>> >> @@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>> >>                                           page_is_file_cache(page));
>> >>
>> >>               } else {
>> >> -                     /* Becasue we don't have big zone->lock. we should
>> >> -                        check this again here. */
>> >> -                     if (page_count(page))
>> >> -                             not_managed++;
>> >>  #ifdef CONFIG_DEBUG_VM
>> >>                       printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
>> >>                              pfn);
>> >>                       dump_page(page);
>> >>  #endif
>> >> +                     /* Becasue we don't have big zone->lock. we should
>> >> +                        check this again here. */
>> >> +                     if (page_count(page)) {
>> >> +                             not_managed++;
>> >> +                             break;
>> >> +                     }
>> >>               }
>> >>       }
>> >>       ret = -EBUSY;
>> >> --
-- 
Regards,
--Bob

--
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>

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Wu Fengguang]

On Fri, Oct 22, 2010 at 04:41:40PM +0800, Bob Liu wrote:
> On Fri, Oct 22, 2010 at 11:22 AM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> > On Fri, Oct 22, 2010 at 10:48:51AM +0800, Bob Liu wrote:
> >> On Thu, Oct 21, 2010 at 10:25 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> >> > On Thu, Oct 21, 2010 at 09:28:20PM +0800, Bob Liu wrote:
> >> >> If not_managed is true all pages will be putback to lru, so
> >> >> break the loop earlier to skip other pages isolate.
> >> >
> >> > It's good fix in itself. However it's normal for isolate_lru_page() to
> >> > fail at times (when there are active reclaimers). The failures are
> >> > typically temporal and may well go away when offline_pages() retries
> >> > the call. So it seems more reasonable to migrate as much as possible
> >> > to increase the chance of complete success in next retry.
> >> >
> >>
> >> Hi, Wu
> >>
> >> The original code will try to migrate pages as much as possible except
> >> page_count(page) is true.
> >> If page_count(page) is true, isolate more pages is mean-less, because
> >> all of them will
> >> be put back after the loop.
> >>
> >> Or maybe we can skip the page_count() check? A It seems unreasonable,
> >> if isolate one page failed and
> >> that page was in use why it needs to put back the whole isolated list?
> >
> > My suggestion was to keep the page_count() check and remove
> > putback_lru_pages() and call migrate_pages() regardless of
> > not_managed.
> >
> 
> If not_managed is no more used, page_count() will also meanless.

Ah yes!

> You mean patch like this:

That's it.

I guess the typical usage would be

        for each 128M block
                try to offline it
                if failed
                        break

So it makes sense to avoid failure as much as possible, since the
above process is virtually not revertible -- if we failed at the
10th 128M block, the pages in the first 9 blocks have already been
migrated. It helps little to avoid page migration inside the 10th
block. The best we can do is to ensure success as much as possible
once the memory offline process starts.

And it's nice to reduce some code :)

Thanks,
Fengguang

> ==
> @@ -687,7 +687,6 @@
>  	unsigned long pfn;
>  	struct page *page;
>  	int move_pages = NR_OFFLINE_AT_ONCE_PAGES;
> -	int not_managed = 0;
>  	int ret = 0;
>  	LIST_HEAD(source);
> 
> @@ -709,10 +708,6 @@
>  					    page_is_file_cache(page));
> 
>  		} else {
> -			/* Becasue we don't have big zone->lock. we should
> -			   check this again here. */
> -			if (page_count(page))
> -				not_managed++;
>  #ifdef CONFIG_DEBUG_VM
>  			printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
>  			       pfn);
> @@ -720,12 +715,6 @@
>  #endif
>  		}
>  	}
> -	ret = -EBUSY;
> -	if (not_managed) {
> -		if (!list_empty(&source))
> -			putback_lru_pages(&source);
> -		goto out;
> -	}
>  	ret = 0;
>  	if (list_empty(&source))
>  		goto out;
> ==
> Thanks,
> 
> > Does that make sense for typical memory hot remove scenarios?
> > That will increase the possibility of success at the cost of some more
> > migrated pages in case memory offline fails.
> >
> > Thanks,
> > Fengguang
> >
> >> >> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> >> >> ---
> >> >> A mm/memory_hotplug.c | A  10 ++++++----
> >> >> A 1 files changed, 6 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >> >> index d4e940a..4f72184 100644
> >> >> --- a/mm/memory_hotplug.c
> >> >> +++ b/mm/memory_hotplug.c
> >> >> @@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >> >> A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  page_is_file_cache(page));
> >> >>
> >> >> A  A  A  A  A  A  A  } else {
> >> >> - A  A  A  A  A  A  A  A  A  A  /* Becasue we don't have big zone->lock. we should
> >> >> - A  A  A  A  A  A  A  A  A  A  A  A check this again here. */
> >> >> - A  A  A  A  A  A  A  A  A  A  if (page_count(page))
> >> >> - A  A  A  A  A  A  A  A  A  A  A  A  A  A  not_managed++;
> >> >> A #ifdef CONFIG_DEBUG_VM
> >> >> A  A  A  A  A  A  A  A  A  A  A  printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
> >> >> A  A  A  A  A  A  A  A  A  A  A  A  A  A  A pfn);
> >> >> A  A  A  A  A  A  A  A  A  A  A  dump_page(page);
> >> >> A #endif
> >> >> + A  A  A  A  A  A  A  A  A  A  /* Becasue we don't have big zone->lock. we should
> >> >> + A  A  A  A  A  A  A  A  A  A  A  A check this again here. */
> >> >> + A  A  A  A  A  A  A  A  A  A  if (page_count(page)) {
> >> >> + A  A  A  A  A  A  A  A  A  A  A  A  A  A  not_managed++;
> >> >> + A  A  A  A  A  A  A  A  A  A  A  A  A  A  break;
> >> >> + A  A  A  A  A  A  A  A  A  A  }
> >> >> A  A  A  A  A  A  A  }
> >> >> A  A  A  }
> >> >> A  A  A  ret = -EBUSY;
> >> >> --
> -- 
> Regards,
> --Bob

--
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>

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[KAMEZAWA Hiroyuki]

On Thu, 21 Oct 2010 21:28:20 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> If not_managed is true all pages will be putback to lru, so
> break the loop earlier to skip other pages isolate.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

please don't skip dump_page().

-Kame

> ---
>  mm/memory_hotplug.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index d4e940a..4f72184 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  					    page_is_file_cache(page));
>  
>  		} else {
> -			/* Becasue we don't have big zone->lock. we should
> -			   check this again here. */
> -			if (page_count(page))
> -				not_managed++;
>  #ifdef CONFIG_DEBUG_VM
>  			printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
>  			       pfn);
>  			dump_page(page);
>  #endif
> +			/* Becasue we don't have big zone->lock. we should
> +			   check this again here. */
> +			if (page_count(page)) {
> +				not_managed++;
> +				break;
> +			}
>  		}
>  	}
>  	ret = -EBUSY;
> -- 
> 1.5.6.3
> 
> 

--
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>

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Bob Liu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 2046 bytes --]

On Fri, Oct 22, 2010 at 11:16 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Thu, 21 Oct 2010 21:28:20 +0800
> Bob Liu <lliubbo@gmail.com> wrote:
>
>> If not_managed is true all pages will be putback to lru, so
>> break the loop earlier to skip other pages isolate.
>>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>
> please don't skip dump_page().
>

Hi, Kame

I put the check after dump_page() in order to we can still see the
dump message if the loop is broken earlier.

Thanks

> -Kame
>
>> ---
>>  mm/memory_hotplug.c |   10 ++++++----
>>  1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index d4e940a..4f72184 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>                                           page_is_file_cache(page));
>>
>>               } else {
>> -                     /* Becasue we don't have big zone->lock. we should
>> -                        check this again here. */
>> -                     if (page_count(page))
>> -                             not_managed++;
>>  #ifdef CONFIG_DEBUG_VM
>>                       printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
>>                              pfn);
>>                       dump_page(page);
>>  #endif
>> +                     /* Becasue we don't have big zone->lock. we should
>> +                        check this again here. */
>> +                     if (page_count(page)) {
>> +                             not_managed++;
>> +                             break;
>> +                     }
>>               }
>>       }
>>       ret = -EBUSY;
-- 
Regards,
--Bob
N‹§²æìr¸›zǧu©ž²Æ {\b­†éì¹»\x1c®&Þ–)îÆi¢žØ^n‡r¶‰šŽŠÝ¢j$½§$¢¸\x05¢¹¨­è§~Š'.)îÄÃ,yèm¶ŸÿÃ\f%Š{±šj+ƒðèž×¦j)Z†·Ÿ

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[KAMEZAWA Hiroyuki]

On Fri, 22 Oct 2010 16:41:13 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> On Fri, Oct 22, 2010 at 11:16 AM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > On Thu, 21 Oct 2010 21:28:20 +0800
> > Bob Liu <lliubbo@gmail.com> wrote:
> >
> >> If not_managed is true all pages will be putback to lru, so
> >> break the loop earlier to skip other pages isolate.
> >>
> >> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> >
> > please don't skip dump_page().
> >
> 
> Hi, Kame
> 
> I put the check after dump_page() in order to we can still see the
> dump message if the loop is broken earlier.
> 
> Thanks
> 

Ah, sorry. I misunderstood.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

--
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>

Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Wu Fengguang]

On Thu, Oct 21, 2010 at 09:28:20PM +0800, Bob Liu wrote:
> If not_managed is true all pages will be putback to lru, so
> break the loop earlier to skip other pages isolate.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

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

--
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>

Re: [PATCH 1/3] page_isolation: codeclean fix comment and rm unneeded val init

[Wu Fengguang]

On Thu, Oct 21, 2010 at 09:28:19PM +0800, Bob Liu wrote:
> function __test_page_isolated_in_pageblock() return 1 if all pages
> in the range is isolated, so fix the comment.
> value pfn will be init in the following loop so rm it.

This is a bit confusing, but the original comment should be intended
for test_pages_isolated()..

> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/page_isolation.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 5e0ffd9..4ae42bb 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -86,7 +86,7 @@ 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 0 if all pages in the range is isolated.
> + * Returns 1 if all pages in the range is isolated.
>   */
>  static int
>  __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
> @@ -119,7 +119,6 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)
>  	struct zone *zone;
>  	int ret;
>  
> -	pfn = start_pfn;
>  	/*
>  	 * Note: pageblock_nr_page != MAX_ORDER. Then, chunks of free page
>  	 * is not aligned to pageblock_nr_pages.
> -- 
> 1.5.6.3

--
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>

Re: [PATCH 1/3] page_isolation: codeclean fix comment and rm unneeded val init

[Bob Liu]

On Thu, Oct 21, 2010 at 10:01 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> On Thu, Oct 21, 2010 at 09:28:19PM +0800, Bob Liu wrote:
>> function __test_page_isolated_in_pageblock() return 1 if all pages
>> in the range is isolated, so fix the comment.
>> value pfn will be init in the following loop so rm it.
>
> This is a bit confusing, but the original comment should be intended
> for test_pages_isolated()..

Maybe it used to but now it said "zone->lock must be held before call this",
so it is the comment for __test_page_isolated_in_pageblock() nomore
test_pages_isolated(),
so fix the comment as this patch did.

Thanks.

>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> ---
>>  mm/page_isolation.c |    3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>> index 5e0ffd9..4ae42bb 100644
>> --- a/mm/page_isolation.c
>> +++ b/mm/page_isolation.c
>> @@ -86,7 +86,7 @@ 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 0 if all pages in the range is isolated.
>> + * Returns 1 if all pages in the range is isolated.
>>   */
>>  static int
>>  __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
>> @@ -119,7 +119,6 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)
>>       struct zone *zone;
>>       int ret;
>>
>> -     pfn = start_pfn;
>>       /*
>>        * Note: pageblock_nr_page != MAX_ORDER. Then, chunks of free page
>>        * is not aligned to pageblock_nr_pages.
>> --
>> 1.5.6.3
>
-- 
Regards,
--Bob

--
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>

Re: [PATCH 1/3] page_isolation: codeclean fix comment and rm unneeded val init

[Wu Fengguang]

On Fri, Oct 22, 2010 at 10:34:01AM +0800, Bob Liu wrote:
> On Thu, Oct 21, 2010 at 10:01 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> > On Thu, Oct 21, 2010 at 09:28:19PM +0800, Bob Liu wrote:
> >> function __test_page_isolated_in_pageblock() return 1 if all pages
> >> in the range is isolated, so fix the comment.
> >> value pfn will be init in the following loop so rm it.
> >
> > This is a bit confusing, but the original comment should be intended
> > for test_pages_isolated()..
> 
> Maybe it used to but now it said "zone->lock must be held before call this",
> so it is the comment for __test_page_isolated_in_pageblock() nomore
> test_pages_isolated(),
> so fix the comment as this patch did.

OK.

The comment still looks a bit awkward though. There are
redundant/inconsistent "... range is free(means isolated) or not",
"... range is isolated." and "all pages are free or Marked as
ISOLATED". And the "start_pfn" is only found in test_pages_isolated().

Thanks,
Fengguang

> >> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> >> ---
> >> A mm/page_isolation.c | A  A 3 +--
> >> A 1 files changed, 1 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> >> index 5e0ffd9..4ae42bb 100644
> >> --- a/mm/page_isolation.c
> >> +++ b/mm/page_isolation.c
> >> @@ -86,7 +86,7 @@ undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn)
> >> A  * all pages in [start_pfn...end_pfn) must be in the same zone.
> >> A  * zone->lock must be held before call this.
> >> A  *
> >> - * Returns 0 if all pages in the range is isolated.
> >> + * Returns 1 if all pages in the range is isolated.
> >> A  */
> >> A static int
> >> A __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
> >> @@ -119,7 +119,6 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)
> >> A  A  A  struct zone *zone;
> >> A  A  A  int ret;
> >>
> >> - A  A  pfn = start_pfn;
> >> A  A  A  /*
> >> A  A  A  A * Note: pageblock_nr_page != MAX_ORDER. Then, chunks of free page
> >> A  A  A  A * is not aligned to pageblock_nr_pages.
> >> --
> >> 1.5.6.3
> >
> -- 
> Regards,
> --Bob

--
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>

Re: [PATCH 1/3] page_isolation: codeclean fix comment and rm unneeded val init

[KAMEZAWA Hiroyuki]

On Thu, 21 Oct 2010 21:28:19 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> function __test_page_isolated_in_pageblock() return 1 if all pages
> in the range is isolated, so fix the comment.
> value pfn will be init in the following loop so rm it.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

--
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>