linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* PS3: Fix memory hotplug
@ 2008-05-13 16:23 Geoff Levand
  2008-05-15  6:48 ` Benjamin Herrenschmidt
  2008-05-15 20:09 ` [patch v2] " Geoff Levand
  0 siblings, 2 replies; 13+ messages in thread
From: Geoff Levand @ 2008-05-13 16:23 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev@ozlabs.org

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.

Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---

Paul,

Please send this one upstream at your earliest convenience.

-Geoff

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

--- a/arch/powerpc/platforms/ps3/mm.c
+++ b/arch/powerpc/platforms/ps3/mm.c
@@ -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)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: PS3: Fix memory hotplug
  2008-05-13 16:23 PS3: Fix memory hotplug Geoff Levand
@ 2008-05-15  6:48 ` Benjamin Herrenschmidt
  2008-05-15  6:51   ` Benjamin Herrenschmidt
  2008-05-15  9:38   ` PS3: Fix memory hotplug Paul Mackerras
  2008-05-15 20:09 ` [patch v2] " Geoff Levand
  1 sibling, 2 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2008-05-15  6:48 UTC (permalink / raw)
  To: Geoff Levand; +Cc: linuxppc-dev@ozlabs.org, Paul Mackerras


On Tue, 2008-05-13 at 09:23 -0700, Geoff Levand wrote:
> 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.
> 
> Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
> ---

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.

> Paul,
> 
> Please send this one upstream at your earliest convenience.
> 
> -Geoff
> 
>  arch/powerpc/platforms/ps3/mm.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- a/arch/powerpc/platforms/ps3/mm.c
> +++ b/arch/powerpc/platforms/ps3/mm.c
> @@ -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)
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: PS3: Fix memory hotplug
  2008-05-15  6:48 ` Benjamin Herrenschmidt
@ 2008-05-15  6:51   ` Benjamin Herrenschmidt
  2008-05-15  7:02     ` David Miller
  2008-05-15  9:38   ` PS3: Fix memory hotplug Paul Mackerras
  1 sibling, 1 reply; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2008-05-15  6:51 UTC (permalink / raw)
  To: Geoff Levand; +Cc: linuxppc-dev@ozlabs.org, Paul Mackerras, David S. Miller


> 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.. 

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: PS3: Fix memory hotplug
  2008-05-15  6:51   ` Benjamin Herrenschmidt
@ 2008-05-15  7:02     ` David Miller
  2008-05-20  0:40       ` [rfc] [patch] LMB: Add basic spin locking to lmb Geoff Levand
  0 siblings, 1 reply; 13+ messages in thread
From: David Miller @ 2008-05-15  7:02 UTC (permalink / raw)
  To: benh; +Cc: linuxppc-dev, paulus

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed, 14 May 2008 23:51:29 -0700

> 
> > 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.. 
> 
> 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 ?

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: PS3: Fix memory hotplug
  2008-05-15  6:48 ` Benjamin Herrenschmidt
  2008-05-15  6:51   ` Benjamin Herrenschmidt
@ 2008-05-15  9:38   ` Paul Mackerras
  2008-05-21 18:31     ` Geoff Levand
  1 sibling, 1 reply; 13+ messages in thread
From: Paul Mackerras @ 2008-05-15  9:38 UTC (permalink / raw)
  To: benh; +Cc: linuxppc-dev@ozlabs.org

Benjamin Herrenschmidt writes:

> 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.. 

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [patch v2] PS3: Fix memory hotplug
  2008-05-13 16:23 PS3: Fix memory hotplug Geoff Levand
  2008-05-15  6:48 ` Benjamin Herrenschmidt
@ 2008-05-15 20:09 ` Geoff Levand
  2008-05-21 16:41   ` Geoff Levand
  1 sibling, 1 reply; 13+ messages in thread
From: Geoff Levand @ 2008-05-15 20:09 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev@ozlabs.org

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.

Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---

v2: Add call to lmb_analyze().

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

--- a/arch/powerpc/platforms/ps3/mm.c
+++ b/arch/powerpc/platforms/ps3/mm.c
@@ -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)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [rfc] [patch] LMB: Add basic spin locking to lmb
  2008-05-15  7:02     ` David Miller
@ 2008-05-20  0:40       ` Geoff Levand
  2008-05-20  0:55         ` [patch v2] " Geoff Levand
  0 siblings, 1 reply; 13+ messages in thread
From: Geoff Levand @ 2008-05-20  0:40 UTC (permalink / raw)
  To: David Miller, linux-kernel; +Cc: paulus, linuxppc-dev

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.

Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---

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(-)

--- a/include/linux/lmb.h
+++ b/include/linux/lmb.h
@@ -30,6 +30,7 @@ struct lmb_region {
 };
 
 struct lmb {
+	spinlock_t lock;
 	unsigned long debug;
 	u64 rmo_size;
 	struct lmb_region memory;
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -32,28 +32,33 @@ early_param("lmb", early_lmb);
 void lmb_dump_all(void)
 {
 	unsigned long i;
+	struct lmb tmp;
 
 	if (!lmb_debug)
 		return;
 
+	spin_lock(&lmb.lock);
+	tmp = lmb;
+	spin_unlock(&lmb.lock);
+
 	pr_info("lmb_dump_all:\n");
-	pr_info("    memory.cnt		  = 0x%lx\n", lmb.memory.cnt);
+	pr_info("    memory.cnt		  = 0x%lx\n", tmp.memory.cnt);
 	pr_info("    memory.size		  = 0x%llx\n",
-	    (unsigned long long)lmb.memory.size);
-	for (i=0; i < lmb.memory.cnt ;i++) {
+	    (unsigned long long)tmp.memory.size);
+	for (i=0; i < tmp.memory.cnt ;i++) {
 		pr_info("    memory.region[0x%lx].base       = 0x%llx\n",
-		    i, (unsigned long long)lmb.memory.region[i].base);
+		    i, (unsigned long long)tmp.memory.region[i].base);
 		pr_info("		      .size     = 0x%llx\n",
-		    (unsigned long long)lmb.memory.region[i].size);
+		    (unsigned long long)tmp.memory.region[i].size);
 	}
 
-	pr_info("    reserved.cnt	  = 0x%lx\n", lmb.reserved.cnt);
-	pr_info("    reserved.size	  = 0x%lx\n", lmb.reserved.size);
-	for (i=0; i < lmb.reserved.cnt ;i++) {
+	pr_info("    reserved.cnt	  = 0x%lx\n", tmp.reserved.cnt);
+	pr_info("    reserved.size	  = 0x%lx\n", tmp.reserved.size);
+	for (i=0; i < tmp.reserved.cnt ;i++) {
 		pr_info("    reserved.region[0x%lx].base       = 0x%llx\n",
-		    i, (unsigned long long)lmb.reserved.region[i].base);
+		    i, (unsigned long long)tmp.reserved.region[i].base);
 		pr_info("		      .size     = 0x%llx\n",
-		    (unsigned long long)lmb.reserved.region[i].size);
+		    (unsigned long long)tmp.reserved.region[i].size);
 	}
 }
 
@@ -105,6 +110,8 @@ static void lmb_coalesce_regions(struct 
 
 void __init lmb_init(void)
 {
+	spin_lock_init(&lmb.lock);
+
 	/* Create a dummy zero size LMB which will get coalesced away later.
 	 * This simplifies the lmb_add() code below...
 	 */
@@ -122,10 +129,14 @@ void __init lmb_analyze(void)
 {
 	int i;
 
+	spin_lock(&lmb.lock);
+
 	lmb.memory.size = 0;
 
 	for (i = 0; i < lmb.memory.cnt; i++)
 		lmb.memory.size += lmb.memory.region[i].size;
+
+	spin_unlock(&lmb.lock);
 }
 
 static long lmb_add_region(struct lmb_region *rgn, u64 base, u64 size)
@@ -194,18 +205,25 @@ static long lmb_add_region(struct lmb_re
 
 long lmb_add(u64 base, u64 size)
 {
+	long ret;
 	struct lmb_region *_rgn = &lmb.memory;
 
+	spin_lock(&lmb.lock);
+
 	/* On pSeries LPAR systems, the first LMB is our RMO region. */
 	if (base == 0)
 		lmb.rmo_size = size;
 
-	return lmb_add_region(_rgn, base, size);
+	ret = lmb_add_region(_rgn, base, size);
+
+	spin_unlock(&lmb.lock);
+	return ret;
 
 }
 
 long lmb_remove(u64 base, u64 size)
 {
+	long ret;
 	struct lmb_region *rgn = &(lmb.memory);
 	u64 rgnbegin, rgnend;
 	u64 end = base + size;
@@ -213,6 +231,8 @@ long lmb_remove(u64 base, u64 size)
 
 	rgnbegin = rgnend = 0; /* supress gcc warnings */
 
+	spin_lock(&lmb.lock);
+
 	/* Find the region where (base, size) belongs to */
 	for (i=0; i < rgn->cnt; i++) {
 		rgnbegin = rgn->region[i].base;
@@ -223,12 +243,15 @@ long lmb_remove(u64 base, u64 size)
 	}
 
 	/* Didn't find the region */
-	if (i == rgn->cnt)
+	if (i == rgn->cnt) {
+		spin_unlock(&lmb.lock);
 		return -1;
+	}
 
 	/* Check to see if we are removing entire region */
 	if ((rgnbegin == base) && (rgnend == end)) {
 		lmb_remove_region(rgn, i);
+		spin_unlock(&lmb.lock);
 		return 0;
 	}
 
@@ -236,12 +259,14 @@ long lmb_remove(u64 base, u64 size)
 	if (rgnbegin == base) {
 		rgn->region[i].base = end;
 		rgn->region[i].size -= size;
+		spin_unlock(&lmb.lock);
 		return 0;
 	}
 
 	/* Check to see if the region is matching at the end */
 	if (rgnend == end) {
 		rgn->region[i].size -= size;
+		spin_unlock(&lmb.lock);
 		return 0;
 	}
 
@@ -250,7 +275,10 @@ long lmb_remove(u64 base, u64 size)
 	 * beginging of the hole and add the region after hole.
 	 */
 	rgn->region[i].size = base - rgn->region[i].base;
-	return lmb_add_region(rgn, end, rgnend - end);
+	ret = lmb_add_region(rgn, end, rgnend - end);
+
+	spin_unlock(&lmb.lock);
+	return ret;
 }
 
 long __init lmb_reserve(u64 base, u64 size)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [patch v2] LMB: Add basic spin locking to lmb
  2008-05-20  0:40       ` [rfc] [patch] LMB: Add basic spin locking to lmb Geoff Levand
@ 2008-05-20  0:55         ` Geoff Levand
  2008-05-20  2:22           ` David Miller
  0 siblings, 1 reply; 13+ messages in thread
From: Geoff Levand @ 2008-05-20  0:55 UTC (permalink / raw)
  To: David Miller, linux-kernel; +Cc: paulus, linuxppc-dev

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.

Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---

v2: o Add locking to lmb_find().

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

--- a/include/linux/lmb.h
+++ b/include/linux/lmb.h
@@ -30,6 +30,7 @@ struct lmb_region {
 };
 
 struct lmb {
+	spinlock_t lock;
 	unsigned long debug;
 	u64 rmo_size;
 	struct lmb_region memory;
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -32,28 +32,33 @@ early_param("lmb", early_lmb);
 void lmb_dump_all(void)
 {
 	unsigned long i;
+	struct lmb tmp;
 
 	if (!lmb_debug)
 		return;
 
+	spin_lock(&lmb.lock);
+	tmp = lmb;
+	spin_unlock(&lmb.lock);
+
 	pr_info("lmb_dump_all:\n");
-	pr_info("    memory.cnt		  = 0x%lx\n", lmb.memory.cnt);
+	pr_info("    memory.cnt		  = 0x%lx\n", tmp.memory.cnt);
 	pr_info("    memory.size		  = 0x%llx\n",
-	    (unsigned long long)lmb.memory.size);
-	for (i=0; i < lmb.memory.cnt ;i++) {
+	    (unsigned long long)tmp.memory.size);
+	for (i=0; i < tmp.memory.cnt ;i++) {
 		pr_info("    memory.region[0x%lx].base       = 0x%llx\n",
-		    i, (unsigned long long)lmb.memory.region[i].base);
+		    i, (unsigned long long)tmp.memory.region[i].base);
 		pr_info("		      .size     = 0x%llx\n",
-		    (unsigned long long)lmb.memory.region[i].size);
+		    (unsigned long long)tmp.memory.region[i].size);
 	}
 
-	pr_info("    reserved.cnt	  = 0x%lx\n", lmb.reserved.cnt);
-	pr_info("    reserved.size	  = 0x%lx\n", lmb.reserved.size);
-	for (i=0; i < lmb.reserved.cnt ;i++) {
+	pr_info("    reserved.cnt	  = 0x%lx\n", tmp.reserved.cnt);
+	pr_info("    reserved.size	  = 0x%lx\n", tmp.reserved.size);
+	for (i=0; i < tmp.reserved.cnt ;i++) {
 		pr_info("    reserved.region[0x%lx].base       = 0x%llx\n",
-		    i, (unsigned long long)lmb.reserved.region[i].base);
+		    i, (unsigned long long)tmp.reserved.region[i].base);
 		pr_info("		      .size     = 0x%llx\n",
-		    (unsigned long long)lmb.reserved.region[i].size);
+		    (unsigned long long)tmp.reserved.region[i].size);
 	}
 }
 
@@ -105,6 +110,8 @@ static void lmb_coalesce_regions(struct 
 
 void __init lmb_init(void)
 {
+	spin_lock_init(&lmb.lock);
+
 	/* Create a dummy zero size LMB which will get coalesced away later.
 	 * This simplifies the lmb_add() code below...
 	 */
@@ -122,10 +129,14 @@ void __init lmb_analyze(void)
 {
 	int i;
 
+	spin_lock(&lmb.lock);
+
 	lmb.memory.size = 0;
 
 	for (i = 0; i < lmb.memory.cnt; i++)
 		lmb.memory.size += lmb.memory.region[i].size;
+
+	spin_unlock(&lmb.lock);
 }
 
 static long lmb_add_region(struct lmb_region *rgn, u64 base, u64 size)
@@ -194,18 +205,25 @@ static long lmb_add_region(struct lmb_re
 
 long lmb_add(u64 base, u64 size)
 {
+	long ret;
 	struct lmb_region *_rgn = &lmb.memory;
 
+	spin_lock(&lmb.lock);
+
 	/* On pSeries LPAR systems, the first LMB is our RMO region. */
 	if (base == 0)
 		lmb.rmo_size = size;
 
-	return lmb_add_region(_rgn, base, size);
+	ret = lmb_add_region(_rgn, base, size);
+
+	spin_unlock(&lmb.lock);
+	return ret;
 
 }
 
 long lmb_remove(u64 base, u64 size)
 {
+	long ret;
 	struct lmb_region *rgn = &(lmb.memory);
 	u64 rgnbegin, rgnend;
 	u64 end = base + size;
@@ -213,6 +231,8 @@ long lmb_remove(u64 base, u64 size)
 
 	rgnbegin = rgnend = 0; /* supress gcc warnings */
 
+	spin_lock(&lmb.lock);
+
 	/* Find the region where (base, size) belongs to */
 	for (i=0; i < rgn->cnt; i++) {
 		rgnbegin = rgn->region[i].base;
@@ -223,12 +243,15 @@ long lmb_remove(u64 base, u64 size)
 	}
 
 	/* Didn't find the region */
-	if (i == rgn->cnt)
+	if (i == rgn->cnt) {
+		spin_unlock(&lmb.lock);
 		return -1;
+	}
 
 	/* Check to see if we are removing entire region */
 	if ((rgnbegin == base) && (rgnend == end)) {
 		lmb_remove_region(rgn, i);
+		spin_unlock(&lmb.lock);
 		return 0;
 	}
 
@@ -236,12 +259,14 @@ long lmb_remove(u64 base, u64 size)
 	if (rgnbegin == base) {
 		rgn->region[i].base = end;
 		rgn->region[i].size -= size;
+		spin_unlock(&lmb.lock);
 		return 0;
 	}
 
 	/* Check to see if the region is matching at the end */
 	if (rgnend == end) {
 		rgn->region[i].size -= size;
+		spin_unlock(&lmb.lock);
 		return 0;
 	}
 
@@ -250,7 +275,10 @@ long lmb_remove(u64 base, u64 size)
 	 * beginging of the hole and add the region after hole.
 	 */
 	rgn->region[i].size = base - rgn->region[i].base;
-	return lmb_add_region(rgn, end, rgnend - end);
+	ret = lmb_add_region(rgn, end, rgnend - end);
+
+	spin_unlock(&lmb.lock);
+	return ret;
 }
 
 long __init lmb_reserve(u64 base, u64 size)
@@ -502,12 +530,16 @@ int lmb_find(struct lmb_property *res)
 	rstart = res->base;
 	rend = rstart + res->size - 1;
 
+	spin_lock(&lmb.lock);
+
 	for (i = 0; i < lmb.memory.cnt; i++) {
 		u64 start = lmb.memory.region[i].base;
 		u64 end = start + lmb.memory.region[i].size - 1;
 
-		if (start > rend)
+		if (start > rend) {
+			spin_unlock(&lmb.lock);
 			return -1;
+		}
 
 		if ((end >= rstart) && (start < rend)) {
 			/* adjust the request */
@@ -517,8 +549,10 @@ int lmb_find(struct lmb_property *res)
 				rend = end;
 			res->base = rstart;
 			res->size = rend - rstart + 1;
+			spin_unlock(&lmb.lock);
 			return 0;
 		}
 	}
+	spin_unlock(&lmb.lock);
 	return -1;
 }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch v2] LMB: Add basic spin locking to lmb
  2008-05-20  0:55         ` [patch v2] " Geoff Levand
@ 2008-05-20  2:22           ` David Miller
  2008-05-20  2:32             ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 13+ messages in thread
From: David Miller @ 2008-05-20  2:22 UTC (permalink / raw)
  To: geoffrey.levand; +Cc: paulus, linux-kernel, linuxppc-dev

From: Geoff Levand <geoffrey.levand@am.sony.com>
Date: Mon, 19 May 2008 17:55:45 -0700

> 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.
> 
> Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
> ---
> 
> v2: o Add locking to lmb_find().

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch v2] LMB: Add basic spin locking to lmb
  2008-05-20  2:22           ` David Miller
@ 2008-05-20  2:32             ` Benjamin Herrenschmidt
  2008-05-20  2:34               ` David Miller
  0 siblings, 1 reply; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2008-05-20  2:32 UTC (permalink / raw)
  To: David Miller; +Cc: linuxppc-dev, paulus, linux-kernel


On Mon, 2008-05-19 at 19:22 -0700, David Miller wrote:
> From: Geoff Levand <geoffrey.levand@am.sony.com>
> Date: Mon, 19 May 2008 17:55:45 -0700
> 
> > 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.
> > 
> > Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
> > ---
> > 
> > v2: o Add locking to lmb_find().
> 
> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch v2] LMB: Add basic spin locking to lmb
  2008-05-20  2:32             ` Benjamin Herrenschmidt
@ 2008-05-20  2:34               ` David Miller
  0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2008-05-20  2:34 UTC (permalink / raw)
  To: benh; +Cc: linuxppc-dev, paulus, linux-kernel

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Mon, 19 May 2008 22:32:58 -0400

> 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.

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch v2] PS3: Fix memory hotplug
  2008-05-15 20:09 ` [patch v2] " Geoff Levand
@ 2008-05-21 16:41   ` Geoff Levand
  0 siblings, 0 replies; 13+ messages in thread
From: Geoff Levand @ 2008-05-21 16:41 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev@ozlabs.org

Geoff Levand wrote:
> 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.
> 
> Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
> ---
> 
> v2: Add call to lmb_analyze().
> 
>  arch/powerpc/platforms/ps3/mm.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> --- a/arch/powerpc/platforms/ps3/mm.c
> +++ b/arch/powerpc/platforms/ps3/mm.c
> @@ -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)
> 

Hi Paul,

Could you please merge this one in for 2.6.26.  Without it the
system boots with just 128 of the total 256 MiB of memory.

There is the concurrency problem as Ben commented on, but I
think not having the 128 MiB of memory is worse than having the
potential race, which we can work on as a separate fix.

-Geoff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: PS3: Fix memory hotplug
  2008-05-15  9:38   ` PS3: Fix memory hotplug Paul Mackerras
@ 2008-05-21 18:31     ` Geoff Levand
  0 siblings, 0 replies; 13+ messages in thread
From: Geoff Levand @ 2008-05-21 18:31 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: davem, linuxppc-dev@ozlabs.org

Paul Mackerras wrote:
> Benjamin Herrenschmidt writes:
> 
>> 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.. 
> 
> 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...

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2008-05-21 18:31 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-13 16:23 PS3: Fix memory hotplug Geoff Levand
2008-05-15  6:48 ` Benjamin Herrenschmidt
2008-05-15  6:51   ` Benjamin Herrenschmidt
2008-05-15  7:02     ` David Miller
2008-05-20  0:40       ` [rfc] [patch] LMB: Add basic spin locking to lmb Geoff Levand
2008-05-20  0:55         ` [patch v2] " Geoff Levand
2008-05-20  2:22           ` David Miller
2008-05-20  2:32             ` Benjamin Herrenschmidt
2008-05-20  2:34               ` David Miller
2008-05-15  9:38   ` PS3: Fix memory hotplug Paul Mackerras
2008-05-21 18:31     ` Geoff Levand
2008-05-15 20:09 ` [patch v2] " Geoff Levand
2008-05-21 16:41   ` Geoff Levand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).