[PATCH v2 0/3] Updates to pxm2node mapping and nodeID sizing

[PATCH v2 0/3] Updates to pxm2node mapping and nodeID sizing

[Boris Ostrovsky]

Changes in v2 (only patch #2 is v2, the others are new)

* Added patch #1 to make pxm-to-node mapping independent of PXM values
* Added in patch #3 a couple of macros to access nodeID in memflags


Boris Ostrovsky (3):
  x86/numa: Allow arbitrary value of PXM in PXM<->node mapping
  x86/numa: Adjust datatypes for node and pxm
  mm: MEMF_node should handle changes in nodeid_t size

 xen/arch/x86/irq.c                  |    4 +-
 xen/arch/x86/numa.c                 |   15 ++--
 xen/arch/x86/setup.c                |    2 +-
 xen/arch/x86/smpboot.c              |    5 +-
 xen/arch/x86/srat.c                 |  122 ++++++++++++++++++++++++-----------
 xen/arch/x86/x86_64/mm.c            |    5 +-
 xen/common/page_alloc.c             |    5 +-
 xen/drivers/passthrough/vtd/iommu.c |    5 +-
 xen/include/asm-arm/numa.h          |    4 +-
 xen/include/asm-x86/irq.h           |    3 +-
 xen/include/asm-x86/numa.h          |   24 ++++---
 xen/include/xen/mm.h                |    6 ++-
 12 files changed, 130 insertions(+), 70 deletions(-)

[PATCH v2 1/3] x86/numa: Allow arbitrary value of PXM in PXM<->node mapping

[Boris Ostrovsky]

ACPI defines proximity domain identifier as a 32-bit integer. While
in most cases the values will be zero-based this is not guaranteed,
making current pxm2node[256] mapping structure not appropriate.

We will instead use MAX_NUMNODES-sized array of struct pxm2node to
store PXM-to-node mapping. To accommodate common case of zero-based
and contiguios PXMs we will, whenever possible, try to use PXM as
index into this array array for fast lookups.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/srat.c |   93 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 68 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 29fc724..c8841b9 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -27,35 +27,79 @@ static nodemask_t memory_nodes_parsed __initdata;
 static nodemask_t processor_nodes_parsed __initdata;
 static nodemask_t nodes_found __initdata;
 static struct node nodes[MAX_NUMNODES] __initdata;
-static u8 __read_mostly pxm2node[256] = { [0 ... 255] = NUMA_NO_NODE };
 
+struct pxm2node {
+    unsigned pxm;
+    int node;
+};
+static struct pxm2node __read_mostly p2n[MAX_NUMNODES] =
+    { [0 ... MAX_NUMNODES - 1] = {.node = NUMA_NO_NODE} };
+
+static int node_to_pxm(int n);
 
 static int num_node_memblks;
 static struct node node_memblk_range[NR_NODE_MEMBLKS];
 static int memblk_nodeid[NR_NODE_MEMBLKS];
 
-
-static int node_to_pxm(int n);
+static inline bool_t node_found(unsigned idx, unsigned pxm)
+{
+    return ((p2n[idx].pxm == pxm) &&
+            (p2n[idx].node != NUMA_NO_NODE));
+}
 
 int pxm_to_node(int pxm)
 {
-	if ((unsigned)pxm >= 256)
-		return -1;
-	/* Extend 0xff to (int)-1 */
-	return (signed char)pxm2node[pxm];
+    unsigned i;
+
+    if ( (pxm < MAX_NUMNODES) && node_found(pxm, pxm) )
+        return p2n[pxm].node;
+
+    for ( i = 0; i < MAX_NUMNODES; i++ )
+        if ( node_found(i, pxm) )
+            return p2n[i].node;
+
+    /* Extend 0xff to (int)-1 */
+    return (signed char)NUMA_NO_NODE;
 }
 
 __devinit int setup_node(int pxm)
 {
-	unsigned node = pxm2node[pxm];
-	if (node == 0xff) {
-		if (nodes_weight(nodes_found) >= MAX_NUMNODES)
-			return -1;
-		node = first_unset_node(nodes_found); 
-		node_set(node, nodes_found);
-		pxm2node[pxm] = node;
-	}
-	return pxm2node[pxm];
+    int node;
+    unsigned idx;
+    static bool_t warned;
+
+    if ( pxm < MAX_NUMNODES )
+    {
+        if ( node_found(pxm, pxm) )
+            return p2n[pxm].node;
+
+        /* Try to maintain indexing of p2n by pxm */
+        if ( p2n[pxm].node == NUMA_NO_NODE )
+        {
+            idx = pxm;
+            goto finish;
+        }
+    }
+
+    for ( idx = 0; idx < MAX_NUMNODES; idx++ )
+        if ( p2n[idx].node == NUMA_NO_NODE )
+            goto finish;
+
+    if ( !warned )
+    {
+        printk(XENLOG_WARNING "More PXMs than available nodes\n");
+        warned = 1;
+    }
+
+    return (signed char)NUMA_NO_NODE;
+
+ finish:
+    node = first_unset_node(nodes_found);
+    node_set(node, nodes_found);
+    p2n[idx].pxm = pxm;
+    p2n[pxm].node = node;
+
+    return node;
 }
 
 int valid_numa_range(u64 start, u64 end, int node)
@@ -111,8 +155,6 @@ static __init void bad_srat(void)
 	acpi_numa = -1;
 	for (i = 0; i < MAX_LOCAL_APIC; i++)
 		apicid_to_node[i] = NUMA_NO_NODE;
-	for (i = 0; i < ARRAY_SIZE(pxm2node); i++)
-		pxm2node[i] = NUMA_NO_NODE;
 	mem_hotplug = 0;
 }
 
@@ -440,13 +482,14 @@ int __init acpi_scan_nodes(u64 start, u64 end)
 
 static int node_to_pxm(int n)
 {
-       int i;
-       if (pxm2node[n] == n)
-               return n;
-       for (i = 0; i < 256; i++)
-               if (pxm2node[i] == n)
-                       return i;
-       return 0;
+    unsigned i;
+
+    if ( p2n[n].node == n )
+        return p2n[n].pxm;
+    for ( i = 0; i < MAX_NUMNODES; i++ )
+        if ( p2n[i].node == n )
+            return p2n[i].pxm;
+    return 0;
 }
 
 int __node_distance(int a, int b)
-- 
1.7.1

Re: [PATCH v2 1/3] x86/numa: Allow arbitrary value of PXM in PXM<->node mapping

[Boris Ostrovsky]

On 02/24/2015 02:11 PM, Boris Ostrovsky wrote:
>   __devinit int setup_node(int pxm)
>   {

...

> +
> + finish:
> +    node = first_unset_node(nodes_found);
> +    node_set(node, nodes_found);
> +    p2n[idx].pxm = pxm;
> +    p2n[pxm].node = node;


This is a typo. Should be

     p2n[idx].node = node;

-boris

Re: [PATCH v2 1/3] x86/numa: Allow arbitrary value of PXM in PXM<->node mapping

[Jan Beulich]

>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -27,35 +27,79 @@ static nodemask_t memory_nodes_parsed __initdata;
>  static nodemask_t processor_nodes_parsed __initdata;
>  static nodemask_t nodes_found __initdata;
>  static struct node nodes[MAX_NUMNODES] __initdata;
> -static u8 __read_mostly pxm2node[256] = { [0 ... 255] = NUMA_NO_NODE };
>  
> +struct pxm2node {
> +    unsigned pxm;
> +    int node;
> +};
> +static struct pxm2node __read_mostly p2n[MAX_NUMNODES] =

Can this please continue to be named pxm2node[]? "p2..." is too
commonly used for representing a phys-to-something conversion.

> +    { [0 ... MAX_NUMNODES - 1] = {.node = NUMA_NO_NODE} };
> +
> +static int node_to_pxm(int n);

I don't see why you need to reinstate this forward declaration.

>  int pxm_to_node(int pxm)
>  {
> -	if ((unsigned)pxm >= 256)
> -		return -1;
> -	/* Extend 0xff to (int)-1 */
> -	return (signed char)pxm2node[pxm];
> +    unsigned i;
> +
> +    if ( (pxm < MAX_NUMNODES) && node_found(pxm, pxm) )
> +        return p2n[pxm].node;

With this the function parameter's type can't remain to be signed.
Also please don't change coding style here, unless you do so for
the whole file (in a separate patch). The one change I wouldn't
mind is switching node_to_pxm() from 8-space to tab indentation.

>  __devinit int setup_node(int pxm)
>  {
> -	unsigned node = pxm2node[pxm];
> -	if (node == 0xff) {
> -		if (nodes_weight(nodes_found) >= MAX_NUMNODES)
> -			return -1;
> -		node = first_unset_node(nodes_found); 
> -		node_set(node, nodes_found);
> -		pxm2node[pxm] = node;
> -	}
> -	return pxm2node[pxm];
> +    int node;
> +    unsigned idx;
> +    static bool_t warned;
> +
> +    if ( pxm < MAX_NUMNODES )
> +    {
> +        if ( node_found(pxm, pxm) )
> +            return p2n[pxm].node;
> +
> +        /* Try to maintain indexing of p2n by pxm */

Coding style.

> @@ -111,8 +155,6 @@ static __init void bad_srat(void)
>  	acpi_numa = -1;
>  	for (i = 0; i < MAX_LOCAL_APIC; i++)
>  		apicid_to_node[i] = NUMA_NO_NODE;
> -	for (i = 0; i < ARRAY_SIZE(pxm2node); i++)
> -		pxm2node[i] = NUMA_NO_NODE;

Without any replacement?

Jan

Re: [PATCH v2 1/3] x86/numa: Allow arbitrary value of PXM in PXM<->node mapping

[Boris Ostrovsky]

On 02/25/2015 04:34 AM, Jan Beulich wrote:
>>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -27,35 +27,79 @@ static nodemask_t memory_nodes_parsed __initdata;
>>   static nodemask_t processor_nodes_parsed __initdata;
>>   static nodemask_t nodes_found __initdata;
>>   static struct node nodes[MAX_NUMNODES] __initdata;
>> -static u8 __read_mostly pxm2node[256] = { [0 ... 255] = NUMA_NO_NODE };
>>   
>> +struct pxm2node {
>> +    unsigned pxm;
>> +    int node;
>> +};
>> +static struct pxm2node __read_mostly p2n[MAX_NUMNODES] =
> Can this please continue to be named pxm2node[]? "p2..." is too
> commonly used for representing a phys-to-something conversion.
>
>> +    { [0 ... MAX_NUMNODES - 1] = {.node = NUMA_NO_NODE} };
>> +
>> +static int node_to_pxm(int n);
> I don't see why you need to reinstate this forward declaration.

It is used in acpi_numa_memory_affinity_init() which is defined above 
node_to_pxm().

I can just move node_to_pxm() definition higher and drop forward 
declaration.

>
>>   int pxm_to_node(int pxm)
>>   {
>> -	if ((unsigned)pxm >= 256)
>> -		return -1;
>> -	/* Extend 0xff to (int)-1 */
>> -	return (signed char)pxm2node[pxm];
>> +    unsigned i;
>> +
>> +    if ( (pxm < MAX_NUMNODES) && node_found(pxm, pxm) )
>> +        return p2n[pxm].node;
> With this the function parameter's type can't remain to be signed.

Right. I was hoping to defer all type changes to the next patch but this 
(and setup_node() below) will need to change here.

-boris

> Also please don't change coding style here, unless you do so for
> the whole file (in a separate patch). The one change I wouldn't
> mind is switching node_to_pxm() from 8-space to tab indentation.
>
>>   __devinit int setup_node(int pxm)
>>   {
>> -	unsigned node = pxm2node[pxm];
>> -	if (node == 0xff) {
>> -		if (nodes_weight(nodes_found) >= MAX_NUMNODES)
>> -			return -1;
>> -		node = first_unset_node(nodes_found);
>> -		node_set(node, nodes_found);
>> -		pxm2node[pxm] = node;
>> -	}
>> -	return pxm2node[pxm];
>> +    int node;
>> +    unsigned idx;
>> +    static bool_t warned;
>> +
>> +    if ( pxm < MAX_NUMNODES )
>> +    {
>> +        if ( node_found(pxm, pxm) )
>> +            return p2n[pxm].node;

Re: [PATCH v2 1/3] x86/numa: Allow arbitrary value of PXM in PXM<->node mapping

[Jan Beulich]

>>> On 25.02.15 at 14:11, <boris.ostrovsky@oracle.com> wrote:
> On 02/25/2015 04:34 AM, Jan Beulich wrote:
>>>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
>>> +    { [0 ... MAX_NUMNODES - 1] = {.node = NUMA_NO_NODE} };
>>> +
>>> +static int node_to_pxm(int n);
>> I don't see why you need to reinstate this forward declaration.
> 
> It is used in acpi_numa_memory_affinity_init() which is defined above 
> node_to_pxm().
> 
> I can just move node_to_pxm() definition higher and drop forward 
> declaration.

Looks like I mixed this up with ...

>>>   int pxm_to_node(int pxm)

... this. Keep things as they are then please.

Jan

Re: [PATCH v2 1/3] x86/numa: Allow arbitrary value of PXM in PXM<->node mapping

[Jan Beulich]

>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -27,35 +27,79 @@ static nodemask_t memory_nodes_parsed __initdata;
>  static nodemask_t processor_nodes_parsed __initdata;
>  static nodemask_t nodes_found __initdata;
>  static struct node nodes[MAX_NUMNODES] __initdata;
> -static u8 __read_mostly pxm2node[256] = { [0 ... 255] = NUMA_NO_NODE };
>  
> +struct pxm2node {
> +    unsigned pxm;
> +    int node;
> +};
> +static struct pxm2node __read_mostly p2n[MAX_NUMNODES] =
> +    { [0 ... MAX_NUMNODES - 1] = {.node = NUMA_NO_NODE} };

Type and initializer don't match up.

Jan

[PATCH v2 2/3] x86/numa: Adjust datatypes for node and pxm

[Boris Ostrovsky]

Use u8-sized node IDs and unsigned PXMs consistently throughout
code (and introduce nodeid_t type).

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---

Changes (due to new pxm-to-node mapping from patch #1):
 * No MAX_PXM
 * No need to BUILD_BUG_ON(MAX_NUMNODES > 254)


 xen/arch/x86/irq.c                  |    4 +-
 xen/arch/x86/numa.c                 |   15 ++++++------
 xen/arch/x86/setup.c                |    2 +-
 xen/arch/x86/smpboot.c              |    5 ++-
 xen/arch/x86/srat.c                 |   41 ++++++++++++++++++----------------
 xen/arch/x86/x86_64/mm.c            |    5 ++-
 xen/common/page_alloc.c             |    5 ++-
 xen/drivers/passthrough/vtd/iommu.c |    5 ++-
 xen/include/asm-arm/numa.h          |    4 ++-
 xen/include/asm-x86/irq.h           |    3 +-
 xen/include/asm-x86/numa.h          |   24 +++++++++++---------
 11 files changed, 63 insertions(+), 50 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index f214072..9be8840 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -153,7 +153,7 @@ int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
 /*
  * Dynamic irq allocate and deallocation for MSI
  */
-int create_irq(int node)
+int create_irq(nodeid_t node)
 {
     int irq, ret;
     struct irq_desc *desc;
@@ -173,7 +173,7 @@ int create_irq(int node)
     {
         cpumask_t *mask = NULL;
 
-        if (node != NUMA_NO_NODE && node >= 0)
+        if ( node != NUMA_NO_NODE )
         {
             mask = &node_to_cpumask(node);
             if (cpumask_empty(mask))
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 628a40a..7a01923 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -35,13 +35,13 @@ static typeof(*memnodemap) _memnodemap[64];
 unsigned long memnodemapsize;
 u8 *memnodemap;
 
-unsigned char cpu_to_node[NR_CPUS] __read_mostly = {
+nodeid_t cpu_to_node[NR_CPUS] __read_mostly = {
     [0 ... NR_CPUS-1] = NUMA_NO_NODE
 };
 /*
  * Keep BIOS's CPU2node information, should not be used for memory allocaion
  */
-unsigned char apicid_to_node[MAX_LOCAL_APIC] __cpuinitdata = {
+nodeid_t apicid_to_node[MAX_LOCAL_APIC] __cpuinitdata = {
     [0 ... MAX_LOCAL_APIC-1] = NUMA_NO_NODE
 };
 cpumask_t node_to_cpumask[MAX_NUMNODES] __read_mostly;
@@ -65,7 +65,7 @@ int srat_disabled(void)
  * -1 if node overlap or lost ram (shift too big)
  */
 static int __init populate_memnodemap(const struct node *nodes,
-                                      int numnodes, int shift, int *nodeids)
+                                      int numnodes, int shift, nodeid_t *nodeids)
 {
     unsigned long spdx, epdx;
     int i, res = -1;
@@ -150,7 +150,7 @@ static int __init extract_lsb_from_nodes(const struct node *nodes,
 }
 
 int __init compute_hash_shift(struct node *nodes, int numnodes,
-                              int *nodeids)
+                              nodeid_t *nodeids)
 {
     int shift;
 
@@ -172,7 +172,7 @@ int __init compute_hash_shift(struct node *nodes, int numnodes,
     return shift;
 }
 /* initialize NODE_DATA given nodeid and start/end */
-void __init setup_node_bootmem(int nodeid, u64 start, u64 end)
+void __init setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end)
 { 
     unsigned long start_pfn, end_pfn;
 
@@ -294,7 +294,7 @@ __cpuinit void numa_add_cpu(int cpu)
     cpumask_set_cpu(cpu, &node_to_cpumask[cpu_to_node(cpu)]);
 } 
 
-void __cpuinit numa_set_node(int cpu, int node)
+void __cpuinit numa_set_node(int cpu, nodeid_t node)
 {
     cpu_to_node[cpu] = node;
 }
@@ -340,7 +340,8 @@ static __init int numa_setup(char *opt)
  */
 void __init init_cpu_to_node(void)
 {
-    int i, node;
+    unsigned i;
+    nodeid_t node;
 
     for ( i = 0; i < nr_cpu_ids; i++ )
     {
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c27c49c..84da722 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -188,7 +188,7 @@ static void __init init_idle_domain(void)
 
 void __devinit srat_detect_node(int cpu)
 {
-    unsigned node;
+    nodeid_t node;
     u32 apicid = x86_cpu_to_apicid[cpu];
 
     node = apicid_to_node[apicid];
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index c54be7e..3d970dd 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -843,7 +843,8 @@ void __cpu_die(unsigned int cpu)
 
 int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm)
 {
-    int node, cpu = -1;
+    nodeid_t node;
+    int cpu = -1;
 
     dprintk(XENLOG_DEBUG, "cpu_add apic_id %x acpi_id %x pxm %x\n",
             apic_id, acpi_id, pxm);
@@ -877,7 +878,7 @@ int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm)
 
     if ( !srat_disabled() )
     {
-        if ( (node = setup_node(pxm)) < 0 )
+        if ( (node = setup_node(pxm)) == NUMA_NO_NODE )
         {
             dprintk(XENLOG_WARNING,
                     "Setup node failed for pxm %x\n", pxm);
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index c8841b9..63569f9 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -30,16 +30,16 @@ static struct node nodes[MAX_NUMNODES] __initdata;
 
 struct pxm2node {
     unsigned pxm;
-    int node;
+    nodeid_t node;
 };
 static struct pxm2node __read_mostly p2n[MAX_NUMNODES] =
     { [0 ... MAX_NUMNODES - 1] = {.node = NUMA_NO_NODE} };
 
-static int node_to_pxm(int n);
+static unsigned node_to_pxm(nodeid_t n);
 
 static int num_node_memblks;
 static struct node node_memblk_range[NR_NODE_MEMBLKS];
-static int memblk_nodeid[NR_NODE_MEMBLKS];
+static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
 
 static inline bool_t node_found(unsigned idx, unsigned pxm)
 {
@@ -47,7 +47,7 @@ static inline bool_t node_found(unsigned idx, unsigned pxm)
             (p2n[idx].node != NUMA_NO_NODE));
 }
 
-int pxm_to_node(int pxm)
+nodeid_t pxm_to_node(unsigned pxm)
 {
     unsigned i;
 
@@ -58,13 +58,12 @@ int pxm_to_node(int pxm)
         if ( node_found(i, pxm) )
             return p2n[i].node;
 
-    /* Extend 0xff to (int)-1 */
-    return (signed char)NUMA_NO_NODE;
+    return NUMA_NO_NODE;
 }
 
-__devinit int setup_node(int pxm)
+__devinit nodeid_t setup_node(unsigned pxm)
 {
-    int node;
+    nodeid_t node;
     unsigned idx;
     static bool_t warned;
 
@@ -91,7 +90,7 @@ __devinit int setup_node(int pxm)
         warned = 1;
     }
 
-    return (signed char)NUMA_NO_NODE;
+    return NUMA_NO_NODE;
 
  finish:
     node = first_unset_node(nodes_found);
@@ -102,7 +101,7 @@ __devinit int setup_node(int pxm)
     return node;
 }
 
-int valid_numa_range(u64 start, u64 end, int node)
+int valid_numa_range(u64 start, u64 end, nodeid_t node)
 {
 	int i;
 
@@ -204,8 +203,9 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 void __init
 acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 {
-	int pxm, node;
-	int apic_id;
+	unsigned pxm;
+	nodeid_t node;
+	u32 apic_id;
 
 	if (srat_disabled())
 		return;
@@ -217,7 +217,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 		return;
 	pxm = pa->proximity_domain;
 	node = setup_node(pxm);
-	if (node < 0) {
+	if (node == NUMA_NO_NODE) {
 		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
 		bad_srat();
 		return;
@@ -234,7 +234,9 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 void __init
 acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 {
-	int pxm, node;
+	unsigned pxm;
+	nodeid_t node;
+
 	if (srat_disabled())
 		return;
 	if (pa->header.length != sizeof(struct acpi_srat_cpu_affinity)) {
@@ -250,7 +252,7 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 		pxm |= pa->proximity_domain_hi[2] << 24;
 	}
 	node = setup_node(pxm);
-	if (node < 0) {
+	if (node == NUMA_NO_NODE) {
 		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
 		bad_srat();
 		return;
@@ -268,7 +270,8 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 {
 	struct node *nd;
 	u64 start, end;
-	int node, pxm;
+	unsigned pxm;
+	nodeid_t node;
 	int i;
 
 	if (srat_disabled())
@@ -294,7 +297,7 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 	if (srat_rev < 2)
 		pxm &= 0xff;
 	node = setup_node(pxm);
-	if (node < 0) {
+	if (node == NUMA_NO_NODE) {
 		printk(KERN_ERR "SRAT: Too many proximity domains.\n");
 		bad_srat();
 		return;
@@ -480,7 +483,7 @@ int __init acpi_scan_nodes(u64 start, u64 end)
 	return 0;
 }
 
-static int node_to_pxm(int n)
+static unsigned node_to_pxm(nodeid_t n)
 {
     unsigned i;
 
@@ -492,7 +495,7 @@ static int node_to_pxm(int n)
     return 0;
 }
 
-int __node_distance(int a, int b)
+int __node_distance(nodeid_t a, nodeid_t b)
 {
 	int index;
 
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index d631aee..6875c92 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1343,7 +1343,8 @@ int mem_hotadd_check(unsigned long spfn, unsigned long epfn)
 int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
 {
     struct mem_hotadd_info info;
-    int ret, node;
+    int ret;
+    nodeid_t node;
     unsigned long old_max = max_page, old_total = total_pages;
     unsigned long old_node_start, old_node_span, orig_online;
     unsigned long i;
@@ -1353,7 +1354,7 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
     if ( !mem_hotadd_check(spfn, epfn) )
         return -EINVAL;
 
-    if ( (node = setup_node(pxm)) == -1 )
+    if ( (node = setup_node(pxm)) == NUMA_NO_NODE )
         return -EINVAL;
 
     if ( !valid_numa_range(spfn << PAGE_SHIFT, epfn << PAGE_SHIFT, node) )
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 7b4092d..124fa88 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -581,7 +581,7 @@ static struct page_info *alloc_heap_pages(
     struct domain *d)
 {
     unsigned int first_node, i, j, zone = 0, nodemask_retry = 0;
-    unsigned int node = (uint8_t)((memflags >> _MEMF_node) - 1);
+    nodeid_t node = (nodeid_t)((memflags >> _MEMF_node) - 1);
     unsigned long request = 1UL << order;
     struct page_info *pg;
     nodemask_t nodemask = (d != NULL ) ? d->node_affinity : node_online_map;
@@ -1278,7 +1278,8 @@ static void __init smp_scrub_heap_pages(void *data)
     unsigned long mfn, start, end;
     struct page_info *pg;
     struct scrub_region *r;
-    unsigned int temp_cpu, node, cpu_idx = 0;
+    unsigned int temp_cpu, cpu_idx = 0;
+    nodeid_t node;
     unsigned int cpu = smp_processor_id();
 
     if ( data )
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 19d8165..2e0af6f 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -190,14 +190,15 @@ u64 alloc_pgtable_maddr(struct acpi_drhd_unit *drhd, unsigned long npages)
     struct acpi_rhsa_unit *rhsa;
     struct page_info *pg, *cur_pg;
     u64 *vaddr;
-    int node = -1, i;
+    nodeid_t node = NUMA_NO_NODE;
+    unsigned i;
 
     rhsa = drhd_to_rhsa(drhd);
     if ( rhsa )
         node =  pxm_to_node(rhsa->proximity_domain);
 
     pg = alloc_domheap_pages(NULL, get_order_from_pages(npages),
-                             (node == -1 ) ? 0 : MEMF_node(node));
+                             (node == NUMA_NO_NODE) ? 0 : MEMF_node(node));
     if ( !pg )
         return 0;
 
diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
index 06a9d5a..a00cb7c 100644
--- a/xen/include/asm-arm/numa.h
+++ b/xen/include/asm-arm/numa.h
@@ -1,11 +1,13 @@
 #ifndef __ARCH_ARM_NUMA_H
 #define __ARCH_ARM_NUMA_H
 
+typedef u8 nodeid_t;
+
 /* Fake one node for now. See also node_online_map. */
 #define cpu_to_node(cpu) 0
 #define node_to_cpumask(node)   (cpu_online_map)
 
-static inline __attribute__((pure)) int phys_to_nid(paddr_t addr)
+static inline __attribute__((pure)) nodeid_t phys_to_nid(paddr_t addr)
 {
     return 0;
 }
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index d3c55f3..a44305e 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -5,6 +5,7 @@
 
 #include <xen/config.h>
 #include <asm/atomic.h>
+#include <asm/numa.h>
 #include <xen/cpumask.h>
 #include <xen/smp.h>
 #include <xen/hvm/irq.h>
@@ -155,7 +156,7 @@ int  init_irq_data(void);
 void clear_irq_vector(int irq);
 
 int irq_to_vector(int irq);
-int create_irq(int node);
+int create_irq(nodeid_t node);
 void destroy_irq(unsigned int irq);
 int assign_irq_vector(int irq, const cpumask_t *);
 
diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
index 5959860..419f770 100644
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -5,6 +5,8 @@
 
 #define NODES_SHIFT 6
 
+typedef u8 nodeid_t;
+
 extern int srat_rev;
 
 extern unsigned char cpu_to_node[];
@@ -20,8 +22,8 @@ struct node {
 };
 
 extern int compute_hash_shift(struct node *nodes, int numnodes,
-			      int *nodeids);
-extern int pxm_to_node(int nid);
+			      nodeid_t *nodeids);
+extern nodeid_t pxm_to_node(unsigned pxm);
 
 #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT))
 #define VIRTUAL_BUG_ON(x) 
@@ -32,12 +34,12 @@ extern int numa_off;
 
 
 extern int srat_disabled(void);
-extern void numa_set_node(int cpu, int node);
-extern int setup_node(int pxm);
+extern void numa_set_node(int cpu, nodeid_t node);
+extern nodeid_t setup_node(unsigned pxm);
 extern void srat_detect_node(int cpu);
 
-extern void setup_node_bootmem(int nodeid, u64 start, u64 end);
-extern unsigned char apicid_to_node[];
+extern void setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end);
+extern nodeid_t apicid_to_node[];
 #ifdef CONFIG_NUMA
 extern void init_cpu_to_node(void);
 
@@ -54,14 +56,14 @@ extern u8 *memnodemap;
 struct node_data {
     unsigned long node_start_pfn;
     unsigned long node_spanned_pages;
-    unsigned int  node_id;
+    nodeid_t      node_id;
 };
 
 extern struct node_data node_data[];
 
-static inline __attribute__((pure)) int phys_to_nid(paddr_t addr) 
+static inline __attribute__((pure)) nodeid_t phys_to_nid(paddr_t addr) 
 { 
-	unsigned nid;
+	nodeid_t nid;
 	VIRTUAL_BUG_ON((paddr_to_pdx(addr) >> memnode_shift) >= memnodemapsize);
 	nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift]; 
 	VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || !node_data[nid]); 
@@ -75,7 +77,7 @@ static inline __attribute__((pure)) int phys_to_nid(paddr_t addr)
 #define node_end_pfn(nid)       (NODE_DATA(nid)->node_start_pfn + \
 				 NODE_DATA(nid)->node_spanned_pages)
 
-extern int valid_numa_range(u64 start, u64 end, int node);
+extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
 #else
 #define init_cpu_to_node() do {} while (0)
 #define clear_node_cpumask(cpu) do {} while (0)
@@ -83,6 +85,6 @@ extern int valid_numa_range(u64 start, u64 end, int node);
 #endif
 
 void srat_parse_regions(u64 addr);
-extern int __node_distance(int a, int b);
+extern int __node_distance(nodeid_t a, nodeid_t b);
 
 #endif
-- 
1.7.1

Re: [PATCH v2 2/3] x86/numa: Adjust datatypes for node and pxm

[Jan Beulich]

>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -843,7 +843,8 @@ void __cpu_die(unsigned int cpu)
>  
>  int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm)
>  {
> -    int node, cpu = -1;
> +    nodeid_t node;

Please take the opportunity and move the declaration into the
scope it's actually needed in, at once allowing the call the
setup_node() to become its initializer.

Jan

[PATCH v2 3/3] mm: MEMF_node should handle changes in nodeid_t size

[Boris Ostrovsky]

Instead of using a hardcoded constant to extract nodeID from
memflags use a macro whose value is based on nodeid_t size.

Also provide a macro for extracting nodeID from memflags so that
users don't need to remember to decrement the value.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/common/page_alloc.c |    2 +-
 xen/include/xen/mm.h    |    6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 124fa88..367b5bd 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -581,7 +581,7 @@ static struct page_info *alloc_heap_pages(
     struct domain *d)
 {
     unsigned int first_node, i, j, zone = 0, nodemask_retry = 0;
-    nodeid_t node = (nodeid_t)((memflags >> _MEMF_node) - 1);
+    nodeid_t node = MEMF2NODE(memflags);
     unsigned long request = 1UL << order;
     struct page_info *pg;
     nodemask_t nodemask = (d != NULL ) ? d->node_affinity : node_online_map;
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 74a65a6..c86c487 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -110,6 +110,8 @@ struct npfec {
 };
 
 /* memflags: */
+#define MEMF_node_mask    ((1U << sizeof(nodeid_t)) - 1)
+
 #define _MEMF_no_refcount 0
 #define  MEMF_no_refcount (1U<<_MEMF_no_refcount)
 #define _MEMF_populate_on_demand 1
@@ -121,10 +123,12 @@ struct npfec {
 #define _MEMF_exact_node  4
 #define  MEMF_exact_node  (1U<<_MEMF_exact_node)
 #define _MEMF_node        8
-#define  MEMF_node(n)     ((((n)+1)&0xff)<<_MEMF_node)
+#define  MEMF_node(n)     ((((n)+1) & MEMF_node_mask) << _MEMF_node)
 #define _MEMF_bits        24
 #define  MEMF_bits(n)     ((n)<<_MEMF_bits)
 
+#define MEMF2NODE(memflags) (MASK_EXTR(memflags, MEMF_node_mask) - 1)
+
 #ifdef CONFIG_PAGEALLOC_MAX_ORDER
 #define MAX_ORDER CONFIG_PAGEALLOC_MAX_ORDER
 #else
-- 
1.7.1

Re: [PATCH v2 3/3] mm: MEMF_node should handle changes in nodeid_t size

[Jan Beulich]

>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
> Instead of using a hardcoded constant to extract nodeID from
> memflags use a macro whose value is based on nodeid_t size.
> 
> Also provide a macro for extracting nodeID from memflags so that
> users don't need to remember to decrement the value.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  xen/common/page_alloc.c |    2 +-
>  xen/include/xen/mm.h    |    6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 124fa88..367b5bd 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -581,7 +581,7 @@ static struct page_info *alloc_heap_pages(
>      struct domain *d)
>  {
>      unsigned int first_node, i, j, zone = 0, nodemask_retry = 0;
> -    nodeid_t node = (nodeid_t)((memflags >> _MEMF_node) - 1);
> +    nodeid_t node = MEMF2NODE(memflags);
>      unsigned long request = 1UL << order;
>      struct page_info *pg;
>      nodemask_t nodemask = (d != NULL ) ? d->node_affinity : node_online_map;
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 74a65a6..c86c487 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -110,6 +110,8 @@ struct npfec {
>  };
>  
>  /* memflags: */
> +#define MEMF_node_mask    ((1U << sizeof(nodeid_t)) - 1)

DYM 8 * sizeof()? Also please put this side by side with _MEMF_node.

> @@ -121,10 +123,12 @@ struct npfec {
>  #define _MEMF_exact_node  4
>  #define  MEMF_exact_node  (1U<<_MEMF_exact_node)
>  #define _MEMF_node        8
> -#define  MEMF_node(n)     ((((n)+1)&0xff)<<_MEMF_node)
> +#define  MEMF_node(n)     ((((n)+1) & MEMF_node_mask) << _MEMF_node)
>  #define _MEMF_bits        24
>  #define  MEMF_bits(n)     ((n)<<_MEMF_bits)

At the very least you'll want to put a BUILD_BUG_ON() somewhere
to make sure MEMF_node() doesn't spill into MEMF_bits().

Jan

Re: [PATCH v2 3/3] mm: MEMF_node should handle changes in nodeid_t size

[Jan Beulich]

>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
> @@ -121,10 +123,12 @@ struct npfec {
>  #define _MEMF_exact_node  4
>  #define  MEMF_exact_node  (1U<<_MEMF_exact_node)
>  #define _MEMF_node        8
> -#define  MEMF_node(n)     ((((n)+1)&0xff)<<_MEMF_node)
> +#define  MEMF_node(n)     ((((n)+1) & MEMF_node_mask) << _MEMF_node)
>  #define _MEMF_bits        24
>  #define  MEMF_bits(n)     ((n)<<_MEMF_bits)
>  
> +#define MEMF2NODE(memflags) (MASK_EXTR(memflags, MEMF_node_mask) - 1)

As this is being used just once, I don't really see a need for it to be
exposed globally. Perhaps do away with the macro, but at the very
least confine it to page_alloc.c.

Jan

Re: [PATCH v2 3/3] mm: MEMF_node should handle changes in nodeid_t size

[Boris Ostrovsky]

On 02/25/2015 05:35 AM, Jan Beulich wrote:
>>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
>> @@ -121,10 +123,12 @@ struct npfec {
>>   #define _MEMF_exact_node  4
>>   #define  MEMF_exact_node  (1U<<_MEMF_exact_node)
>>   #define _MEMF_node        8
>> -#define  MEMF_node(n)     ((((n)+1)&0xff)<<_MEMF_node)
>> +#define  MEMF_node(n)     ((((n)+1) & MEMF_node_mask) << _MEMF_node)
>>   #define _MEMF_bits        24
>>   #define  MEMF_bits(n)     ((n)<<_MEMF_bits)
>>   
>> +#define MEMF2NODE(memflags) (MASK_EXTR(memflags, MEMF_node_mask) - 1)
> As this is being used just once, I don't really see a need for it to be
> exposed globally. Perhaps do away with the macro, but at the very
> least confine it to page_alloc.c.

I intentionally put it here: we have macro to encode ('put', so to 
speak) nodeID in memflags in this file (by adding one to it) and so I 
felt that we don't need to expose this encoding outside of this file by 
providing a "get" macro.

-boris

Re: [PATCH v2 3/3] mm: MEMF_node should handle changes in nodeid_t size

[Jan Beulich]

>>> On 25.02.15 at 14:30, <boris.ostrovsky@oracle.com> wrote:
> On 02/25/2015 05:35 AM, Jan Beulich wrote:
>>>>> On 24.02.15 at 20:11, <boris.ostrovsky@oracle.com> wrote:
>>> @@ -121,10 +123,12 @@ struct npfec {
>>>   #define _MEMF_exact_node  4
>>>   #define  MEMF_exact_node  (1U<<_MEMF_exact_node)
>>>   #define _MEMF_node        8
>>> -#define  MEMF_node(n)     ((((n)+1)&0xff)<<_MEMF_node)
>>> +#define  MEMF_node(n)     ((((n)+1) & MEMF_node_mask) << _MEMF_node)
>>>   #define _MEMF_bits        24
>>>   #define  MEMF_bits(n)     ((n)<<_MEMF_bits)
>>>   
>>> +#define MEMF2NODE(memflags) (MASK_EXTR(memflags, MEMF_node_mask) - 1)
>> As this is being used just once, I don't really see a need for it to be
>> exposed globally. Perhaps do away with the macro, but at the very
>> least confine it to page_alloc.c.
> 
> I intentionally put it here: we have macro to encode ('put', so to 
> speak) nodeID in memflags in this file (by adding one to it) and so I 
> felt that we don't need to expose this encoding outside of this file by 
> providing a "get" macro.

Hmm, don't know. The encoding one obviously is needed in various
places, while the decoding one is not supposed to be needed
elsewhere. But if you strongly feel so, I can live with that macro
staying here.

Jan