[PATCH]Fix parsing kernelcore boot option for ia64

[PATCH]Fix parsing kernelcore boot option for ia64

[Yasunori Goto]

Hello.

cmdline_parse_kernelcore() should return the next pointer of boot option
like memparse() doing. If not, it is cause of eternal loop on ia64 box.
This patch is for 2.6.21-rc6-mm1.

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>

----

 arch/ia64/kernel/efi.c |    2 +-
 include/linux/mm.h     |    2 +-
 mm/page_alloc.c        |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

Index: current_test/arch/ia64/kernel/efi.c
===================================================================
--- current_test.orig/arch/ia64/kernel/efi.c	2007-04-12 17:33:28.000000000 +0900
+++ current_test/arch/ia64/kernel/efi.c	2007-04-13 12:13:21.000000000 +0900
@@ -424,7 +424,7 @@ efi_init (void)
 		} else if (memcmp(cp, "max_addr=", 9) == 0) {
 			max_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
 		} else if (memcmp(cp, "kernelcore=",11) == 0) {
-			cmdline_parse_kernelcore(cp+11);
+			cmdline_parse_kernelcore(cp+11, &cp);
 		} else if (memcmp(cp, "min_addr=", 9) == 0) {
 			min_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
 		} else {
Index: current_test/mm/page_alloc.c
===================================================================
--- current_test.orig/mm/page_alloc.c	2007-04-12 18:25:37.000000000 +0900
+++ current_test/mm/page_alloc.c	2007-04-13 12:12:58.000000000 +0900
@@ -3736,13 +3736,13 @@ void __init free_area_init_nodes(unsigne
  * kernelcore=size sets the amount of memory for use for allocations that
  * cannot be reclaimed or migrated.
  */
-int __init cmdline_parse_kernelcore(char *p)
+int __init cmdline_parse_kernelcore(char *p, char **retp)
 {
 	unsigned long long coremem;
 	if (!p)
 		return -EINVAL;
 
-	coremem = memparse(p, &p);
+	coremem = memparse(p, retp);
 	required_kernelcore = coremem >> PAGE_SHIFT;
 
 	/* Paranoid check that UL is enough for required_kernelcore */
Index: current_test/include/linux/mm.h
===================================================================
--- current_test.orig/include/linux/mm.h	2007-04-11 14:15:33.000000000 +0900
+++ current_test/include/linux/mm.h	2007-04-13 12:12:20.000000000 +0900
@@ -1051,7 +1051,7 @@ extern unsigned long find_max_pfn_with_a
 extern void free_bootmem_with_active_regions(int nid,
 						unsigned long max_low_pfn);
 extern void sparse_memory_present_with_active_regions(int nid);
-extern int cmdline_parse_kernelcore(char *p);
+extern int cmdline_parse_kernelcore(char *p, char **retp);
 #ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
 extern int early_pfn_to_nid(unsigned long pfn);
 #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */

-- 
Yasunori Goto 

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Mel Gorman]

On Fri, 13 Apr 2007, Yasunori Goto wrote:

> Hello.
>
> cmdline_parse_kernelcore() should return the next pointer of boot option
> like memparse() doing. If not, it is cause of eternal loop on ia64 box.
> This patch is for 2.6.21-rc6-mm1.
>
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
>

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

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Andrew Morton]

On Fri, 13 Apr 2007 14:26:22 +0900 Yasunori Goto <y-goto@jp.fujitsu.com> wrote:

> Hello.
> 
> cmdline_parse_kernelcore() should return the next pointer of boot option
> like memparse() doing. If not, it is cause of eternal loop on ia64 box.
> This patch is for 2.6.21-rc6-mm1.
> 
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> 
> ----
> 
>  arch/ia64/kernel/efi.c |    2 +-
>  include/linux/mm.h     |    2 +-
>  mm/page_alloc.c        |    4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> Index: current_test/arch/ia64/kernel/efi.c
> ===================================================================
> --- current_test.orig/arch/ia64/kernel/efi.c	2007-04-12 17:33:28.000000000 +0900
> +++ current_test/arch/ia64/kernel/efi.c	2007-04-13 12:13:21.000000000 +0900
> @@ -424,7 +424,7 @@ efi_init (void)
>  		} else if (memcmp(cp, "max_addr=", 9) == 0) {
>  			max_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
>  		} else if (memcmp(cp, "kernelcore=",11) == 0) {
> -			cmdline_parse_kernelcore(cp+11);
> +			cmdline_parse_kernelcore(cp+11, &cp);
>  		} else if (memcmp(cp, "min_addr=", 9) == 0) {
>  			min_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
>  		} else {
> Index: current_test/mm/page_alloc.c
> ===================================================================
> --- current_test.orig/mm/page_alloc.c	2007-04-12 18:25:37.000000000 +0900
> +++ current_test/mm/page_alloc.c	2007-04-13 12:12:58.000000000 +0900
> @@ -3736,13 +3736,13 @@ void __init free_area_init_nodes(unsigne
>   * kernelcore=size sets the amount of memory for use for allocations that
>   * cannot be reclaimed or migrated.
>   */
> -int __init cmdline_parse_kernelcore(char *p)
> +int __init cmdline_parse_kernelcore(char *p, char **retp)
>  {
>  	unsigned long long coremem;
>  	if (!p)
>  		return -EINVAL;
>  
> -	coremem = memparse(p, &p);
> +	coremem = memparse(p, retp);
>  	required_kernelcore = coremem >> PAGE_SHIFT;
>  
>  	/* Paranoid check that UL is enough for required_kernelcore */
> Index: current_test/include/linux/mm.h
> ===================================================================
> --- current_test.orig/include/linux/mm.h	2007-04-11 14:15:33.000000000 +0900
> +++ current_test/include/linux/mm.h	2007-04-13 12:12:20.000000000 +0900
> @@ -1051,7 +1051,7 @@ extern unsigned long find_max_pfn_with_a
>  extern void free_bootmem_with_active_regions(int nid,
>  						unsigned long max_low_pfn);
>  extern void sparse_memory_present_with_active_regions(int nid);
> -extern int cmdline_parse_kernelcore(char *p);
> +extern int cmdline_parse_kernelcore(char *p, char **retp);
>  #ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
>  extern int early_pfn_to_nid(unsigned long pfn);
>  #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> 

This will cause all other architectures to crash when kernelcore= is used.

I wasn't even aware of this kernelcore thing.  It's pretty nasty-looking. 
yet another reminder that this code hasn't been properly reviewed in the
past year or three.

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Yasunori Goto]

> On Fri, 13 Apr 2007 14:26:22 +0900 Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> 
> > Hello.
> > 
> > cmdline_parse_kernelcore() should return the next pointer of boot option
> > like memparse() doing. If not, it is cause of eternal loop on ia64 box.
> > This patch is for 2.6.21-rc6-mm1.
> > 
> > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> > 
> > ----
> > 
> >  arch/ia64/kernel/efi.c |    2 +-
> >  include/linux/mm.h     |    2 +-
> >  mm/page_alloc.c        |    4 ++--
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > Index: current_test/arch/ia64/kernel/efi.c
> > ===================================================================
> > --- current_test.orig/arch/ia64/kernel/efi.c	2007-04-12 17:33:28.000000000 +0900
> > +++ current_test/arch/ia64/kernel/efi.c	2007-04-13 12:13:21.000000000 +0900
> > @@ -424,7 +424,7 @@ efi_init (void)
> >  		} else if (memcmp(cp, "max_addr=", 9) == 0) {
> >  			max_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
> >  		} else if (memcmp(cp, "kernelcore=",11) == 0) {
> > -			cmdline_parse_kernelcore(cp+11);
> > +			cmdline_parse_kernelcore(cp+11, &cp);
> >  		} else if (memcmp(cp, "min_addr=", 9) == 0) {
> >  			min_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
> >  		} else {
> > Index: current_test/mm/page_alloc.c
> > ===================================================================
> > --- current_test.orig/mm/page_alloc.c	2007-04-12 18:25:37.000000000 +0900
> > +++ current_test/mm/page_alloc.c	2007-04-13 12:12:58.000000000 +0900
> > @@ -3736,13 +3736,13 @@ void __init free_area_init_nodes(unsigne
> >   * kernelcore=size sets the amount of memory for use for allocations that
> >   * cannot be reclaimed or migrated.
> >   */
> > -int __init cmdline_parse_kernelcore(char *p)
> > +int __init cmdline_parse_kernelcore(char *p, char **retp)
> >  {
> >  	unsigned long long coremem;
> >  	if (!p)
> >  		return -EINVAL;
> >  
> > -	coremem = memparse(p, &p);
> > +	coremem = memparse(p, retp);
> >  	required_kernelcore = coremem >> PAGE_SHIFT;
> >  
> >  	/* Paranoid check that UL is enough for required_kernelcore */
> > Index: current_test/include/linux/mm.h
> > ===================================================================
> > --- current_test.orig/include/linux/mm.h	2007-04-11 14:15:33.000000000 +0900
> > +++ current_test/include/linux/mm.h	2007-04-13 12:12:20.000000000 +0900
> > @@ -1051,7 +1051,7 @@ extern unsigned long find_max_pfn_with_a
> >  extern void free_bootmem_with_active_regions(int nid,
> >  						unsigned long max_low_pfn);
> >  extern void sparse_memory_present_with_active_regions(int nid);
> > -extern int cmdline_parse_kernelcore(char *p);
> > +extern int cmdline_parse_kernelcore(char *p, char **retp);
> >  #ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> >  extern int early_pfn_to_nid(unsigned long pfn);
> >  #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> > 
> 
> This will cause all other architectures to crash when kernelcore= is used.
> 
> I wasn't even aware of this kernelcore thing.  It's pretty nasty-looking. 
> yet another reminder that this code hasn't been properly reviewed in the
> past year or three.

Just now, I'm making memory-unplug patches with current MOVABLE_ZONE
code. So, I might be the first user of it on ia64.

Anyway, I'll try to fix it.


-- 
Yasunori Goto 

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Mel Gorman]

On Mon, 23 Apr 2007, Yasunori Goto wrote:

>> On Fri, 13 Apr 2007 14:26:22 +0900 Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
>>
>>> Hello.
>>>
>>> cmdline_parse_kernelcore() should return the next pointer of boot option
>>> like memparse() doing. If not, it is cause of eternal loop on ia64 box.
>>> This patch is for 2.6.21-rc6-mm1.
>>>
>>> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
>>>
>>> ----
>>>
>>>  arch/ia64/kernel/efi.c |    2 +-
>>>  include/linux/mm.h     |    2 +-
>>>  mm/page_alloc.c        |    4 ++--
>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> Index: current_test/arch/ia64/kernel/efi.c
>>> ===================================================================
>>> --- current_test.orig/arch/ia64/kernel/efi.c	2007-04-12 17:33:28.000000000 +0900
>>> +++ current_test/arch/ia64/kernel/efi.c	2007-04-13 12:13:21.000000000 +0900
>>> @@ -424,7 +424,7 @@ efi_init (void)
>>>  		} else if (memcmp(cp, "max_addr=", 9) == 0) {
>>>  			max_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
>>>  		} else if (memcmp(cp, "kernelcore=",11) == 0) {
>>> -			cmdline_parse_kernelcore(cp+11);
>>> +			cmdline_parse_kernelcore(cp+11, &cp);
>>>  		} else if (memcmp(cp, "min_addr=", 9) == 0) {
>>>  			min_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
>>>  		} else {
>>> Index: current_test/mm/page_alloc.c
>>> ===================================================================
>>> --- current_test.orig/mm/page_alloc.c	2007-04-12 18:25:37.000000000 +0900
>>> +++ current_test/mm/page_alloc.c	2007-04-13 12:12:58.000000000 +0900
>>> @@ -3736,13 +3736,13 @@ void __init free_area_init_nodes(unsigne
>>>   * kernelcore=size sets the amount of memory for use for allocations that
>>>   * cannot be reclaimed or migrated.
>>>   */
>>> -int __init cmdline_parse_kernelcore(char *p)
>>> +int __init cmdline_parse_kernelcore(char *p, char **retp)
>>>  {
>>>  	unsigned long long coremem;
>>>  	if (!p)
>>>  		return -EINVAL;
>>>
>>> -	coremem = memparse(p, &p);
>>> +	coremem = memparse(p, retp);
>>>  	required_kernelcore = coremem >> PAGE_SHIFT;
>>>
>>>  	/* Paranoid check that UL is enough for required_kernelcore */
>>> Index: current_test/include/linux/mm.h
>>> ===================================================================
>>> --- current_test.orig/include/linux/mm.h	2007-04-11 14:15:33.000000000 +0900
>>> +++ current_test/include/linux/mm.h	2007-04-13 12:12:20.000000000 +0900
>>> @@ -1051,7 +1051,7 @@ extern unsigned long find_max_pfn_with_a
>>>  extern void free_bootmem_with_active_regions(int nid,
>>>  						unsigned long max_low_pfn);
>>>  extern void sparse_memory_present_with_active_regions(int nid);
>>> -extern int cmdline_parse_kernelcore(char *p);
>>> +extern int cmdline_parse_kernelcore(char *p, char **retp);
>>>  #ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
>>>  extern int early_pfn_to_nid(unsigned long pfn);
>>>  #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
>>>
>>
>> This will cause all other architectures to crash when kernelcore= is used.
>>
>> I wasn't even aware of this kernelcore thing.  It's pretty nasty-looking.
>> yet another reminder that this code hasn't been properly reviewed in the
>> past year or three.
>
> Just now, I'm making memory-unplug patches with current MOVABLE_ZONE
> code. So, I might be the first user of it on ia64.
>
> Anyway, I'll try to fix it.
>

I'm looking at this as well. There may be more than one problem in there 
so I'm being more thorough with testing on other arches.

Andrew, the option is part of the zone-based solution to fragmentation 
avoidance which hasn't been mentioned in a while. That's probably why it 
fell off your radar. It's nasty looking because sizing the portion of 
memory usable by any allocation so that it is evenly spread throughout all 
nodes is not trivial. I think that having mem= parsing broken on NUMA at 
various times in the past (and probably still broken) highlights that.

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

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Mel Gorman]

> > I wasn't even aware of this kernelcore thing.  It's pretty nasty-looking. 
> > yet another reminder that this code hasn't been properly reviewed in the
> > past year or three.
> 
> Just now, I'm making memory-unplug patches with current MOVABLE_ZONE
> code. So, I might be the first user of it on ia64.
> 
> Anyway, I'll try to fix it.
> 

Can you review this patch and see does it fix the problem please? There
was a second problem that showed up while testing this in relation to the
bootmem allocator assumptions about zone boundary alignment. I'll follow up
this mail with the patch in case you are seeing that problem.

Subject: Fix parsing kernelcore boot option V2
cmdline_parse_kernelcore() should return the next pointer of boot option
like memparse() doing. If not, it is cause of eternal loop on ia64 box.
This patch is for 2.6.21-rc6-mm1. This patch changes the kernelcore command
line parsing so that is compatible with both early_param() way of doing
things and IA64.

[y-goto@jp.fujitsu.com: Fix parsing kernelcore boot option V1]
Signed-off-by: Mel Gorman <mel@csn.ul.ie>

diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-001_latest/arch/i386/kernel/setup.c linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/i386/kernel/setup.c
--- linux-2.6.21-rc6-mm1-001_latest/arch/i386/kernel/setup.c	2007-04-17 14:31:59.000000000 +0100
+++ linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/i386/kernel/setup.c	2007-04-23 11:08:20.000000000 +0100
@@ -195,7 +195,7 @@ static int __init parse_mem(char *arg)
 	return 0;
 }
 early_param("mem", parse_mem);
-early_param("kernelcore", cmdline_parse_kernelcore);
+early_param("kernelcore", early_parse_kernelcore);
 
 #ifdef CONFIG_PROC_VMCORE
 /* elfcorehdr= specifies the location of elf core header
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-001_latest/arch/ia64/kernel/efi.c linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/ia64/kernel/efi.c
--- linux-2.6.21-rc6-mm1-001_latest/arch/ia64/kernel/efi.c	2007-04-23 10:46:27.000000000 +0100
+++ linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/ia64/kernel/efi.c	2007-04-23 11:08:43.000000000 +0100
@@ -423,7 +423,7 @@ efi_init (void)
 		} else if (memcmp(cp, "max_addr=", 9) == 0) {
 			max_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
 		} else if (memcmp(cp, "kernelcore=",11) == 0) {
-			cmdline_parse_kernelcore(cp+11);
+			parse_kernelcore(cp+11, &cp);
 		} else if (memcmp(cp, "min_addr=", 9) == 0) {
 			min_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
 		} else {
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-001_latest/arch/powerpc/kernel/prom.c linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/powerpc/kernel/prom.c
--- linux-2.6.21-rc6-mm1-001_latest/arch/powerpc/kernel/prom.c	2007-04-17 14:31:59.000000000 +0100
+++ linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/powerpc/kernel/prom.c	2007-04-23 11:11:54.000000000 +0100
@@ -431,7 +431,7 @@ static int __init early_parse_mem(char *
 	return 0;
 }
 early_param("mem", early_parse_mem);
-early_param("kernelcore", cmdline_parse_kernelcore);
+early_param("kernelcore", early_parse_kernelcore);
 
 /*
  * The device tree may be allocated below our memory limit, or inside the
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-001_latest/arch/ppc/mm/init.c linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/ppc/mm/init.c
--- linux-2.6.21-rc6-mm1-001_latest/arch/ppc/mm/init.c	2007-04-17 14:31:59.000000000 +0100
+++ linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/ppc/mm/init.c	2007-04-23 11:08:58.000000000 +0100
@@ -214,7 +214,7 @@ void MMU_setup(void)
 	}
 }
 
-early_param("kernelcore", cmdline_parse_kernelcore);
+early_param("kernelcore", early_parse_kernelcore);
 
 /*
  * MMU_init sets up the basic memory mappings for the kernel,
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-001_latest/arch/x86_64/kernel/e820.c linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/x86_64/kernel/e820.c
--- linux-2.6.21-rc6-mm1-001_latest/arch/x86_64/kernel/e820.c	2007-04-17 14:32:00.000000000 +0100
+++ linux-2.6.21-rc6-mm1-002_ia64cmdline/arch/x86_64/kernel/e820.c	2007-04-23 11:09:09.000000000 +0100
@@ -604,7 +604,7 @@ static int __init parse_memopt(char *p)
 	return 0;
 } 
 early_param("mem", parse_memopt);
-early_param("kernelcore", cmdline_parse_kernelcore);
+early_param("kernelcore", early_parse_kernelcore);
 
 static int userdef __initdata;
 
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-001_latest/include/linux/mm.h linux-2.6.21-rc6-mm1-002_ia64cmdline/include/linux/mm.h
--- linux-2.6.21-rc6-mm1-001_latest/include/linux/mm.h	2007-04-23 10:46:27.000000000 +0100
+++ linux-2.6.21-rc6-mm1-002_ia64cmdline/include/linux/mm.h	2007-04-23 11:04:07.000000000 +0100
@@ -1051,7 +1051,8 @@ extern unsigned long find_max_pfn_with_a
 extern void free_bootmem_with_active_regions(int nid,
 						unsigned long max_low_pfn);
 extern void sparse_memory_present_with_active_regions(int nid);
-extern int cmdline_parse_kernelcore(char *p);
+extern int parse_kernelcore(char *p, char **retp);
+extern int early_parse_kernelcore(char *p);
 #ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
 extern int early_pfn_to_nid(unsigned long pfn);
 #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-001_latest/mm/page_alloc.c linux-2.6.21-rc6-mm1-002_ia64cmdline/mm/page_alloc.c
--- linux-2.6.21-rc6-mm1-001_latest/mm/page_alloc.c	2007-04-23 10:46:27.000000000 +0100
+++ linux-2.6.21-rc6-mm1-002_ia64cmdline/mm/page_alloc.c	2007-04-23 12:27:32.000000000 +0100
@@ -3728,13 +3728,13 @@ void __init free_area_init_nodes(unsigne
  * kernelcore=size sets the amount of memory for use for allocations that
  * cannot be reclaimed or migrated.
  */
-int __init cmdline_parse_kernelcore(char *p)
+int __init parse_kernelcore(char *p, char **retp)
 {
 	unsigned long long coremem;
 	if (!p)
 		return -EINVAL;
 
-	coremem = memparse(p, &p);
+	coremem = memparse(p, retp);
 	required_kernelcore = coremem >> PAGE_SHIFT;
 
 	/* Paranoid check that UL is enough for required_kernelcore */
@@ -3742,6 +3742,12 @@ int __init cmdline_parse_kernelcore(char
 
 	return 0;
 }
+
+/* For use with early_param where available */
+int __init early_parse_kernelcore(char *p)
+{
+	return parse_kernelcore(p, &p);
+}
 #endif /* CONFIG_ARCH_POPULATES_NODE_MAP */
 
 /**

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Mel Gorman]

On (23/04/07 19:32), Mel Gorman didst pronounce:
> There
> was a second problem that showed up while testing this in relation to the
> bootmem allocator assumptions about zone boundary alignment. I'll follow up
> this mail with the patch in case you are seeing that problem.
> 

Tony Luck added to cc list as I believe he had a lot to do with the speed
improvements within the bootmem allocator and might suggest a better fix.



Subject: Check zone boundaries when freeing bootmem
Zone boundaries do not have to be aligned to MAX_ORDER_NR_PAGES. However,
during boot, there is an implicit assumption that they are aligned to a
BITS_PER_LONG boundary when freeing pages as quickly as possible. This
patch checks the zone boundaries when freeing pages from the bootmem allocator.

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

diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-002_ia64cmdline/mm/bootmem.c linux-2.6.21-rc6-mm1-003_bootmem_zonecheck/mm/bootmem.c
--- linux-2.6.21-rc6-mm1-002_ia64cmdline/mm/bootmem.c	2007-04-06 03:36:56.000000000 +0100
+++ linux-2.6.21-rc6-mm1-003_bootmem_zonecheck/mm/bootmem.c	2007-04-23 18:58:10.000000000 +0100
@@ -302,13 +302,15 @@ found:
 
 static unsigned long __init free_all_bootmem_core(pg_data_t *pgdat)
 {
-	struct page *page;
+	struct page *page = NULL, *end_page = NULL;
 	unsigned long pfn;
 	bootmem_data_t *bdata = pgdat->bdata;
 	unsigned long i, count, total = 0;
 	unsigned long idx;
 	unsigned long *map; 
 	int gofast = 0;
+	int gofast_order = ffs(BITS_PER_LONG) - 1;
+	int gofast_size = 1 << gofast_order;
 
 	BUG_ON(!bdata->node_bootmem_map);
 
@@ -324,19 +326,21 @@ static unsigned long __init free_all_boo
 	for (i = 0; i < idx; ) {
 		unsigned long v = ~map[i / BITS_PER_LONG];
 
-		if (gofast && v == ~0UL) {
-			int order;
-
+		if (v) {
 			page = pfn_to_page(pfn);
+			end_page = page + gofast_size - 1;
+		}
+
+		/* Do not cross zone boundaries when fast freeing */
+		if (gofast && v == ~0UL &&
+				page_zone_id(page) == page_zone_id(end_page)) {
 			count += BITS_PER_LONG;
-			order = ffs(BITS_PER_LONG) - 1;
-			__free_pages_bootmem(page, order);
+			__free_pages_bootmem(page, gofast_order);
 			i += BITS_PER_LONG;
 			page += BITS_PER_LONG;
 		} else if (v) {
 			unsigned long m;
 
-			page = pfn_to_page(pfn);
 			for (m = 1; m && i < idx; m<<=1, page++, i++) {
 				if (v & m) {
 					count++;

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Yasunori Goto]

> Subject: Check zone boundaries when freeing bootmem
> Zone boundaries do not have to be aligned to MAX_ORDER_NR_PAGES. 

Hmm. I don't understand here yet... Could you explain more? 

This issue occurs only when ZONE_MOVABLE is specified.
If its boundary is aligned to MAX_ORDER automatically,
I guess user will not mind it.

>From memory hotplug view, I prefer section size alignment to make
simple code. :-P


> However,
> during boot, there is an implicit assumption that they are aligned to a
> BITS_PER_LONG boundary when freeing pages as quickly as possible. This
> patch checks the zone boundaries when freeing pages from the bootmem allocator.

Anyway, the patch works well.

Bye.

-- 
Yasunori Goto 

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Mel Gorman]

On Tue, 24 Apr 2007, Yasunori Goto wrote:

>
>
>> Subject: Check zone boundaries when freeing bootmem
>> Zone boundaries do not have to be aligned to MAX_ORDER_NR_PAGES.
>
> Hmm. I don't understand here yet... Could you explain more?
>

Nodes are required to be MAX_ORDER_NR_PAGES aligned for the buddy 
algorithm to work but zones can be at any alignment because the 
page_is_buddy() check checks the zone_id of two buddies when merging. As 
zones are generally aligned anyway, it was never noticed that the bootmem 
allocators assumes zones are at least order-5 aligned on 32 bit and 
order-6 aligned on 64 bit.

> This issue occurs only when ZONE_MOVABLE is specified.

Yes, because it can be sized to any value. At the moment, zones are 
aligned to MAX_ORDER_NR_PAGES so it was not noticed that bootmem makes 
assumptions on zone alignment.

> If its boundary is aligned to MAX_ORDER automatically,
> I guess user will not mind it.
>

Probably not. They will get a different amount of memory usable by the 
kernel than they asked for but it doesn't really matter. Huge pages 
generally need MAX_ORDER_NR_PAGES base pages as well so the alignment 
doesn't hurt there.

> From memory hotplug view, I prefer section size alignment to make
> simple code. :-P
>

That's fair. I'll roll up a patch that aligns to MAX_ORDER_NR_PAGES to 
begin with and then decide if it should align to section size on SPARSEMEM 
or not.

>
>> However,
>> during boot, there is an implicit assumption that they are aligned to a
>> BITS_PER_LONG boundary when freeing pages as quickly as possible. This
>> patch checks the zone boundaries when freeing pages from the bootmem allocator.
>
> Anyway, the patch works well.
>

Right, I'll resend it to linux-mm as a standalone patch later so because 
it fixes a correctness issue albeit one that is easily avoided.

> Bye.
>

Thanks

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

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Mel Gorman]

On (24/04/07 18:58), Yasunori Goto didst pronounce:
> 
> 
> > Subject: Check zone boundaries when freeing bootmem
> > Zone boundaries do not have to be aligned to MAX_ORDER_NR_PAGES. 
> 
> Hmm. I don't understand here yet... Could you explain more? 
> 
> This issue occurs only when ZONE_MOVABLE is specified.
> If its boundary is aligned to MAX_ORDER automatically,
> I guess user will not mind it.
> 
> From memory hotplug view, I prefer section size alignment to make
> simple code. :-P
 
How does this look to you?


======
The two potential users of ZONE_MOVABLE would prefer if the zone was aligned
to a boundary. This patch aligns the zone to a MAX_ORDER_NR_PAGES boundary.

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

diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.21-rc6-mm1-002_commonparse/mm/page_alloc.c linux-2.6.21-rc6-mm1-003_alignmovable/mm/page_alloc.c
--- linux-2.6.21-rc6-mm1-002_commonparse/mm/page_alloc.c	2007-04-24 09:38:30.000000000 +0100
+++ linux-2.6.21-rc6-mm1-003_alignmovable/mm/page_alloc.c	2007-04-24 11:15:40.000000000 +0100
@@ -3642,6 +3642,11 @@ restart:
 	usable_nodes--;
 	if (usable_nodes && required_kernelcore > usable_nodes)
 		goto restart;
+	
+	/* Align start of ZONE_MOVABLE on all nids to MAX_ORDER_NR_PAGES */
+	for (nid = 0; nid < MAX_NUMNODES; nid++)
+		zone_movable_pfn[nid] =
+			roundup(zone_movable_pfn[nid], MAX_ORDER_NR_PAGES);
 }
 
 /**
-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[KAMEZAWA Hiroyuki]

On Mon, 23 Apr 2007 19:32:46 +0100
mel@skynet.ie (Mel Gorman) wrote:

> > > I wasn't even aware of this kernelcore thing.  It's pretty nasty-looking. 
> > > yet another reminder that this code hasn't been properly reviewed in the
> > > past year or three.
> > 
> > Just now, I'm making memory-unplug patches with current MOVABLE_ZONE
> > code. So, I might be the first user of it on ia64.
> > 
> > Anyway, I'll try to fix it.
> > 
> 
> Can you review this patch and see does it fix the problem please? There
> was a second problem that showed up while testing this in relation to the
> bootmem allocator assumptions about zone boundary alignment. I'll follow up
> this mail with the patch in case you are seeing that problem.
> 
> Subject: Fix parsing kernelcore boot option V2
> cmdline_parse_kernelcore() should return the next pointer of boot option
> like memparse() doing. If not, it is cause of eternal loop on ia64 box.
> This patch is for 2.6.21-rc6-mm1. This patch changes the kernelcore command
> line parsing so that is compatible with both early_param() way of doing
> things and IA64.
> 
In my understanding, why ia64 doesn't use early_param() macro for mem= at el. is that 
it has to use mem= option at efi handling which is called before parse_early_param().

Current ia64's boot path is
 setup_arch()
    -> efi handling -> parse_early_param() -> numa handling -> pgdat/zone init

kernelcore= option is just used at pgdat/zone initialization. (no arch dependent part...)

So I think just adding
==
early_param("kernelcore",cmpdline_parse_kernelcore)
==
to ia64 is ok.....

-Kame

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Yasunori Goto]

Mel-san.

I tested your patch (Thanks!). It worked. But......

> In my understanding, why ia64 doesn't use early_param() macro for mem= at el. is that 
> it has to use mem= option at efi handling which is called before parse_early_param().
> 
> Current ia64's boot path is
>  setup_arch()
>     -> efi handling -> parse_early_param() -> numa handling -> pgdat/zone init
> 
> kernelcore= option is just used at pgdat/zone initialization. (no arch dependent part...)
> 
> So I think just adding
> ==
> early_param("kernelcore",cmpdline_parse_kernelcore)
> ==
> to ia64 is ok.....

Then, it can be common code.
How is this patch? I confirmed this can work well too.

--------

When "kernelcore" boot option is specified, kernel can't boot up
on ia64. It is cause of eternal loop.
In addition, its code can be common code. This is fix for it.
I tested this patch on my ia64 box.


Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>

---------

 arch/i386/kernel/setup.c   |    1 -
 arch/ia64/kernel/efi.c     |    2 --
 arch/powerpc/kernel/prom.c |    1 -
 arch/ppc/mm/init.c         |    2 --
 arch/x86_64/kernel/e820.c  |    1 -
 include/linux/mm.h         |    1 -
 mm/page_alloc.c            |    3 +++
 7 files changed, 3 insertions(+), 8 deletions(-)

Index: kernelcore/arch/ia64/kernel/efi.c
===================================================================
--- kernelcore.orig/arch/ia64/kernel/efi.c	2007-04-24 15:09:37.000000000 +0900
+++ kernelcore/arch/ia64/kernel/efi.c	2007-04-24 15:25:22.000000000 +0900
@@ -423,8 +423,6 @@ efi_init (void)
 			mem_limit = memparse(cp + 4, &cp);
 		} else if (memcmp(cp, "max_addr=", 9) == 0) {
 			max_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
-		} else if (memcmp(cp, "kernelcore=",11) == 0) {
-			cmdline_parse_kernelcore(cp+11);
 		} else if (memcmp(cp, "min_addr=", 9) == 0) {
 			min_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
 		} else {
Index: kernelcore/arch/i386/kernel/setup.c
===================================================================
--- kernelcore.orig/arch/i386/kernel/setup.c	2007-04-24 15:29:20.000000000 +0900
+++ kernelcore/arch/i386/kernel/setup.c	2007-04-24 15:29:39.000000000 +0900
@@ -195,7 +195,6 @@ static int __init parse_mem(char *arg)
 	return 0;
 }
 early_param("mem", parse_mem);
-early_param("kernelcore", cmdline_parse_kernelcore);
 
 #ifdef CONFIG_PROC_VMCORE
 /* elfcorehdr= specifies the location of elf core header
Index: kernelcore/arch/powerpc/kernel/prom.c
===================================================================
--- kernelcore.orig/arch/powerpc/kernel/prom.c	2007-04-24 15:04:47.000000000 +0900
+++ kernelcore/arch/powerpc/kernel/prom.c	2007-04-24 15:30:25.000000000 +0900
@@ -431,7 +431,6 @@ static int __init early_parse_mem(char *
 	return 0;
 }
 early_param("mem", early_parse_mem);
-early_param("kernelcore", cmdline_parse_kernelcore);
 
 /*
  * The device tree may be allocated below our memory limit, or inside the
Index: kernelcore/arch/ppc/mm/init.c
===================================================================
--- kernelcore.orig/arch/ppc/mm/init.c	2007-04-24 15:04:47.000000000 +0900
+++ kernelcore/arch/ppc/mm/init.c	2007-04-24 15:30:56.000000000 +0900
@@ -214,8 +214,6 @@ void MMU_setup(void)
 	}
 }
 
-early_param("kernelcore", cmdline_parse_kernelcore);
-
 /*
  * MMU_init sets up the basic memory mappings for the kernel,
  * including both RAM and possibly some I/O regions,
Index: kernelcore/arch/x86_64/kernel/e820.c
===================================================================
--- kernelcore.orig/arch/x86_64/kernel/e820.c	2007-04-24 15:04:47.000000000 +0900
+++ kernelcore/arch/x86_64/kernel/e820.c	2007-04-24 15:34:02.000000000 +0900
@@ -604,7 +604,6 @@ static int __init parse_memopt(char *p)
 	return 0;
 } 
 early_param("mem", parse_memopt);
-early_param("kernelcore", cmdline_parse_kernelcore);
 
 static int userdef __initdata;
 
Index: kernelcore/include/linux/mm.h
===================================================================
--- kernelcore.orig/include/linux/mm.h	2007-04-24 15:09:37.000000000 +0900
+++ kernelcore/include/linux/mm.h	2007-04-24 15:35:52.000000000 +0900
@@ -1051,7 +1051,6 @@ extern unsigned long find_max_pfn_with_a
 extern void free_bootmem_with_active_regions(int nid,
 						unsigned long max_low_pfn);
 extern void sparse_memory_present_with_active_regions(int nid);
-extern int cmdline_parse_kernelcore(char *p);
 #ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
 extern int early_pfn_to_nid(unsigned long pfn);
 #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
Index: kernelcore/mm/page_alloc.c
===================================================================
--- kernelcore.orig/mm/page_alloc.c	2007-04-24 15:09:37.000000000 +0900
+++ kernelcore/mm/page_alloc.c	2007-04-24 16:00:21.000000000 +0900
@@ -3728,6 +3728,9 @@ int __init cmdline_parse_kernelcore(char
 
 	return 0;
 }
+
+early_param("kernelcore", cmdline_parse_kernelcore);
+
 #endif /* CONFIG_ARCH_POPULATES_NODE_MAP */
 
 /**


-- 
Yasunori Goto 

Re: [PATCH]Fix parsing kernelcore boot option for ia64

[Mel Gorman]

On (24/04/07 16:31), Yasunori Goto didst pronounce:
> Mel-san.
> 
> I tested your patch (Thanks!). It worked. But......
> 
> > In my understanding, why ia64 doesn't use early_param() macro for mem= at el. is that 
> > it has to use mem= option at efi handling which is called before parse_early_param().
> > 
> > Current ia64's boot path is
> >  setup_arch()
> >     -> efi handling -> parse_early_param() -> numa handling -> pgdat/zone init
> > 
> > kernelcore= option is just used at pgdat/zone initialization. (no arch dependent part...)
> > 
> > So I think just adding
> > ==
> > early_param("kernelcore",cmpdline_parse_kernelcore)
> > ==
> > to ia64 is ok.....

It should be ok. I had missed that IA64 supports early_param now and
handling this in common code makes sense.

> 
> Then, it can be common code.
> How is this patch? I confirmed this can work well too.
> 

This worked fine from a command-line perspective on most test machines.
In combination with the align-zone_movable-to-maxordernrpages-boundary
patch, x86_64, ppc64 and IA64 test machines all booted successfully with
and without kernelcore= specified.

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

I'll resend the two relevant patches for this problem with an explanation
leader mail and cc linux-mm. Thanks for helping sort this out.

> --------
> 
> When "kernelcore" boot option is specified, kernel can't boot up
> on ia64. It is cause of eternal loop.
> In addition, its code can be common code. This is fix for it.
> I tested this patch on my ia64 box.
> 
> 
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> 
> ---------
> 
>  arch/i386/kernel/setup.c   |    1 -
>  arch/ia64/kernel/efi.c     |    2 --
>  arch/powerpc/kernel/prom.c |    1 -
>  arch/ppc/mm/init.c         |    2 --
>  arch/x86_64/kernel/e820.c  |    1 -
>  include/linux/mm.h         |    1 -
>  mm/page_alloc.c            |    3 +++
>  7 files changed, 3 insertions(+), 8 deletions(-)
> 
> Index: kernelcore/arch/ia64/kernel/efi.c
> ===================================================================
> --- kernelcore.orig/arch/ia64/kernel/efi.c	2007-04-24 15:09:37.000000000 +0900
> +++ kernelcore/arch/ia64/kernel/efi.c	2007-04-24 15:25:22.000000000 +0900
> @@ -423,8 +423,6 @@ efi_init (void)
>  			mem_limit = memparse(cp + 4, &cp);
>  		} else if (memcmp(cp, "max_addr=", 9) == 0) {
>  			max_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
> -		} else if (memcmp(cp, "kernelcore=",11) == 0) {
> -			cmdline_parse_kernelcore(cp+11);
>  		} else if (memcmp(cp, "min_addr=", 9) == 0) {
>  			min_addr = GRANULEROUNDDOWN(memparse(cp + 9, &cp));
>  		} else {
> Index: kernelcore/arch/i386/kernel/setup.c
> ===================================================================
> --- kernelcore.orig/arch/i386/kernel/setup.c	2007-04-24 15:29:20.000000000 +0900
> +++ kernelcore/arch/i386/kernel/setup.c	2007-04-24 15:29:39.000000000 +0900
> @@ -195,7 +195,6 @@ static int __init parse_mem(char *arg)
>  	return 0;
>  }
>  early_param("mem", parse_mem);
> -early_param("kernelcore", cmdline_parse_kernelcore);
>  
>  #ifdef CONFIG_PROC_VMCORE
>  /* elfcorehdr= specifies the location of elf core header
> Index: kernelcore/arch/powerpc/kernel/prom.c
> ===================================================================
> --- kernelcore.orig/arch/powerpc/kernel/prom.c	2007-04-24 15:04:47.000000000 +0900
> +++ kernelcore/arch/powerpc/kernel/prom.c	2007-04-24 15:30:25.000000000 +0900
> @@ -431,7 +431,6 @@ static int __init early_parse_mem(char *
>  	return 0;
>  }
>  early_param("mem", early_parse_mem);
> -early_param("kernelcore", cmdline_parse_kernelcore);
>  
>  /*
>   * The device tree may be allocated below our memory limit, or inside the
> Index: kernelcore/arch/ppc/mm/init.c
> ===================================================================
> --- kernelcore.orig/arch/ppc/mm/init.c	2007-04-24 15:04:47.000000000 +0900
> +++ kernelcore/arch/ppc/mm/init.c	2007-04-24 15:30:56.000000000 +0900
> @@ -214,8 +214,6 @@ void MMU_setup(void)
>  	}
>  }
>  
> -early_param("kernelcore", cmdline_parse_kernelcore);
> -
>  /*
>   * MMU_init sets up the basic memory mappings for the kernel,
>   * including both RAM and possibly some I/O regions,
> Index: kernelcore/arch/x86_64/kernel/e820.c
> ===================================================================
> --- kernelcore.orig/arch/x86_64/kernel/e820.c	2007-04-24 15:04:47.000000000 +0900
> +++ kernelcore/arch/x86_64/kernel/e820.c	2007-04-24 15:34:02.000000000 +0900
> @@ -604,7 +604,6 @@ static int __init parse_memopt(char *p)
>  	return 0;
>  } 
>  early_param("mem", parse_memopt);
> -early_param("kernelcore", cmdline_parse_kernelcore);
>  
>  static int userdef __initdata;
>  
> Index: kernelcore/include/linux/mm.h
> ===================================================================
> --- kernelcore.orig/include/linux/mm.h	2007-04-24 15:09:37.000000000 +0900
> +++ kernelcore/include/linux/mm.h	2007-04-24 15:35:52.000000000 +0900
> @@ -1051,7 +1051,6 @@ extern unsigned long find_max_pfn_with_a
>  extern void free_bootmem_with_active_regions(int nid,
>  						unsigned long max_low_pfn);
>  extern void sparse_memory_present_with_active_regions(int nid);
> -extern int cmdline_parse_kernelcore(char *p);
>  #ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
>  extern int early_pfn_to_nid(unsigned long pfn);
>  #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> Index: kernelcore/mm/page_alloc.c
> ===================================================================
> --- kernelcore.orig/mm/page_alloc.c	2007-04-24 15:09:37.000000000 +0900
> +++ kernelcore/mm/page_alloc.c	2007-04-24 16:00:21.000000000 +0900
> @@ -3728,6 +3728,9 @@ int __init cmdline_parse_kernelcore(char
>  
>  	return 0;
>  }
> +
> +early_param("kernelcore", cmdline_parse_kernelcore);
> +
>  #endif /* CONFIG_ARCH_POPULATES_NODE_MAP */
>  
>  /**
> 
> 
> -- 
> Yasunori Goto 
> 

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