Re: [PATCH] sparsemem interaction with memory add bug fixes

[jschopp]

Andy Whitcroft wrote:
> Mike Kravetz wrote:
> > This patch fixes two bugs with the way sparsemem interacts with memory
> > add.  They are:
> > - memory leak if memmap for section already exists
> > - calling alloc_bootmem_node() after boot
> > These bugs were discovered and a first cut at the fixes were provided by
> > Arnd Bergmann <arnd@xxxxxxxx> and Joel Schopp <jschopp@xxxxxxxxxx>.
> >
> > Signed-off-by: Mike Kravetz <kravetz@xxxxxxxxxx>
> >
> > diff -Naupr linux-2.6.17-rc1-mm2/mm/sparse.c linux-2.6.17-rc1-mm2.work/mm/sparse.c
> > --- linux-2.6.17-rc1-mm2/mm/sparse.c	2006-04-03 03:22:10.000000000 +0000
> > +++ linux-2.6.17-rc1-mm2.work/mm/sparse.c	2006-04-11 23:32:10.000000000 +0000
> > @@ -32,7 +32,10 @@ static struct mem_section *sparse_index_
> >  	unsigned long array_size = SECTIONS_PER_ROOT *
> >  				   sizeof(struct mem_section);
> >  
> > -	section = alloc_bootmem_node(NODE_DATA(nid), array_size);
> > +	if (system_state == SYSTEM_RUNNING)
> > +		section = kmalloc_node(array_size, GFP_KERNEL, nid);
> > +	else
> > +		section = alloc_bootmem_node(NODE_DATA(nid), array_size);
> >  
> >  	if (section)
> >  		memset(section, 0, array_size);
> > @@ -281,9 +284,9 @@ int sparse_add_one_section(struct zone *
> >  
> >  	ret = sparse_init_one_section(ms, section_nr, memmap);
> >  
> > +out:
> >  	if (ret <= 0)
> >  		__kfree_section_memmap(memmap, nr_pages);
> > -out:
> >  	pgdat_resize_unlock(pgdat, &flags);
> >  	return ret;
> >  }
>
> First change looks sane.  For the second it makes it obvious that we are
> freeing the alloc'd section within the pgdat resize lock.  Doesn't seem
> to make any sense to do that to me?  Perhaps it should be more like the
> attached.
>
> -apw
>
>
>
> ------------------------------------------------------------------------
>
>  sparse.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> diff -upN reference/mm/sparse.c current/mm/sparse.c
> --- reference/mm/sparse.c
> +++ current/mm/sparse.c
> @@ -281,9 +281,9 @@ int sparse_add_one_section(struct zone *
>  
>  	ret = sparse_init_one_section(ms, section_nr, memmap);
>  
> -	if (ret <= 0)
> -		__kfree_section_memmap(memmap, nr_pages);
>  out:
>  	pgdat_resize_unlock(pgdat, &flags);
> +	if (ret <= 0)
> +		__kfree_section_memmap(memmap, nr_pages);
>  	return ret;
>  }

Whatever, I don't really care.  It doesn't matter functionally if we free under the 
pgdat_resize lock or not.  This looks fine, as does the original patch.  If resizing 
pgdats was a hot path we might prefer this way outside the lock.

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