Re: [PATCH] memory_hotplug: always initialize pageblock bitmap.

[Heiko Carstens]

On Fri, May 09, 2008 at 03:39:10PM +0900, KAMEZAWA Hiroyuki wrote:
> On Fri, 9 May 2008 08:06:09 +0200
> Heiko Carstens <heiko.carstens@xxxxxxxxxx> wrote:
> 
> > From: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
> > 
> > Trying to online a new memory section that was added via memory hotplug
> > sometimes results in crashes when the new pages are added via
> > __free_page. Reason for that is that the pageblock bitmap isn't
> > initialized and hence contains random stuff.
> 
> Hmm, curious. In my understanding, memmap_init_zone() initializes it.
> 
>  __add_pages()
> 	-> __add_section()
> 		-> sparse-add_one_section() // allocate usemap
> 		-> __add_zone()
> 			-> memmap_init_zone() // reset pageblock's bitmap 
> 
> Can't memmap_init_zone() does proper initialization ?
> ........................
> Ah, ok. I see. grow_zone_span() is not called at __add_zone(), then,
> memmap_init_zone() doesn't initialize usemap because memmap is not in zone's
> range.

Now I got it. So my patch was incorrect, it just worked around the crash.

> Recently, I added a check "zone's start_pfn < pfn < zone's end"
> to memmap_init_zone()'s usemap initialization for !SPARSEMEM case bug FIX.
> (and I think the fix itself is sane.)

Oh, you broke memory hot-add on -stable ;)

> How about calling grow_pgdat_span()/grow_zone_span() from __add_zone() ?

Like this?

Note that this patch is on top of the other patch I already sent, but
reverts it...

This works for me. A final version (if this is acceptable) should move
the grow* functions also.

---
 mm/memory_hotplug.c |   28 +++++++++++++++++++---------
 mm/page_alloc.c     |    3 +--
 2 files changed, 20 insertions(+), 11 deletions(-)

Index: linux-2.6/mm/memory_hotplug.c
===================================================================
--- linux-2.6.orig/mm/memory_hotplug.c
+++ linux-2.6/mm/memory_hotplug.c
@@ -159,17 +159,33 @@ void register_page_bootmem_info_node(str
 }
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */
 
+static void grow_zone_span(struct zone *zone, unsigned long start_pfn,
+			   unsigned long end_pfn);
+static void grow_pgdat_span(struct pglist_data *pgdat,
+			    unsigned long start_pfn, unsigned long end_pfn);
+
 static int __add_zone(struct zone *zone, unsigned long phys_start_pfn)
 {
 	struct pglist_data *pgdat = zone->zone_pgdat;
 	int nr_pages = PAGES_PER_SECTION;
 	int nid = pgdat->node_id;
 	int zone_type;
+	unsigned long flags;
 
 	zone_type = zone - pgdat->node_zones;
-	if (!zone->wait_table)
-		return init_currently_empty_zone(zone, phys_start_pfn,
-						 nr_pages, MEMMAP_HOTPLUG);
+	if (!zone->wait_table) {
+		int ret;
+
+		ret = init_currently_empty_zone(zone, phys_start_pfn,
+						nr_pages, MEMMAP_HOTPLUG);
+		if (ret)
+			return ret;
+	}
+	pgdat_resize_lock(zone->zone_pgdat, &flags);
+	grow_zone_span(zone, phys_start_pfn, phys_start_pfn + nr_pages);
+	grow_pgdat_span(zone->zone_pgdat, phys_start_pfn,
+			phys_start_pfn + nr_pages);
+	pgdat_resize_unlock(zone->zone_pgdat, &flags);
 	memmap_init_zone(nr_pages, nid, zone_type,
 			 phys_start_pfn, MEMMAP_HOTPLUG);
 	return 0;
@@ -363,7 +379,6 @@ static int online_pages_range(unsigned l
 
 int online_pages(unsigned long pfn, unsigned long nr_pages)
 {
-	unsigned long flags;
 	unsigned long onlined_pages = 0;
 	struct zone *zone;
 	int need_zonelists_rebuild = 0;
@@ -391,11 +406,6 @@ int online_pages(unsigned long pfn, unsi
 	 * memory_block->state_mutex.
 	 */
 	zone = page_zone(pfn_to_page(pfn));
-	pgdat_resize_lock(zone->zone_pgdat, &flags);
-	grow_zone_span(zone, pfn, pfn + nr_pages);
-	grow_pgdat_span(zone->zone_pgdat, pfn, pfn + nr_pages);
-	pgdat_resize_unlock(zone->zone_pgdat, &flags);
-
 	/*
 	 * If this zone is not populated, then it is not in zonelist.
 	 * This means the page allocator ignores this zone.
Index: linux-2.6/mm/page_alloc.c
===================================================================
--- linux-2.6.orig/mm/page_alloc.c
+++ linux-2.6/mm/page_alloc.c
@@ -2862,8 +2862,6 @@ __meminit int init_currently_empty_zone(
 
 	zone->zone_start_pfn = zone_start_pfn;
 
-	memmap_init(size, pgdat->node_id, zone_idx(zone), zone_start_pfn);
-
 	zone_init_free_lists(zone);
 
 	return 0;
@@ -3433,6 +3431,7 @@ static void __paginginit free_area_init_
 		ret = init_currently_empty_zone(zone, zone_start_pfn,
 						size, MEMMAP_EARLY);
 		BUG_ON(ret);
+		memmap_init(size, nid, j, zone_start_pfn);
 		zone_start_pfn += size;
 	}
 }
--
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/