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

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

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/