Re: [PATCH 1/4] x86_64: Fix ACPI NUMA parsing

[Matthew Dobson]

On Tue, 2005-01-11 at 23:01, Andi Kleen wrote:
> Fix SRAT NUMA parsing
> 
> Fix fallout from the recent nodemask_t changes. The node ids assigned 
> in the SRAT parser were off by one.
> 
> I added a new first_unset_node() function to nodemask.h to allocate
> IDs sanely.
> 
> Signed-off-by: Andi Kleen <ak@xxxxxxx>
> 
> Index: linux/arch/x86_64/mm/srat.c
> ===================================================================
> --- linux.orig/arch/x86_64/mm/srat.c	2005-01-09 18:19:17.%N +0100
> +++ linux/arch/x86_64/mm/srat.c	2005-01-12 04:15:43.%N +0100
> @@ -20,17 +20,20 @@
>  
>  static struct acpi_table_slit *acpi_slit;
>  
> -static DECLARE_BITMAP(nodes_parsed, MAX_NUMNODES) __initdata;
> +static nodemask_t nodes_parsed __initdata;
> +static nodemask_t nodes_found __initdata;
>  static struct node nodes[MAX_NUMNODES] __initdata;
>  static __u8  pxm2node[256] __initdata = { [0 ... 255] = 0xff };
>  
>  static __init int setup_node(int pxm)
>  {
> -	if (pxm2node[pxm] == 0xff) {
> -		if (num_online_nodes() >= MAX_NUMNODES)
> +	unsigned node = pxm2node[pxm];
> +	if (node == 0xff) {
> +		if (nodes_weight(nodes_found) >= MAX_NUMNODES)
>  			return -1;
> -		pxm2node[pxm] = num_online_nodes();
> -		node_set_online(num_online_nodes());
> +		node = first_unset_node(nodes_found); 
> +		node_set(node, nodes_found);
> +		pxm2node[pxm] = node;
>  	}
>  	return pxm2node[pxm];
>  }

This snippet looks incorrect because first_unset_node() can return
MAX_NUMNODES, which isn't a valid node number, nor is it valid to set a
bit >= MAX_NUMNODES in a nodemask.  Although, this is init code, so
there shouldn't be other CPUs messing with the nodes_found mask at the
same time, and you do check that there are unset bits in nodes_found
before the first_unset_node() call, so I'm probably wrong.  Is there any
reason you don't just do all these checks on node_online_map?  ie:

	if (nodes_weight(node_online_map) >= MAX_NUMNODES)
		return -1;
	node = first_unset_node(node_online_map); 
	node_set(node, nodes_found);
	pxm2node[pxm] = node;

The extra nodes_found bitmask seems unnecessary?  I like the idea of the
first_unset_node() function, though.

-Matt

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/