[PATCH] sparsemem vmemmap: initialize memmap.

[PATCH] sparsemem vmemmap: initialize memmap.

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Trying to online a new memory section that was added via memory hotplug
results in lots of messages of pages in bad page state.
Reason is that the alloacted virtual memmap isn't initialized.
This is only an issue for memory sections that get added after boot
time since for all other memmaps the bootmem allocator was used which
returns only initialized memory.

I noticed this on s390 which has its private vmemmap_populate function
without using callbacks to the common code. But as far as I can see the
generic code has the same bug, so fix it just once.

Cc: Andy Whitcroft <apw@shadowen.org>
Cc: Christoph Lameter <clameter@sgi.com>
Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 mm/sparse-vmemmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/mm/sparse-vmemmap.c
===================================================================
--- linux-2.6.orig/mm/sparse-vmemmap.c
+++ linux-2.6/mm/sparse-vmemmap.c
@@ -154,6 +154,6 @@ struct page * __meminit sparse_mem_map_p
 	int error = vmemmap_populate(map, PAGES_PER_SECTION, nid);
 	if (error)
 		return NULL;
-
+	memset(map, 0, PAGES_PER_SECTION * sizeof(struct page));
 	return map;
 }

Re: [PATCH] sparsemem vmemmap: initialize memmap.

[Andy Whitcroft]

On Fri, May 09, 2008 at 08:38:56AM +0200, Heiko Carstens wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Trying to online a new memory section that was added via memory hotplug
> results in lots of messages of pages in bad page state.
> Reason is that the alloacted virtual memmap isn't initialized.
> This is only an issue for memory sections that get added after boot
> time since for all other memmaps the bootmem allocator was used which
> returns only initialized memory.
> 
> I noticed this on s390 which has its private vmemmap_populate function
> without using callbacks to the common code. But as far as I can see the
> generic code has the same bug, so fix it just once.
> 
> Cc: Andy Whitcroft <apw@shadowen.org>
> Cc: Christoph Lameter <clameter@sgi.com>
> Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  mm/sparse-vmemmap.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/mm/sparse-vmemmap.c
> ===================================================================
> --- linux-2.6.orig/mm/sparse-vmemmap.c
> +++ linux-2.6/mm/sparse-vmemmap.c
> @@ -154,6 +154,6 @@ struct page * __meminit sparse_mem_map_p
>  	int error = vmemmap_populate(map, PAGES_PER_SECTION, nid);
>  	if (error)
>  		return NULL;
> -
> +	memset(map, 0, PAGES_PER_SECTION * sizeof(struct page));
>  	return map;
>  }

The normal expectation is that all allocations are made using
vmemmap_alloc_block() which allocates from the appropriate place.  Once
the buddy is up and available it uses:

	struct page *page = alloc_pages_node(node,
			GFP_KERNEL | __GFP_ZERO, get_order(size));

to get the memory so it should all be zero'd.  So I would expect all
existing users to be covered by that?  Can you not simply use __GFP_ZERO
for your allocations or use vmemmap_alloc_block() ?

-apw

Re: [PATCH] sparsemem vmemmap: initialize memmap.

[Heiko Carstens]

> > Index: linux-2.6/mm/sparse-vmemmap.c
> > ===================================================================
> > --- linux-2.6.orig/mm/sparse-vmemmap.c
> > +++ linux-2.6/mm/sparse-vmemmap.c
> > @@ -154,6 +154,6 @@ struct page * __meminit sparse_mem_map_p
> >  	int error = vmemmap_populate(map, PAGES_PER_SECTION, nid);
> >  	if (error)
> >  		return NULL;
> > -
> > +	memset(map, 0, PAGES_PER_SECTION * sizeof(struct page));
> >  	return map;
> >  }
> 
> The normal expectation is that all allocations are made using
> vmemmap_alloc_block() which allocates from the appropriate place.  Once
> the buddy is up and available it uses:
> 
> 	struct page *page = alloc_pages_node(node,
> 			GFP_KERNEL | __GFP_ZERO, get_order(size));
> 
> to get the memory so it should all be zero'd.  So I would expect all
> existing users to be covered by that?  Can you not simply use __GFP_ZERO
> for your allocations or use vmemmap_alloc_block() ?

Ah, I didn't notice the __GFP_ZERO. So it's just an s390 bug. Will
move the memset to our code instead.
Thanks!

Re: [PATCH] sparsemem vmemmap: initialize memmap.

[Christoph Lameter]

Nack. vmemmap_alloc_block does 

void * __meminit vmemmap_alloc_block(unsigned long size, int node)
{
        /* If the main allocator is up use that, fallback to bootmem. */
        if (slab_is_available()) {
                struct page *page = alloc_pages_node(node,
                                GFP_KERNEL | __GFP_ZERO, get_order(size));
                if (page)
                        return page_address(page);
                return NULL;
        } else
                return __earlyonly_bootmem_alloc(node, size, size,
                                __pa(MAX_DMA_ADDRESS));
}

memory is always zeroed. Maybe you use an alternate implementation that is 
broken?

Re: [PATCH] sparsemem vmemmap: initialize memmap.

[Heiko Carstens]

On Fri, May 09, 2008 at 09:25:41AM -0700, Christoph Lameter wrote:
> Nack. vmemmap_alloc_block does 
> 
> void * __meminit vmemmap_alloc_block(unsigned long size, int node)
> {
>         /* If the main allocator is up use that, fallback to bootmem. */
>         if (slab_is_available()) {
>                 struct page *page = alloc_pages_node(node,
>                                 GFP_KERNEL | __GFP_ZERO, get_order(size));
>                 if (page)
>                         return page_address(page);
>                 return NULL;
>         } else
>                 return __earlyonly_bootmem_alloc(node, size, size,
>                                 __pa(MAX_DMA_ADDRESS));
> }
> 
> memory is always zeroed. Maybe you use an alternate implementation that is 
> broken?

Yes, I missed the __GFP_ZERO. Already figured out and fixed in arch code.

Thanks ;)