mm: get_nid_for_pfn() returns int

PATCH In review Roel Kluin 4 participants 13 messages Jan 18, 2009 – Mar 1, 2009

Subsystems (2)

Memory management Driver core

lore

Share & tools

lore

Roel Kluin

author

2009-01-18 22:36 UTC #0 lore raw

PATCH 1/1 mm: get_nid_for_pfn() returns int

get_nid_for_pfn() returns int

Signed-off-by Roel Kluin <[email protected]>

---
vi drivers/base/node.c +256
static int get_nid_for_pfn(unsigned long pfn)

drivers/base/node.c +1 -1

index 43fa90b..f8f578a 100644
@@ -303,7 +303,7 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
-		unsigned int nid;
+		int nid;
 
 		nid = get_nid_for_pfn(pfn);
 		if (nid < 0)

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

Gary Hade

2009-01-19 17:59 UTC #1 lore raw

Quoted 12 lines · Roel Kluin

get_nid_for_pfn() returns int
Signed-off-by Roel Kluin <[email protected]>
---
vi drivers/base/node.c +256
static int get_nid_for_pfn(unsigned long pfn)
drivers/base/node.c +0 -0
index 43fa90b..f8f578a 100644

drivers/base/node.c int unregister_mem_sect_under_nodes(struct memory_block *mem_blk) · line 309

Quoted 8 lines

drivers/base/node.c +1 -1

@@ -303,7 +303,7 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
-		unsigned int nid;
+		int nid;
 
 		nid = get_nid_for_pfn(pfn);
 		if (nid < 0)

My mistake. Good catch.

Thanks,
Gary

Signature · 12 lines

Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc

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

2009-01-27 06:35 UTC #2 lore raw

Quoted 19 lines · On Mon, 19 Jan 2009 09:59:19 -0800 Gary Hade <[email protected]> wrote:

Quoted 18 lines · Roel Kluin
get_nid_for_pfn() returns int
Signed-off-by Roel Kluin <[email protected]>
---
vi drivers/base/node.c +256
static int get_nid_for_pfn(unsigned long pfn)
drivers/base/node.c +1 -1
index 43fa90b..f8f578a 100644
@@ -303,7 +303,7 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
-		unsigned int nid;
+		int nid;
 

Quoted 5 lines

drivers/base/node.c int unregister_mem_sect_under_nodes(struct memory_block *mem_blk) · line 309
Quoted 2 lines
drivers/base/node.c +0 -0
 		nid = get_nid_for_pfn(pfn);
 		if (nid < 0)
My mistake. Good catch.

Presumably the (nid < 0) case has never happened.

Should we retain the test?

Is silently skipping the node in that case desirable behaviour?

Gary Hade

2009-01-27 21:07 UTC #3 lore raw

Quoted 21 lines · Andrew Morton

Quoted 19 lines · On Mon, 19 Jan 2009 09:59:19 -0800 Gary Hade <[email protected]> wrote:
Quoted 18 lines · Roel Kluin
get_nid_for_pfn() returns int
Signed-off-by Roel Kluin <[email protected]>
---
vi drivers/base/node.c +256
static int get_nid_for_pfn(unsigned long pfn)
drivers/base/node.c +1 -1
index 43fa90b..f8f578a 100644
@@ -303,7 +303,7 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
-		unsigned int nid;
+		int nid;
 

Quoted 7 lines

Quoted 5 lines
drivers/base/node.c int unregister_mem_sect_under_nodes(struct memory_block *mem_blk) · line 309
Quoted 2 lines
drivers/base/node.c +0 -0
 		nid = get_nid_for_pfn(pfn);
 		if (nid < 0)
My mistake. Good catch.
Presumably the (nid < 0) case has never happened.

We do know that it is happening on one system while creating a symlink for a memory section so it should also happen on the same system if unregister_mem_sect_under_nodes() were called to remove the same symlink.

The test was actually added in response to a problem with an earlier version reported by Yasunori Goto where one or more of the leading pages of a memory section on the 2nd node of one of his systems was uninitialized because I believe they coincided with a memory hole. The earlier version did not ignore uninitialized pages and determined the nid by considering only the 1st page of each memory section. This caused the symlink to the 1st memory section on the 2nd node to be incorrectly created in /sys/devices/system/node/node0 instead of /sys/devices/system/node/node1. The problem was fixed by adding the test to skip over uninitialized pages.

I suspect we have not seen any reports of the non-removal of a symlink due to the incorrect declaration of the nid variable in unregister_mem_sect_under_nodes() because
- systems where a memory section could have an uninitialized range of leading pages are probably rare.
- memory remove is probably not done very frequently on the systems that are capable of demonstrating the problem.
- lingering symlink(s) that should have been removed may have simply gone unnoticed.

Quoted 2 lines

Should we retain the test?

Yes.

Quoted 2 lines

Is silently skipping the node in that case desirable behaviour?

It actually silently skips pages (not nodes) in it's quest for valid nids for all the nodes that the memory section scans. This is definitely desirable.

I hope this answers your questions.

Thanks,
Gary

Signature · 12 lines

Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc

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

2009-01-28 05:04 UTC #4 lore raw

Quoted 35 lines · Andrew Morton

Quoted 21 lines · On Mon, 19 Jan 2009 09:59:19 -0800 Gary Hade <[email protected]> wrote:
Quoted 20 lines · Roel Kluin
Quoted 18 lines
get_nid_for_pfn() returns int
Signed-off-by Roel Kluin <[email protected]>
---
vi drivers/base/node.c +256
static int get_nid_for_pfn(unsigned long pfn)
drivers/base/node.c +1 -1
index 43fa90b..f8f578a 100644
@@ -303,7 +303,7 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk)
 	sect_start_pfn = section_nr_to_pfn(mem_blk->phys_index);
 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
-		unsigned int nid;
+		int nid;
 
Quoted 7 lines
Quoted 5 lines
drivers/base/node.c int unregister_mem_sect_under_nodes(struct memory_block *mem_blk) · line 309
Quoted 2 lines
drivers/base/node.c +0 -0
 		nid = get_nid_for_pfn(pfn);
 		if (nid < 0)
My mistake. Good catch.
Presumably the (nid < 0) case has never happened.
We do know that it is happening on one system while creating a symlink for a memory section so it should also happen on the same system if unregister_mem_sect_under_nodes() were called to remove the same symlink.

Quoted 5 lines

The test was actually added in response to a problem with an earlier version reported by Yasunori Goto where one or more of the leading pages of a memory section on the 2nd node of one of his systems was uninitialized because I believe they coincided with a memory hole.

Yes. There are some memory hole pages which are occupied by firmware in our box.

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>

roel kluin

author

2009-02-27 14:56 UTC #5 lore raw

In reply to Gary Hade

Quoted 38 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
get_nid_for_pfn() returns int
My mistake. Good catch.
Presumably the (nid < 0) case has never happened.
We do know that it is happening on one system while creating a symlink for a memory section so it should also happen on the same system if unregister_mem_sect_under_nodes() were called to remove the same symlink.
The test was actually added in response to a problem with an earlier version reported by Yasunori Goto where one or more of the leading pages of a memory section on the 2nd node of one of his systems was uninitialized because I believe they coincided with a memory hole. The earlier version did not ignore uninitialized pages and determined the nid by considering only the 1st page of each memory section. This caused the symlink to the 1st memory section on the 2nd node to be incorrectly created in /sys/devices/system/node/node0 instead of /sys/devices/system/node/node1. The problem was fixed by adding the test to skip over uninitialized pages.
I suspect we have not seen any reports of the non-removal of a symlink due to the incorrect declaration of the nid variable in unregister_mem_sect_under_nodes() because
- systems where a memory section could have an uninitialized range of leading pages are probably rare.
- memory remove is probably not done very frequently on the systems that are capable of demonstrating the problem.
- lingering symlink(s) that should have been removed may have simply gone unnoticed.
Quoted 2 lines
Should we retain the test?
Yes.
Quoted 1 line

Quoted 7 lines

Quoted 1 line
Is silently skipping the node in that case desirable behaviour?
It actually silently skips pages (not nodes) in it's quest for valid nids for all the nodes that the memory section scans. This is definitely desirable.
I hope this answers your questions.

This still isn't applied, was it lost?

Roel

Gary Hade

2009-02-27 21:33 UTC #6 lore raw

Quoted 40 lines · roel kluin

Quoted 40 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
get_nid_for_pfn() returns int
My mistake. Good catch.
Presumably the (nid < 0) case has never happened.
We do know that it is happening on one system while creating a symlink for a memory section so it should also happen on the same system if unregister_mem_sect_under_nodes() were called to remove the same symlink.
The test was actually added in response to a problem with an earlier version reported by Yasunori Goto where one or more of the leading pages of a memory section on the 2nd node of one of his systems was uninitialized because I believe they coincided with a memory hole. The earlier version did not ignore uninitialized pages and determined the nid by considering only the 1st page of each memory section. This caused the symlink to the 1st memory section on the 2nd node to be incorrectly created in /sys/devices/system/node/node0 instead of /sys/devices/system/node/node1. The problem was fixed by adding the test to skip over uninitialized pages.
I suspect we have not seen any reports of the non-removal of a symlink due to the incorrect declaration of the nid variable in unregister_mem_sect_under_nodes() because
- systems where a memory section could have an uninitialized range of leading pages are probably rare.
- memory remove is probably not done very frequently on the systems that are capable of demonstrating the problem.
- lingering symlink(s) that should have been removed may have simply gone unnoticed.
Quoted 2 lines
Should we retain the test?
Yes.
Quoted 2 lines
Is silently skipping the node in that case desirable behaviour?

Quoted 7 lines

Quoted 5 lines
It actually silently skips pages (not nodes) in it's quest for valid nids for all the nodes that the memory section scans. This is definitely desirable.
I hope this answers your questions.
This still isn't applied, was it lost?

It is still lingering in -mm:
http://userweb.kernel.org/~akpm/mmotm/broken-out/mm-get_nid_for_pfn-returns-int.patch

Gary

Signature · 12 lines

Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc

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

2009-02-27 21:46 UTC #7 lore raw

Quoted 45 lines · On Fri, 27 Feb 2009 13:33:40 -0800 Gary Hade <[email protected]> wrote:

Quoted 44 lines · roel kluin
Quoted 44 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
get_nid_for_pfn() returns int
My mistake. __Good catch.
Presumably the (nid < 0) case has never happened.
We do know that it is happening on one system while creating a symlink for a memory section so it should also happen on the same system if unregister_mem_sect_under_nodes() were called to remove the same symlink.
The test was actually added in response to a problem with an earlier version reported by Yasunori Goto where one or more of the leading pages of a memory section on the 2nd node of one of his systems was uninitialized because I believe they coincided with a memory hole. __The earlier version did not ignore uninitialized pages and determined the nid by considering only the 1st page of each memory section. __This caused the symlink to the 1st memory section on the 2nd node to be incorrectly created in /sys/devices/system/node/node0 instead of /sys/devices/system/node/node1. __The problem was fixed by adding the test to skip over uninitialized pages.
I suspect we have not seen any reports of the non-removal of a symlink due to the incorrect declaration of the nid variable in unregister_mem_sect_under_nodes() because __- systems where a memory section could have an uninitialized __ __range of leading pages are probably rare.
__- memory remove is probably not done very frequently on the __ __systems that are capable of demonstrating the problem. __- lingering symlink(s) that should have been removed may __ __have simply gone unnoticed.
Quoted 2 lines
Should we retain the test?
Yes.
Quoted 2 lines
Is silently skipping the node in that case desirable behaviour?
It actually silently skips pages (not nodes) in it's quest for valid nids for all the nodes that the memory section scans. This is definitely desirable.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
I hope this answers your questions.
This still isn't applied, was it lost?
It is still lingering in -mm:
http://userweb.kernel.org/~akpm/mmotm/broken-out/mm-get_nid_for_pfn-returns-int.patch

Should it unlinger? I have it in the 2.6.30 pile. Does it actually fix a demonstrable bug?

Gary Hade

2009-02-28 00:14 UTC #8 lore raw

Quoted 50 lines · Andrew Morton

Quoted 47 lines · On Fri, 27 Feb 2009 13:33:40 -0800 Gary Hade <[email protected]> wrote:
Quoted 46 lines · roel kluin
Quoted 45 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
get_nid_for_pfn() returns int
My mistake. __Good catch.
Presumably the (nid < 0) case has never happened.
We do know that it is happening on one system while creating a symlink for a memory section so it should also happen on the same system if unregister_mem_sect_under_nodes() were called to remove the same symlink.
The test was actually added in response to a problem with an earlier version reported by Yasunori Goto where one or more of the leading pages of a memory section on the 2nd node of one of his systems was uninitialized because I believe they coincided with a memory hole. __The earlier version did not ignore uninitialized pages and determined the nid by considering only the 1st page of each memory section. __This caused the symlink to the 1st memory section on the 2nd node to be incorrectly created in /sys/devices/system/node/node0 instead of /sys/devices/system/node/node1. __The problem was fixed by adding the test to skip over uninitialized pages.
I suspect we have not seen any reports of the non-removal of a symlink due to the incorrect declaration of the nid variable in unregister_mem_sect_under_nodes() because __- systems where a memory section could have an uninitialized __ __range of leading pages are probably rare.
__- memory remove is probably not done very frequently on the __ __systems that are capable of demonstrating the problem. __- lingering symlink(s) that should have been removed may __ __have simply gone unnoticed.
Quoted 2 lines
Should we retain the test?
Yes.
Quoted 2 lines
Is silently skipping the node in that case desirable behaviour?
It actually silently skips pages (not nodes) in it's quest for valid nids for all the nodes that the memory section scans. This is definitely desirable.
I hope this answers your questions.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
This still isn't applied, was it lost?
It is still lingering in -mm:
http://userweb.kernel.org/~akpm/mmotm/broken-out/mm-get_nid_for_pfn-returns-int.patch
Should it unlinger? I have it in the 2.6.30 pile.

Yes, that would be good. :)

Quoted 1 line

Does it actually fix a demonstrable bug?

I am not aware of anyone that has actually reproduced the problem. I do not believe that we have any systems where it can be reproduced since it would require both
(1) a memory section with an uninitialized range of pages and
(2) a memory remove event for that memory section.
As far as I know, none of our systems have (1). Yasunori Goto has a system with (1) but I am not sure if he can do (2).

Gary

Signature · 12 lines

Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc

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

2009-02-28 00:23 UTC #9 lore raw

Quoted 9 lines · On Fri, 27 Feb 2009 16:14:00 -0800 Gary Hade <[email protected]> wrote:

Quoted 5 lines · Andrew Morton
Quoted 3 lines
It is still lingering in -mm:
http://userweb.kernel.org/~akpm/mmotm/broken-out/mm-get_nid_for_pfn-returns-int.patch
Should it unlinger? I have it in the 2.6.30 pile.
Yes, that would be good. :)

What would be good? Your answer is ambiguous.

Quoted 4 lines

Quoted 1 line
Does it actually fix a demonstrable bug?
I am not aware of anyone that has actually reproduced the problem.

What problem?

All I gave at present is

From: Roel Kluin <[email protected]>

get_nid_for_pfn() returns int

Signed-off-by Roel Kluin <[email protected]>

Cc Gary Hade <[email protected]>

Quoted 7 lines

I do not believe that we have any systems where
it can be reproduced since it would require both
(1) a memory section with an uninitialized range of pages and
(2) a memory remove event for that memory section.
As far as I know, none of our systems have (1). Yasunori Goto has a system with (1) but I am not sure if he can do (2).

Please send a new changelog for this patch.

If you believe this patch should be merged into 2.6.29 then please explain why. Please also consider whether it should be backported into 2.6.28.x and eariler.

Gary Hade

2009-02-28 03:02 UTC #10 lore raw

Quoted 9 lines · Andrew Morton

Quoted 6 lines · On Fri, 27 Feb 2009 16:14:00 -0800 Gary Hade <[email protected]> wrote:
Quoted 4 lines · Andrew Morton
Quoted 3 lines
It is still lingering in -mm:
http://userweb.kernel.org/~akpm/mmotm/broken-out/mm-get_nid_for_pfn-returns-int.patch

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Should it unlinger? I have it in the 2.6.30 pile.
Yes, that would be good. :)
What would be good? Your answer is ambiguous.

Sorry, I was just trying to agree that your plan to wait until 2.6.30 works for me. Unless someone else objects leave it in your 2.6.30 pile.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Does it actually fix a demonstrable bug?
I am not aware of anyone that has actually reproduced the problem.
What problem?

During a memory remove operation there is a chance on yet to be discovered system(s) that a mem section symlink for a removed memory section could incorrectly persist. Earlier in this thread I described the possible problem as follows.
===
On Mon, Jan 26, 2009 at 10:33:50PM -0800, Andrew Morton wrote:

...

Quoted 1 line

Presumably the (nid < 0) case has never happened.

Quoted 18 lines

All I gave at present is
From: Roel Kluin <[email protected]>
get_nid_for_pfn() returns int
Signed-off-by Roel Kluin <[email protected]>
Cc Gary Hade <[email protected]>
Quoted 7 lines
I do not believe that we have any systems where
it can be reproduced since it would require both
(1) a memory section with an uninitialized range of pages and
(2) a memory remove event for that memory section.
As far as I know, none of our systems have (1). Yasunori Goto has a system with (1) but I am not sure if he can do (2).

Quoted 1 line

Please send a new changelog for this patch.

Can you include the above words?

Quoted 3 lines

If you believe this patch should be merged into 2.6.29 then please explain why.

2.6.30 is fine with me.

Quoted 2 lines

Please also consider whether it should be backported into 2.6.28.x and eariler.

The "mm: show node to memory section relationship with symlinks in sysfs" code that it improves was not introduced until 2.6.29-rc1.

Thanks,
Gary

Signature · 12 lines

Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc

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

2009-02-28 04:08 UTC #11 lore raw

Quoted 9 lines · On Fri, 27 Feb 2009 19:02:00 -0800 Gary Hade <[email protected]> wrote:

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Should it unlinger? I have it in the 2.6.30 pile.
Yes, that would be good. :)
What would be good? Your answer is ambiguous.
Sorry, I was just trying to agree that your plan to wait until 2.6.30 works for me. Unless someone else objects leave it in your 2.6.30 pile.

I object ;)

The change is obviously correct, let's merge it now.

This could cause presently-working systems to stop working due to hitherto-undiscovered bugs. If so, sue me.

Gary Hade

2009-03-01 20:58 UTC #12 lore raw

Quoted 8 lines · Andrew Morton

Quoted 6 lines · On Fri, 27 Feb 2009 19:02:00 -0800 Gary Hade <[email protected]> wrote:
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Should it unlinger? I have it in the 2.6.30 pile.
Yes, that would be good. :)
What would be good? Your answer is ambiguous.

Quoted 7 lines

Quoted 3 lines
Sorry, I was just trying to agree that your plan to wait until 2.6.30 works for me. Unless someone else objects leave it in your 2.6.30 pile.
I object ;)
The change is obviously correct, let's merge it now.

Well, I wouldn't quibble with that. :) Thanks!

Quoted 3 lines

This could cause presently-working systems to stop working due to hitherto-undiscovered bugs. If so, sue me.

Extremely unlikely IMO.

Gary

Signature · 12 lines

Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc

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