mm: Fix memory hotplug + sparsemem build.

PATCH In review Paul Mundt 5 participants 11 messages Sep 11, 2007 – Sep 14, 2007

Subsystems (2)

Memory management Memory hot(un)plug

lore

Share & tools

lore

Paul Mundt

author

2007-09-11 07:25 UTC #0 lore raw

PATCH 1/1 mm: Fix memory hotplug + sparsemem build.

Building with CONFIG_MEMORY_HOTPLUG_SPARSE enabled results in:

CC mm/memory_hotplug.o
mm/memory_hotplug.c: In function 'online_pages':
mm/memory_hotplug.c:215: error: 'struct zone' has no member named 'node' make[1]: *** [mm/memory_hotplug.o] Error 1
make: *** [mm] Error 2

Fix it up.

Signed-off-by Paul Mundt <[email protected]>

 mm/memory_hotplug.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory_hotplug.c +1 -1

@@ -212,7 +212,7 @@
 	zone->present_pages += onlined_pages;
 	zone->zone_pgdat->node_present_pages += onlined_pages;
 	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yasunori Goto

2007-09-11 08:18 UTC #1 lore raw

Review

Quoted 5 lines

(quoted diff) +1 -1

 	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();

Thanks Paul-san.

I also have another issue around here.
(Kswapd doesn't run on memory less node now. It should run when
the node has memory.)

I would like to merge them like following if you don't mind.

Bye.

---

Fix kswapd doesn't run when memory is added on memory-less-node. Fix compile error of zone->node when CONFIG_NUMA is off.

Signed-off-by Yasunori Goto <[email protected]>

Signed-off-by Paul Mundt <[email protected]>

---
 mm/memory_hotplug.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

mm/memory_hotplug.c +4 -5

Index: current/mm/memory_hotplug.c
===================================================================
@@ -211,10 +211,12 @@ int online_pages(unsigned long pfn, unsi
 		online_pages_range);
 	zone->present_pages += onlined_pages;
 	zone->zone_pgdat->node_present_pages += onlined_pages;
-	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();
+	if (onlined_pages){
+		kswapd_run(zone_to_nid(zone));
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
+	}
 
 	if (need_zonelists_rebuild)
 		build_all_zonelists();

@@ -269,9 +271,6 @@ int add_memory(int nid, u64 start, u64 s
 		if (!pgdat)
 			return -ENOMEM;
 		new_pgdat = 1;
-		ret = kswapd_run(nid);
-		if (ret)
-			goto error;
 	}
 
 	/* call arch's memory hotadd */

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Paul Mundt

author

2007-09-11 08:25 UTC #2 lore raw

Hi Goto-san,

Quoted 8 lines · Yasunori Goto

mm/memory_hotplug.c int online_pages(unsigned long pfn, unsi
Quoted 5 lines
mm/memory_hotplug.c +1 -1
 	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();
Thanks Paul-san.

Quoted 6 lines

I also have another issue around here.
(Kswapd doesn't run on memory less node now. It should run when
the node has memory.)
I would like to merge them like following if you don't mind.

Looks fine to me!

Andy Whitcroft

Acked-by

2007-09-11 09:15 UTC #3 lore raw

In reply to Yasunori Goto

Quoted 35 lines · Yasunori Goto

mm/memory_hotplug.c int online_pages(unsigned long pfn, unsi
Quoted 5 lines
mm/memory_hotplug.c +1 -1
 	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();
Thanks Paul-san.
I also have another issue around here.
(Kswapd doesn't run on memory less node now. It should run when
the node has memory.)
I would like to merge them like following if you don't mind.
Bye.
---
Fix kswapd doesn't run when memory is added on memory-less-node. Fix compile error of zone->node when CONFIG_NUMA is off.
Signed-off-by Yasunori Goto <[email protected]>
Signed-off-by Paul Mundt <[email protected]>
---
 mm/memory_hotplug.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
Index: current/mm/memory_hotplug.c
===================================================================

mm/memory_hotplug.c int online_pages(unsigned long pfn, unsi · line 216

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -211,10 +211,12 @@ int online_pages(unsigned long pfn, unsi
 		online_pages_range);
 	zone->present_pages += onlined_pages;
 	zone->zone_pgdat->node_present_pages += onlined_pages;
-	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();
+	if (onlined_pages){

Nit, needs a space there before the '{'.

Quoted 7 lines

mm/memory_hotplug.c +3 -0

+		kswapd_run(zone_to_nid(zone));
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
+	}
 
 	if (need_zonelists_rebuild)
 		build_all_zonelists();

mm/memory_hotplug.c int add_memory(int nid, u64 start, u64 s · line 276

Quoted 9 lines

mm/memory_hotplug.c +0 -3

@@ -269,9 +271,6 @@ int add_memory(int nid, u64 start, u64 s
 		if (!pgdat)
 			return -ENOMEM;
 		new_pgdat = 1;
-		ret = kswapd_run(nid);
-		if (ret)
-			goto error;
 	}
 
 	/* call arch's memory hotadd */

The problem as I see it is that when we boot the system we start a kswapd on all nodes with memory. If the hot-add adds memory to a pre-existing node with no memory we will not start one and we end up with a node with memory and no kswapd. Bad.

As kswapd_run is a no-op when a kswapd already exists this seems a safe way to fix that. Paul's ->zone conversion is obviously correct also.

Acked-by Andy Whitcroft <[email protected]>

-apw

Yasunori Goto

Acked-by Andy Whitcroft

2007-09-11 09:37 UTC #4 lore raw

Quoted 3 lines

Review
Quoted 1 line
(quoted diff) +1 -0
+	if (onlined_pages){
Nit, needs a space there before the '{'.

Ah, Ok. I attached fixed patch in this mail.

Quoted 9 lines

The problem as I see it is that when we boot the system we start a kswapd on all nodes with memory. If the hot-add adds memory to a pre-existing node with no memory we will not start one and we end up with a node with memory and no kswapd. Bad.
As kswapd_run is a no-op when a kswapd already exists this seems a safe way to fix that. Paul's ->zone conversion is obviously correct also.
Acked-by Andy Whitcroft <[email protected]>

Thanks for your explanation.
You mentioned all of my intention correctly. :-)

----

Fix kswapd doesn't run when memory is added on memory-less-node. Fix compile error of zone->node when CONFIG_NUMA is off.

Signed-off-by Yasunori Goto <[email protected]>

Signed-off-by Paul Mundt <[email protected]>

Acked-by Andy Whitcroft <[email protected]>

---
 mm/memory_hotplug.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

mm/memory_hotplug.c +4 -5

Index: current/mm/memory_hotplug.c
===================================================================
@@ -211,10 +211,12 @@ int online_pages(unsigned long pfn, unsi
 		online_pages_range);
 	zone->present_pages += onlined_pages;
 	zone->zone_pgdat->node_present_pages += onlined_pages;
-	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();
+	if (onlined_pages) {
+		kswapd_run(zone_to_nid(zone));
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
+	}
 
 	if (need_zonelists_rebuild)
 		build_all_zonelists();

@@ -269,9 +271,6 @@ int add_memory(int nid, u64 start, u64 s
 		if (!pgdat)
 			return -ENOMEM;
 		new_pgdat = 1;
-		ret = kswapd_run(nid);
-		if (ret)
-			goto error;
 	}
 
 	/* call arch's memory hotadd */

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2007-09-14 01:44 UTC #5 lore raw

Quoted 56 lines · On Tue, 11 Sep 2007 18:37:12 +0900 Yasunori Goto <[email protected]> wrote:

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1 -0
+	if (onlined_pages){
Nit, needs a space there before the '{'.
Ah, Ok. I attached fixed patch in this mail.
Quoted 9 lines
The problem as I see it is that when we boot the system we start a kswapd on all nodes with memory. If the hot-add adds memory to a pre-existing node with no memory we will not start one and we end up with a node with memory and no kswapd. Bad.
As kswapd_run is a no-op when a kswapd already exists this seems a safe way to fix that. Paul's ->zone conversion is obviously correct also.
Acked-by Andy Whitcroft <[email protected]>
Thanks for your explanation.
You mentioned all of my intention correctly. :-)
----
Fix kswapd doesn't run when memory is added on memory-less-node. Fix compile error of zone->node when CONFIG_NUMA is off.
Signed-off-by Yasunori Goto <[email protected]>
Signed-off-by Paul Mundt <[email protected]>
Acked-by Andy Whitcroft <[email protected]>
---
 mm/memory_hotplug.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
Index: current/mm/memory_hotplug.c
===================================================================
mm/memory_hotplug.c +4 -2
@@ -211,10 +211,12 @@ int online_pages(unsigned long pfn, unsi
 		online_pages_range);
 	zone->present_pages += onlined_pages;
 	zone->zone_pgdat->node_present_pages += onlined_pages;
-	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();
+	if (onlined_pages) {
+		kswapd_run(zone_to_nid(zone));
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
+	}
 
 	if (need_zonelists_rebuild)
 		build_all_zonelists();

mm/memory_hotplug.c int add_memory(int nid, u64 start, u64 s · line 276

Quoted 9 lines

mm/memory_hotplug.c +0 -3

@@ -269,9 +271,6 @@ int add_memory(int nid, u64 start, u64 s
 		if (!pgdat)
 			return -ENOMEM;
 		new_pgdat = 1;
-		ret = kswapd_run(nid);
-		if (ret)
-			goto error;
 	}
 
 	/* call arch's memory hotadd */

OK, we're getting into a mess here. This patch fixes update-n_high_memory-node-state-for-memory-hotadd.patch, but which patch does update-n_high_memory-node-state-for-memory-hotadd.patch fix?

At present I just whacked update-n_high_memory-node-state-for-memory-hotadd.patch at the end of everything, but that was lazy of me and it ends up making a mess.

Yasunori Goto

2007-09-14 02:02 UTC #6 lore raw

Quoted 72 lines · On Tue, 11 Sep 2007 18:37:12 +0900 Yasunori Goto <[email protected]> wrote:

Quoted 57 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +1 -0
+	if (onlined_pages){
Nit, needs a space there before the '{'.
Ah, Ok. I attached fixed patch in this mail.
Quoted 9 lines
The problem as I see it is that when we boot the system we start a kswapd on all nodes with memory. If the hot-add adds memory to a pre-existing node with no memory we will not start one and we end up with a node with memory and no kswapd. Bad.
As kswapd_run is a no-op when a kswapd already exists this seems a safe way to fix that. Paul's ->zone conversion is obviously correct also.
Acked-by Andy Whitcroft <[email protected]>
Thanks for your explanation.
You mentioned all of my intention correctly. :-)
----
Fix kswapd doesn't run when memory is added on memory-less-node. Fix compile error of zone->node when CONFIG_NUMA is off.
Signed-off-by Yasunori Goto <[email protected]>
Signed-off-by Paul Mundt <[email protected]>
Acked-by Andy Whitcroft <[email protected]>
---
 mm/memory_hotplug.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
Index: current/mm/memory_hotplug.c
===================================================================
mm/memory_hotplug.c +4 -2
@@ -211,10 +211,12 @@ int online_pages(unsigned long pfn, unsi
 		online_pages_range);
 	zone->present_pages += onlined_pages;
 	zone->zone_pgdat->node_present_pages += onlined_pages;
-	if (onlined_pages)
-		node_set_state(zone->node, N_HIGH_MEMORY);
 
 	setup_per_zone_pages_min();
+	if (onlined_pages) {
+		kswapd_run(zone_to_nid(zone));
+		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
+	}
 
 	if (need_zonelists_rebuild)
 		build_all_zonelists();
mm/memory_hotplug.c int add_memory(int nid, u64 start, u64 s · line 276
Quoted 9 lines
mm/memory_hotplug.c +0 -3
@@ -269,9 +271,6 @@ int add_memory(int nid, u64 start, u64 s
 		if (!pgdat)
 			return -ENOMEM;
 		new_pgdat = 1;
-		ret = kswapd_run(nid);
-		if (ret)
-			goto error;
 	}
 
 	/* call arch's memory hotadd */
OK, we're getting into a mess here. This patch fixes update-n_high_memory-node-state-for-memory-hotadd.patch, but which patch does update-n_high_memory-node-state-for-memory-hotadd.patch fix?

Quoted 3 lines

At present I just whacked update-n_high_memory-node-state-for-memory-hotadd.patch at the end of everything, but that was lazy of me and it ends up making a mess.

It is enough. No more patch is necessary for these issues. I already fixed about Andy-san's comment. :-)

Thanks.

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2007-09-14 02:41 UTC #7 lore raw

Quoted 7 lines · On Fri, 14 Sep 2007 11:02:43 +0900 Yasunori Goto <[email protected]> wrote:

Quoted 7 lines
Quoted 2 lines
/* call arch's memory hotadd */
OK, we're getting into a mess here. This patch fixes update-n_high_memory-node-state-for-memory-hotadd.patch, but which patch does update-n_high_memory-node-state-for-memory-hotadd.patch fix?

Quoted 6 lines

Quoted 3 lines
At present I just whacked update-n_high_memory-node-state-for-memory-hotadd.patch at the end of everything, but that was lazy of me and it ends up making a mess.
It is enough. No more patch is necessary for these issues. I already fixed about Andy-san's comment. :-)

Now I'm more confused. I have two separeate questions:

a) Is the justr-added update-n_high_memory-node-state-for-memory-hotadd-fix.patch still needed?

b) Which patch in 2.6.22-rc4-mm1 does update-n_high_memory-node-state-for-memory-hotadd.patch fix? In other words, into which patch should I fold update-n_high_memory-node-state-for-memory-hotadd.patch prior to sending to Linus?

(I (usually) get to work this out for myself. Sometimes it is painful).

Generally, if people tell me which patch-in-mm their patch is fixing, it really helps. Adrian does this all the time.

Thanks.

Yasunori Goto

2007-09-14 05:14 UTC #8 lore raw

Quoted 13 lines · On Fri, 14 Sep 2007 11:02:43 +0900 Yasunori Goto <[email protected]> wrote:

Quoted 12 lines
Quoted 11 lines
Quoted 3 lines
/* call arch's memory hotadd */
OK, we're getting into a mess here. This patch fixes update-n_high_memory-node-state-for-memory-hotadd.patch, but which patch does update-n_high_memory-node-state-for-memory-hotadd.patch fix?
At present I just whacked update-n_high_memory-node-state-for-memory-hotadd.patch at the end of everything, but that was lazy of me and it ends up making a mess.

Quoted 7 lines

Quoted 2 lines
It is enough. No more patch is necessary for these issues. I already fixed about Andy-san's comment. :-)
Now I'm more confused. I have two separeate questions:
a) Is the justr-added update-n_high_memory-node-state-for-memory-hotadd-fix.patch still needed?

I'm not sure exact meaning of "just-added".
But, update-n_high_memory-node-state-for-memory-hotadd-fix.patch is necessary for 2.6.23-rc4-mm1.

Quoted 1 line

b) Which patch in 2.6.22-rc4-mm1 does

                    2.6.23-rc4-mm1?

Quoted 4 lines

update-n_high_memory-node-state-for-memory-hotadd.patch fix? In other words, into which patch should I fold update-n_high_memory-node-state-for-memory-hotadd.patch prior to sending to Linus?

In my understanding, update-n_high_memory-node-state-for-memory-hotadd.patch should be folded with all of memoryless-nodes-xxxxxxxxxxxx.patch.
It sets N_HIGH_MEMORY for a new node-with-memory.

But if you need specifing of more detail patch, becase N_HIGH_MEMORY is set in memoryless-nodes-introduce-ask-of-nodes-with-memory.patch, I suppose update-n_high_memory-node-state-for-memory-hotadd.patch should be fold with it.

update-n_high_memory-node-state-for-memory-hotadd-fix.patch

^^^

is fixes of update-n_high_memory-node-state-for-memory-hotadd.patch and memoryless-nodes-no-need-for-kswapd.patch

Is it enough for your question? Or more confuse?

Quoted 4 lines

(I (usually) get to work this out for myself. Sometimes it is painful).
Generally, if people tell me which patch-in-mm their patch is fixing, it really helps. Adrian does this all the time.

Sorry for your confusing...

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Lee Schermerhorn

2007-09-14 14:09 UTC #9 lore raw

In reply to Andrew Morton

Quoted 18 lines · Andrew Morton

Quoted 7 lines · On Fri, 14 Sep 2007 11:02:43 +0900 Yasunori Goto <[email protected]> wrote:
Quoted 7 lines
Quoted 2 lines
/* call arch's memory hotadd */
OK, we're getting into a mess here. This patch fixes update-n_high_memory-node-state-for-memory-hotadd.patch, but which patch does update-n_high_memory-node-state-for-memory-hotadd.patch fix?
Quoted 6 lines
Quoted 3 lines
At present I just whacked update-n_high_memory-node-state-for-memory-hotadd.patch at the end of everything, but that was lazy of me and it ends up making a mess.
It is enough. No more patch is necessary for these issues. I already fixed about Andy-san's comment. :-)
Now I'm more confused. I have two separeate questions:

Quoted 8 lines

a) Is the justr-added update-n_high_memory-node-state-for-memory-hotadd-fix.patch still needed?
b) Which patch in 2.6.22-rc4-mm1 does update-n_high_memory-node-state-for-memory-hotadd.patch fix? In other words, into which patch should I fold update-n_high_memory-node-state-for-memory-hotadd.patch prior to sending to Linus?

Andrew:

I originally sent in the "update-n_high_memory..." patch against 23-rc3-mm1 on 27aug to fix a problem that I introduced when I moved the populating of N_HIGH_MEMORY state to free_area_init_nodes(). This would miss setting the "has memory" node state for hot added memory. I never saw any response, but then it ended up in 23-rc4-mm1.

This Tuesday, Paul Mundt sent in a patch to fix a build problem with MEMORY_HOTPLUG_SPARSE introduced by my patch. He replaced zone->node with zone_to_nid(zone) in the node_set_state() arguments.

The latest patch, from Yasunori-san, I believe, starts kswapd for nodes to which memory has been hot-added. As I understand it, his is needed because the memoryless nodes patch results in no kswapd for memoryless nodes.

Does that help?

Lee

Quoted 6 lines

(I (usually) get to work this out for myself. Sometimes it is painful).
Generally, if people tell me which patch-in-mm their patch is fixing, it really helps. Adrian does this all the time.

Quoted 7 lines

Thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2007-09-14 20:51 UTC #10 lore raw

Quoted 10 lines · On Fri, 14 Sep 2007 10:09:27 -0400 Lee Schermerhorn <[email protected]>…

I originally sent in the "update-n_high_memory..." patch against 23-rc3-mm1 on 27aug to fix a problem that I introduced when I moved the populating of N_HIGH_MEMORY state to free_area_init_nodes(). This would miss setting the "has memory" node state for hot added memory. I never saw any response, but then it ended up in 23-rc4-mm1.
This Tuesday, Paul Mundt sent in a patch to fix a build problem with MEMORY_HOTPLUG_SPARSE introduced by my patch. He replaced zone->node with zone_to_nid(zone) in the node_set_state() arguments.

Quoted 6 lines

The latest patch, from Yasunori-san, I believe, starts kswapd for nodes to which memory has been hot-added. As I understand it, his is needed because the memoryless nodes patch results in no kswapd for memoryless nodes.
Does that help?

not really ;)

See, when I get some rinky-dink little fix for a patch in -mm I will position that patch immediately after the patch which it is fixing, with a filename which is derived from the fixed patch's name. So when send-to-Linus time comes, I can fold the fixes into the base patch. This practice also keeps the patches in a sensible presentation order, with minimum interdependencies and good git-bisect friendliness.

However it sometimes (rarely) takes considerable effort to work out which patch in -mm a particular fix is fixing. That was the case with update-n_high_memory-node-state-for-memory-hotadd.patch.

It helps me quite a bit if people tell me which patch they're fixing. Usually they don't and I get to work it out. Usually it's fairly obvious.