start_isolate_page_range() question/offline_pages() bug ?

start_isolate_page_range() question/offline_pages() bug ?

[Badari Pulavarty]

Hi KAME,

While testing hotplug memory remove on x86_64, found an issue.

offline_pages()
{

	...
	
	/* set above range as isolated */
        ret = start_isolate_page_range(start_pfn, end_pfn);
	

	... does all the work and successful ...

        /* reset pagetype flags */
        start_isolate_page_range(start_pfn, end_pfn);
        /* removal success */
}

As you can see it calls, start_isolate_page_range() again at
the end. Why ? I am assuming that, to clear MIGRATE_ISOLATE
type for those pages we marked earlier. Isn't it ? But its
wrong. The pages are already set MIGRATE_ISOLATE and it
will end up clearing ONLY the first page in the pageblock.
Shouldn't we clear MIGRATE_ISOLATE for all the pages ?

I see this issue on x86-64, because /sysfs memory block
is 128MB, but pageblock_nr_pages = 512 (2MB).

I can reproduce the problem easily.. by doing ..

echo offline > state
echo online > state
echo offline > state <--- this one will fail
echo offline > state <-- fail
echo offline > state <-- fail

Everytime we do "offline" it clears first page in 2MB
section as part of undo :(

Here is the debug:
	
memory offlining 58000 to 60000 started
Offlined Pages 32768

memory offlining 58000 to 60000 started
isolate failed for pfn 58200 migratetype 4

memory offlining 58000 to 60000 started
isolate failed for pfn 58400 migratetype 4

memory offlining 58000 to 60000 started
isolate failed for pfn 58600 migratetype 4

memory offlining 58000 to 60000 started
isolate failed for pfn 58800 migratetype 4


Thanks,
Badari

--
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: start_isolate_page_range() question/offline_pages() bug ?

[KAMEZAWA Hiroyuki]

Hi, Badari. Thank you for digging.

On Thu, 01 Nov 2007 11:19:28 -0800
Badari Pulavarty <pbadari@us.ibm.com> wrote:

> Hi KAME,
> 
> While testing hotplug memory remove on x86_64, found an issue.
> 
> offline_pages()
> {
> 
> 	...
> 	
> 	/* set above range as isolated */
>         ret = start_isolate_page_range(start_pfn, end_pfn);
> 	
> 
> 	... does all the work and successful ...
> 
>         /* reset pagetype flags */
>         start_isolate_page_range(start_pfn, end_pfn);
>         /* removal success */
> }
> 
> As you can see it calls, start_isolate_page_range() again at
> the end. Why ? I am assuming that, to clear MIGRATE_ISOLATE
> type for those pages we marked earlier. Isn't it ? But its
> wrong. The pages are already set MIGRATE_ISOLATE and it
> will end up clearing ONLY the first page in the pageblock.
> Shouldn't we clear MIGRATE_ISOLATE for all the pages ?
> 
Hmm.. Maybe I wanted to call undo_isolate_page_range()

> I see this issue on x86-64, because /sysfs memory block
> is 128MB, but pageblock_nr_pages = 512 (2MB).
> 
Hmm, do you have patches for x86_64 ?

> I can reproduce the problem easily.. by doing ..
> 
> echo offline > state
> echo online > state
> echo offline > state <--- this one will fail
> echo offline > state <-- fail
> echo offline > state <-- fail
> 
> Everytime we do "offline" it clears first page in 2MB
> section as part of undo :(
> 
Hmmmm.... could you share your x86_64 patch ?
I'll dig more. below is my patch at this point.

Index: devel-2.6.23-mm1/mm/memory_hotplug.c
===================================================================
--- devel-2.6.23-mm1.orig/mm/memory_hotplug.c
+++ devel-2.6.23-mm1/mm/memory_hotplug.c
@@ -536,8 +536,8 @@ repeat:
 	/* Ok, all of our target is islaoted.
 	   We cannot do rollback at this point. */
 	offline_isolated_pages(start_pfn, end_pfn);
-	/* reset pagetype flags */
-	start_isolate_page_range(start_pfn, end_pfn);
+	/* reset pagetype flags and makes migrate type to be MOVABLE */
+	undo_isolate_page_range(start_pfn, end_pfn);
 	/* removal success */
 	zone = page_zone(pfn_to_page(start_pfn));
 	zone->present_pages -= offlined_pages;
Index: devel-2.6.23-mm1/mm/page_isolation.c
===================================================================
--- devel-2.6.23-mm1.orig/mm/page_isolation.c
+++ devel-2.6.23-mm1/mm/page_isolation.c
@@ -55,7 +55,7 @@ start_isolate_page_range(unsigned long s
 	return 0;
 undo:
 	for (pfn = start_pfn;
-	     pfn <= undo_pfn;
+	     pfn < undo_pfn;
 	     pfn += pageblock_nr_pages)
 		unset_migratetype_isolate(pfn_to_page(pfn));
 

--
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: start_isolate_page_range() question/offline_pages() bug ?

[KAMEZAWA Hiroyuki]

Hi, Badari, and Andrew

This is bugfix for memory hotremove.

But we'll need x86_64 memory hotremove patch set to test this easily.

Then, I'd like to schedule this with Goto's ioresource patch and
Badari's fix and this one and x86_64 memory hotremove support patch
against the next -mm.

This is quick fix. Thank you, Badari.

Regards,
-Kame
==

We should unset migrate type "ISOLATE" when we successfully removed
memory. But current code has BUG and cannot works well.

This patch also includes bugfix? to change get_pageblock_flags to
get_pageblock_migratetype().

Tested with x86_64 memory hotremove (private) patch and works well.
(It will be posted if things settled.)

Thanks to Badari Pulavarty for finding this.

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


 mm/memory_hotplug.c |    4 ++--
 mm/page_isolation.c |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

Index: devel-2.6.23-mm1/mm/memory_hotplug.c
===================================================================
--- devel-2.6.23-mm1.orig/mm/memory_hotplug.c
+++ devel-2.6.23-mm1/mm/memory_hotplug.c
@@ -536,8 +536,8 @@ repeat:
 	/* Ok, all of our target is islaoted.
 	   We cannot do rollback at this point. */
 	offline_isolated_pages(start_pfn, end_pfn);
-	/* reset pagetype flags */
-	start_isolate_page_range(start_pfn, end_pfn);
+	/* reset pagetype flags and makes migrate type to be MOVABLE */
+	undo_isolate_page_range(start_pfn, end_pfn);
 	/* removal success */
 	zone = page_zone(pfn_to_page(start_pfn));
 	zone->present_pages -= offlined_pages;
Index: devel-2.6.23-mm1/mm/page_isolation.c
===================================================================
--- devel-2.6.23-mm1.orig/mm/page_isolation.c
+++ devel-2.6.23-mm1/mm/page_isolation.c
@@ -55,7 +55,7 @@ start_isolate_page_range(unsigned long s
 	return 0;
 undo:
 	for (pfn = start_pfn;
-	     pfn <= undo_pfn;
+	     pfn < undo_pfn;
 	     pfn += pageblock_nr_pages)
 		unset_migratetype_isolate(pfn_to_page(pfn));
 
@@ -76,7 +76,7 @@ undo_isolate_page_range(unsigned long st
 	     pfn < end_pfn;
 	     pfn += pageblock_nr_pages) {
 		page = __first_valid_page(pfn, pageblock_nr_pages);
-		if (!page || get_pageblock_flags(page) != MIGRATE_ISOLATE)
+		if (!page || get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
 			continue;
 		unset_migratetype_isolate(page);
 	}
@@ -126,7 +126,7 @@ int test_pages_isolated(unsigned long st
 	 */
 	for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
 		page = __first_valid_page(pfn, pageblock_nr_pages);
-		if (page && get_pageblock_flags(page) != MIGRATE_ISOLATE)
+		if (page && get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
 			break;
 	}
 	if (pfn < end_pfn)

--
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: start_isolate_page_range() question/offline_pages() bug ?

[Badari Pulavarty]

On Fri, 2007-11-02 at 16:58 +0900, KAMEZAWA Hiroyuki wrote:
> Hi, Badari, and Andrew
> 
> This is bugfix for memory hotremove.
> 
> But we'll need x86_64 memory hotremove patch set to test this easily.
> 
> Then, I'd like to schedule this with Goto's ioresource patch and
> Badari's fix and this one and x86_64 memory hotremove support patch
> against the next -mm.
> 
> This is quick fix. Thank you, Badari.

Yes. These are needed for testing x86-64 (which we don't have support
for). We can schedule these for next -mm.

I am okay with your patch & Goto's patch. Please include my changes
also.

Acked-by: Badari Pulavarty <pbadari@us.ibm.com>

Thanks,
Badari



> 
> We should unset migrate type "ISOLATE" when we successfully removed
> memory. But current code has BUG and cannot works well.
> 
> This patch also includes bugfix? to change get_pageblock_flags to
> get_pageblock_migratetype().
> 
> Tested with x86_64 memory hotremove (private) patch and works well.
> (It will be posted if things settled.)
> 
> Thanks to Badari Pulavarty for finding this.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> 
>  mm/memory_hotplug.c |    4 ++--
>  mm/page_isolation.c |    6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> Index: devel-2.6.23-mm1/mm/memory_hotplug.c
> ===================================================================
> --- devel-2.6.23-mm1.orig/mm/memory_hotplug.c
> +++ devel-2.6.23-mm1/mm/memory_hotplug.c
> @@ -536,8 +536,8 @@ repeat:
>  	/* Ok, all of our target is islaoted.
>  	   We cannot do rollback at this point. */
>  	offline_isolated_pages(start_pfn, end_pfn);
> -	/* reset pagetype flags */
> -	start_isolate_page_range(start_pfn, end_pfn);
> +	/* reset pagetype flags and makes migrate type to be MOVABLE */
> +	undo_isolate_page_range(start_pfn, end_pfn);
>  	/* removal success */
>  	zone = page_zone(pfn_to_page(start_pfn));
>  	zone->present_pages -= offlined_pages;
> Index: devel-2.6.23-mm1/mm/page_isolation.c
> ===================================================================
> --- devel-2.6.23-mm1.orig/mm/page_isolation.c
> +++ devel-2.6.23-mm1/mm/page_isolation.c
> @@ -55,7 +55,7 @@ start_isolate_page_range(unsigned long s
>  	return 0;
>  undo:
>  	for (pfn = start_pfn;
> -	     pfn <= undo_pfn;
> +	     pfn < undo_pfn;
>  	     pfn += pageblock_nr_pages)
>  		unset_migratetype_isolate(pfn_to_page(pfn));
> 
> @@ -76,7 +76,7 @@ undo_isolate_page_range(unsigned long st
>  	     pfn < end_pfn;
>  	     pfn += pageblock_nr_pages) {
>  		page = __first_valid_page(pfn, pageblock_nr_pages);
> -		if (!page || get_pageblock_flags(page) != MIGRATE_ISOLATE)
> +		if (!page || get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
>  			continue;
>  		unset_migratetype_isolate(page);
>  	}
> @@ -126,7 +126,7 @@ int test_pages_isolated(unsigned long st
>  	 */
>  	for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
>  		page = __first_valid_page(pfn, pageblock_nr_pages);
> -		if (page && get_pageblock_flags(page) != MIGRATE_ISOLATE)
> +		if (page && get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
>  			break;
>  	}
>  	if (pfn < end_pfn)
> 

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