[Patch] physnode_map definition should be signed

[Patch] physnode_map definition should be signed

[Yasunori Goto]

Hello Martin-san.

In your modification of pfn_valid() for IA32 at 2.6.4 stock kernel,
it doesn't return 0 even if the node is offline.

True problem is physnode_map's definition.
Physnode_map[]'s default (offline) value is -1,
but it is defined as UNSIGNED 8. 
So, pfn_to_nid() return 255. 

I think this should be defined as signed like this patch.
Maximum node number of IA32 is 16, so this is enough yet.

I found this problem on multi-node emulation for memory-hotplug test.
When I started X on this emulation, system panicked at remap_pte_range()
by this problem.
I think that system will be down when a program will call mmap()
for hardware area.

Could you check this? 
Or, Do you already know this problem?

Thanks.

-----------------------------------------------------------------
 mem_node_hotplug-goto/arch/i386/mm/discontig.c  |    2 +-
 mem_node_hotplug-goto/include/asm-i386/mmzone.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff -puN include/asm-i386/mmzone.h~phys_nodemap_modify include/asm-i386/mmzone.h
--- mem_node_hotplug/include/asm-i386/mmzone.h~phys_nodemap_modify	Wed Mar 31 12:34:33 2004
+++ mem_node_hotplug-goto/include/asm-i386/mmzone.h	Wed Mar 31 12:35:07 2004
@@ -37,7 +37,7 @@ extern struct pglist_data *node_data[];
 #define MAX_ELEMENTS 256
 #define PAGES_PER_ELEMENT (MAX_NR_PAGES/MAX_ELEMENTS)
 
-extern u8 physnode_map[];
+extern s8 physnode_map[];
 
 static inline int pfn_to_nid(unsigned long pfn)
 {
diff -puN arch/i386/mm/discontig.c~phys_nodemap_modify arch/i386/mm/discontig.c
--- mem_node_hotplug/arch/i386/mm/discontig.c~phys_nodemap_modify	Wed Mar 31 12:34:43 2004
+++ mem_node_hotplug-goto/arch/i386/mm/discontig.c	Wed Mar 31 12:36:25 2004
@@ -56,7 +56,7 @@ bootmem_data_t node0_bdata;
  *     physnode_map[4-7] = 1;
  *     physnode_map[8- ] = -1;
  */
-u8 physnode_map[MAX_ELEMENTS] = { [0 ... (MAX_ELEMENTS - 1)] = -1};
+s8 physnode_map[MAX_ELEMENTS] = { [0 ... (MAX_ELEMENTS - 1)] = -1};
 
 unsigned long node_start_pfn[MAX_NUMNODES];
 unsigned long node_end_pfn[MAX_NUMNODES];


-- 
Yasunori Goto <ygoto at us.fujitsu.com>

Re: [Patch] physnode_map definition should be signed

[Martin J. Bligh]

> In your modification of pfn_valid() for IA32 at 2.6.4 stock kernel,
> it doesn't return 0 even if the node is offline.
> 
> True problem is physnode_map's definition.
> Physnode_map[]'s default (offline) value is -1,
> but it is defined as UNSIGNED 8. 
> So, pfn_to_nid() return 255. 
> 
> I think this should be defined as signed like this patch.
> Maximum node number of IA32 is 16, so this is enough yet.
> 
> I found this problem on multi-node emulation for memory-hotplug test.
> When I started X on this emulation, system panicked at remap_pte_range()
> by this problem.
> I think that system will be down when a program will call mmap()
> for hardware area.

There are several breakages in this area, particularly on Summit with
4/4 split right now - in my tree I'm init'ing the array to 0 temporarily
to get around some problems, but we have better fixes lined up.

Anyway, your patch looks correct - 128 nodes should be plenty. I shall
apply it. Thanks,

M.