PS3: Fix memory hotplug

A change was made to walk_memory_resource() in commit 4b119e21d0c66c22e8ca03df05d9de623d0eb50f that added a check of find_lmb(). Add the corresponding lmb_add() call to ps3_mm_add_memory() so that that check will succeed.

This fixes the condition where the PS3 boots up with just the 128 MiB of boot memory.

---

Paul,

Please send this one upstream at your earliest convenience.

-Geoff

 arch/powerpc/platforms/ps3/mm.c |    2 ++
 1 file changed, 2 insertions(+)

@@ -317,6 +317,8 @@ static int __init ps3_mm_add_memory(void
 		return result;
 	}
 
+	lmb_add(start_addr, map.r1.size);
+
 	result = online_pages(start_pfn, nr_pages);
 
 	if (result)

When you do an lmb_add you should probably also do an lmb_analyze to update the total memory count etc...

That leads to some interesting issues such as the LMB stuff wasn't really meant to be dynamically modified after boot, and thus the kernel has no locks in there. That can be an issue...

Paul, any thoughts here ? Should we add a lock ? That would mean being careful as the LMB stuff can be called very early, and spinlock wants things like PACA and possibly lockdep to be around..

Ben.

Actually, we call early_init_devtree(), which populates the LMB, after we initialize the PACA and lockdep, so it -should- be safe to add a spinlock.

Dave ? Any objection to adding a spinlock to the LMB code ?

Cheers,
Ben.

From: Benjamin Herrenschmidt <[email protected]>
Date: Wed, 14 May 2008 23:51:29 -0700

No objections, just don't add any bugs. :-)

Add a spinlock to struct lmb to enforce concurrency in lmb_add(), lmb_remove(), lmb_analyze(), and lmb_dump_all().

This locking is needed for SMP systems that access the lmb structure during hot memory add and remove operations after secondary cpus have been started.

---

This patch just adds locks for the few lmb routines that would be used for hot memory adding and removing.

-Geoff

 include/linux/lmb.h |    1 
 lib/lmb.c           |   54 +++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 42 insertions(+), 13 deletions(-)

@@ -30,6 +30,7 @@ struct lmb_region {
 };
 
 struct lmb {
+	spinlock_t lock;
 	unsigned long debug;
 	u64 rmo_size;
 	struct lmb_region memory;

Add a spinlock to struct lmb to enforce concurrency in lmb_add(), lmb_remove(), lmb_analyze(), lmb_find(), and lmb_dump_all().

This locking is needed for SMP systems that access the lmb structure during hot memory add and remove operations after secondary cpus have been started.

---

v2: o Add locking to lmb_find().

 include/linux/lmb.h |    1 
 lib/lmb.c           |   62 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 49 insertions(+), 14 deletions(-)

@@ -30,6 +30,7 @@ struct lmb_region {
 };
 
 struct lmb {
+	spinlock_t lock;
 	unsigned long debug;
 	u64 rmo_size;
 	struct lmb_region memory;

From: Geoff Levand <[email protected]>
Date: Mon, 19 May 2008 17:55:45 -0700

I'm not against this patch, but I'm pretty sure it's not necessary. Isn't memory hotplug already synchronized at a higher level?

If not, it should be.

I think the core memory hotplug is... However, we used to not change the LMB when doing so (afaik, I'm travelling and not looking at the code right now). However, things like PS3 memory hotplug tries to keep LMB is sync for the sake of /dev/mem or similar and that happens before the memory is added to the core.

Ben.

From: Benjamin Herrenschmidt <[email protected]>
Date: Mon, 19 May 2008 22:32:58 -0400

But if the memory hotplug is synchronized, so are changes to the LMB tables.

And if there are LMB read side access concernes outside of the hotplug event, ideally we should use the synchronization mechanism that hotplug uses instead of adding a new one.

Either that, or we give in and use iomem_resource to track where system RAM is, as well as the other things in the physical address space, like other architectures do...

Paul.

The generic hot plug routines already use iomem_resource (mm/memory_hotplug.c). Both __add_pages() and add_memory() add the new mem to iomem_resource, and so it seems there is no need for the powerpc specific walk_memory_resource(), since the generic one does its check with iomem_resource.

I need to look a little closer at how the pSeries does its memory hot plug, but I think removing the powerpc specific walk_memory_resource() won't effect pSeries since it seems to have its own hot plug routines that do their own thing entirely with lmb.

It doesn't seem that it would be difficult to make the pSeries hot plug code to use iomem_resource, but some of the generic hot plug routines cannot be called until fairly late in the startup.

The other thing to do then would be to change the other powerpc startup code to use iomem_resource instead of lmb.

-Geoff

A change was made to walk_memory_resource() in commit 4b119e21d0c66c22e8ca03df05d9de623d0eb50f that added a check of find_lmb(). Add the coresponding lmb_add() call to ps3_mm_add_memory() so that that check will succeed.

This fixes the condition where the PS3 boots up with only the 128 MiB of boot memory.

---

v2: Add call to lmb_analyze().

 arch/powerpc/platforms/ps3/mm.c |    3 +++
 1 file changed, 3 insertions(+)

@@ -317,6 +317,9 @@ static int __init ps3_mm_add_memory(void
 		return result;
 	}
 
+	lmb_add(start_addr, map.r1.size);
+	lmb_analyze();
+
 	result = online_pages(start_pfn, nr_pages);
 
 	if (result)