numa: enable sparse node numbering

Add a present field to NodeInfo which indicates if a given nodeid was present on the command-line or not. Current code relies on a memory value being passed for a node to indicate presence, which is insufficient in the presence of memoryless nodes or sparse numbering. Adjust the iteration of various NUMA loops to use the maximum known NUMA ID rather than the number of NUMA nodes.

numa.c::set_numa_nodes() has become a bit more convoluted for round-robin'ing the CPUs over known nodes when not specified by the user.

With generic sparse NUMA node parsing in place ("numa: enable sparse node numbering"), ppc can be updated to iterate over only the user-specified nodes.

qemu-system-ppc64 -machine pseries,accel=kvm,usb=off -m 4096 -realtime mlock=off -numa node,nodeid=3 -numa node,nodeid=2 -smp 4

Quoted 10 lines · Nishanth Aravamudan

Add a present field to NodeInfo which indicates if a given nodeid was present on the command-line or not. Current code relies on a memory value being passed for a node to indicate presence, which is insufficient in the presence of memoryless nodes or sparse numbering. Adjust the iteration of various NUMA loops to use the maximum known NUMA ID rather than the number of NUMA nodes.

numa.c::set_numa_nodes() has become a bit more convoluted for round-robin'ing the CPUs over known nodes when not specified by the user.

Sorry for the noise, but after staring at it for a bit, I think this version is a bit cleaner for the set_numa_nodes() change.

Add a present field to NodeInfo which indicates if a given nodeid was present on the command-line or not. Current code relies on a memory value being passed for a node to indicate presence, which is insufficient in the presence of memoryless nodes or sparse numbering. Adjust the iteration of various NUMA loops to use the maximum known NUMA ID rather than the number of NUMA nodes.

numa.c::set_numa_nodes() has become a bit more convoluted for round-robin'ing the CPUs over known nodes when not specified by the user.

Note that architecture-specific code still needs changes (forthcoming) for both sparse node numbering and memoryless nodes.

Examples:

(1) qemu-system-x86_64 -enable-kvm -m 4096 -numa node,nodeid=3 -numa node,nodeid=2 -smp 16

Before:

node 0 cpus: 0 2 4 6 8 10 12 14
node 0 size: 2048 MB
node 1 cpus: 1 3 5 7 9 11 13 15
node 1 size: 2048 MB

After:

node 2 cpus: 0 2 4 6 8 10 12 14 |
node 2 size: 2048 MB |
node 3 cpus: 1 3 5 7 9 11 13 15 |
node 3 size: 2048 MB

(2) qemu-system-x86_64 -enable-kvm -m 4096 -numa node,nodeid=0 -numa node,nodeid=1 -numa node,nodeid=2 -numa node,nodeid=3 -smp 16

node 0 cpus: 0 4 8 12 |
node 0 size: 1024 MB |
node 1 cpus: 1 5 9 13 |
node 1 size: 1024 MB |
node 2 cpus: 2 6 10 14 |
node 2 size: 1024 MB |
node 3 cpus: 3 7 11 15 |
node 3 size: 1024 MB

(qemu) info numa
4 nodes
node 0 cpus: 0 4 8 12
node 0 size: 1024 MB
node 1 cpus: 1 5 9 13
node 1 size: 1024 MB
node 2 cpus: 2 6 10 14
node 2 size: 1024 MB
node 3 cpus: 3 7 11 15
node 3 size: 1024 MB

Signed-off-by Nishanth Aravamudan <[email protected]>

Cc Eduardo Habkost <[email protected]>

Cc Alexey Kardashevskiy <[email protected]>

Cc Michael S. Tsirkin <[email protected]>

Cc [email protected]

---
Based off mst's for_upstream tag, which has the NodeInfo changes.

v1 -> v2:
Modify set_numa_nodes loop for round-robin'ing CPUs.

include/sysemu/sysemu.h +3 -1

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 
-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;
 } NodeInfo;
 extern NodeInfo numa_info[MAX_NODES];
 void set_numa_nodes(void);

monitor.c +0 -0

index c7f8797..4721996 100644

monitor.c +0 -0

         monitor_printf(mon, "node %d cpus:", i);
         CPU_FOREACH(cpu) {
             if (cpu->numa_node == i) {

numa.c +18 -11

index e471afe..2a4c577 100644
@@ -106,6 +106,10 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
         numa_info[nodenr].node_mem = object_property_get_int(o, "size", NULL);
         numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
     }
+    numa_info[nodenr].present = true;
+    if (nodenr >= max_numa_node) {
+        max_numa_node = nodenr + 1;
+    }
 }
 
 int numa_init_func(QemuOpts *opts, void *opaque)

@@ -155,7 +159,7 @@ void set_numa_nodes(void)
 {
     if (nb_numa_nodes > 0) {
         uint64_t numa_total;
-        int i;
+        int i, j = -1;
 
         if (nb_numa_nodes > MAX_NODES) {
             nb_numa_nodes = MAX_NODES;

@@ -164,27 +168,29 @@ void set_numa_nodes(void)
         /* If no memory size if given for any node, assume the default case
          * and distribute the available memory equally across all nodes
          */
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (numa_info[i].node_mem != 0) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present && numa_info[i].node_mem != 0) {
                 break;
             }
         }
-        if (i == nb_numa_nodes) {
+        if (i == max_numa_node) {
             uint64_t usedmem = 0;
 
             /* On Linux, the each node's border has to be 8MB aligned,
              * the final node gets the rest.
              */
-            for (i = 0; i < nb_numa_nodes - 1; i++) {
-                numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
-                                        ~((1 << 23UL) - 1);
-                usedmem += numa_info[i].node_mem;
+            for (i = 0; i < max_numa_node - 1; i++) {
+                if (numa_info[i].present) {
+                    numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
+                                            ~((1 << 23UL) - 1);
+                    usedmem += numa_info[i].node_mem;
+                }
             }
             numa_info[i].node_mem = ram_size - usedmem;
         }
 
         numa_total = 0;
-        for (i = 0; i < nb_numa_nodes; i++) {
+        for (i = 0; i < max_numa_node; i++) {
             numa_total += numa_info[i].node_mem;
         }
         if (numa_total != ram_size) {

@@ -194,8 +200,9 @@ void set_numa_nodes(void)
             exit(1);
         }
 
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present &&
+                !bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
                 break;
             }
         }

@@ -203,9 +210,15 @@ void set_numa_nodes(void)
          * must cope with this anyway, because there are BIOSes out there in
          * real machines which also use this scheme.

numa.c +3 -1

+                } while (!numa_info[j].present);
+                set_bit(i, numa_info[j].node_cpu);
             }
         }
     }

@@ -217,7 +230,7 @@ void set_numa_modes(void)
     int i;
 
     CPU_FOREACH(cpu) {
-        for (i = 0; i < nb_numa_nodes; i++) {
+        for (i = 0; i < max_numa_node; i++) {
             if (test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
                 cpu->numa_node = i;
             }

vl.c +3 -0

index 54b4627..e1a6ab8 100644
@@ -195,6 +195,7 @@ static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
     QTAILQ_HEAD_INITIALIZER(fw_boot_order);
 
 int nb_numa_nodes;
+int max_numa_node;
 NodeInfo numa_info[MAX_NODES];
 
 uint8_t qemu_uuid[16];

@@ -2967,10 +2968,12 @@ int main(int argc, char **argv, char **envp)
 
     for (i = 0; i < MAX_NODES; i++) {
         numa_info[i].node_mem = 0;
+        numa_info[i].present = false;
         bitmap_zero(numa_info[i].node_cpu, MAX_CPUMASK_BITS);
     }
 
     nb_numa_nodes = 0;
+    max_numa_node = -1;
     nb_nics = 0;
 
     bdrv_init_with_whitelist();

Sparse node numbering occurs on powerpc in practice under PowerVM. In order to emulate the same NUMA topology under qemu, the assumption that NUMA nodes are linearly ordered has to be removed. qemu actually already supports (inasmuch as it doesn't throw an error) sparse node numbering by the end-user, but the information is effectively ignored and the nodes are populated linearly starting at 0. This means a user's node ID requests are not honored in the Linux kernel, and that the topology may not be as requested (in terms of the CPU/memory placement).

Add a present field to NodeInfo which indicates if a given nodeid was present on the command-line or not. Current code relies on a memory value being passed for a node to indicate presence, which is insufficient in the presence of memoryless nodes or sparse numbering. Adjust the iteration of various NUMA loops to use the maximum known NUMA ID rather than the number of NUMA nodes.

numa.c::set_numa_nodes() has become a bit more convoluted for round-robin'ing the CPUs over known nodes when not specified by the user.

Note that architecture-specific code still possibly needs changes (forthcoming) for both sparse node numbering and memoryless nodes. This only puts in the generic infrastructure to support that.

Examples:

(1) qemu-system-x86_64 -enable-kvm -m 4096 -numa node,nodeid=3 -numa node,nodeid=2 -smp 16

Before:

node 0 cpus: 0 2 4 6 8 10 12 14
node 0 size: 2048 MB
node 1 cpus: 1 3 5 7 9 11 13 15
node 1 size: 2048 MB

After:

node 2 cpus: 0 2 4 6 8 10 12 14 |
node 2 size: 2048 MB |
node 3 cpus: 1 3 5 7 9 11 13 15 |
node 3 size: 2048 MB

(2) qemu-system-x86_64 -enable-kvm -m 4096 -numa node,nodeid=0 -numa node,nodeid=1 -numa node,nodeid=2 -numa node,nodeid=3 -smp 16

node 0 cpus: 0 4 8 12 |
node 0 size: 1024 MB |
node 1 cpus: 1 5 9 13 |
node 1 size: 1024 MB |
node 2 cpus: 2 6 10 14 |
node 2 size: 1024 MB |
node 3 cpus: 3 7 11 15 |
node 3 size: 1024 MB

(qemu) info numa
4 nodes
node 0 cpus: 0 4 8 12
node 0 size: 1024 MB
node 1 cpus: 1 5 9 13
node 1 size: 1024 MB
node 2 cpus: 2 6 10 14
node 2 size: 1024 MB
node 3 cpus: 3 7 11 15
node 3 size: 1024 MB

Signed-off-by Nishanth Aravamudan <[email protected]>

Tested-by Hu Tao <[email protected]>

---
Based off mst's for_upstream tag, which has the NodeInfo changes.

v1 -> v2:
Modify set_numa_nodes loop for round-robin'ing CPUs.

v2 -> v3:
Updated changelog to indicate problem being solved. Updated memory_region_allocate_system_memory based upon feedback from

    Hu.

Updated set_numa_nodes loop again to be simpler based upon feedback
from Hu.
Fixed bug with a mix of nodes with nodeid specified and without, where
the same nodeid would be used by the explicit specification and the auto-numbering code.

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */

include/sysemu/sysemu.h +0 -0

 } NodeInfo;
 extern NodeInfo numa_info[MAX_NODES];
 void set_numa_nodes(void);

monitor.c +4 -1

index c7f8797..4721996 100644
@@ -2003,7 +2003,10 @@ static void do_info_numa(Monitor *mon, const QDict *qdict)
     CPUState *cpu;
 
     monitor_printf(mon, "%d nodes\n", nb_numa_nodes);
-    for (i = 0; i < nb_numa_nodes; i++) {
+    for (i = 0; i < max_numa_node; i++) {
+        if (!numa_info[i].present) {
+            continue;
+        }
         monitor_printf(mon, "node %d cpus:", i);
         CPU_FOREACH(cpu) {
             if (cpu->numa_node == i) {

numa.c +34 -17

index e471afe..d6b86ab 100644
@@ -53,7 +53,10 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
     if (node->has_nodeid) {
         nodenr = node->nodeid;
     } else {
-        nodenr = nb_numa_nodes;
+        nodenr = 0;
+        while (numa_info[nodenr].present) {
+            nodenr++;
+        }
     }
 
     if (nodenr >= MAX_NODES) {

@@ -106,6 +109,10 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
         numa_info[nodenr].node_mem = object_property_get_int(o, "size", NULL);
         numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
     }
+    numa_info[nodenr].present = true;
+    if (nodenr >= max_numa_node) {
+        max_numa_node = nodenr + 1;
+    }
 }
 
 int numa_init_func(QemuOpts *opts, void *opaque)

@@ -155,7 +162,7 @@ void set_numa_nodes(void)
 {
     if (nb_numa_nodes > 0) {
         uint64_t numa_total;
-        int i;
+        int i, j = -1;
 
         if (nb_numa_nodes > MAX_NODES) {
             nb_numa_nodes = MAX_NODES;

@@ -164,27 +171,29 @@ void set_numa_nodes(void)
         /* If no memory size if given for any node, assume the default case
          * and distribute the available memory equally across all nodes
          */
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (numa_info[i].node_mem != 0) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present && numa_info[i].node_mem != 0) {
                 break;
             }
         }
-        if (i == nb_numa_nodes) {
+        if (i == max_numa_node) {
             uint64_t usedmem = 0;
 
             /* On Linux, the each node's border has to be 8MB aligned,
              * the final node gets the rest.
              */
-            for (i = 0; i < nb_numa_nodes - 1; i++) {
-                numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
-                                        ~((1 << 23UL) - 1);
-                usedmem += numa_info[i].node_mem;
+            for (i = 0; i < max_numa_node - 1; i++) {
+                if (numa_info[i].present) {
+                    numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
+                                            ~((1 << 23UL) - 1);
+                    usedmem += numa_info[i].node_mem;
+                }
             }
             numa_info[i].node_mem = ram_size - usedmem;
         }
 
         numa_total = 0;
-        for (i = 0; i < nb_numa_nodes; i++) {
+        for (i = 0; i < max_numa_node; i++) {
             numa_total += numa_info[i].node_mem;
         }
         if (numa_total != ram_size) {

@@ -194,8 +203,9 @@ void set_numa_nodes(void)
             exit(1);
         }
 
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present &&
+                !bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
                 break;
             }
         }

@@ -203,9 +213,12 @@ void set_numa_nodes(void)
          * must cope with this anyway, because there are BIOSes out there in
          * real machines which also use this scheme.
          */
-        if (i == nb_numa_nodes) {
+        if (i == max_numa_node) {
             for (i = 0; i < max_cpus; i++) {
-                set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
+                do {
+                    j = (j + 1) % max_numa_node;
+                } while (!numa_info[j].present);
+                set_bit(i, numa_info[j].node_cpu);
             }
         }
     }

@@ -217,8 +230,9 @@ void set_numa_modes(void)
     int i;
 
     CPU_FOREACH(cpu) {
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present &&
+                test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
                 cpu->numa_node = i;
             }
         }

@@ -266,10 +280,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
     }
 
     memory_region_init(mr, owner, name, ram_size);
-    for (i = 0; i < MAX_NODES; i++) {
+    for (i = 0; i < max_numa_node; i++) {
         Error *local_err = NULL;
         uint64_t size = numa_info[i].node_mem;
         HostMemoryBackend *backend = numa_info[i].node_memdev;
+        if (!numa_info[i].present) {
+            continue;
+        }
         if (!backend) {
             continue;
         }

vl.c +3 -0

index 54b4627..e1a6ab8 100644
@@ -195,6 +195,7 @@ static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
     QTAILQ_HEAD_INITIALIZER(fw_boot_order);
 
 int nb_numa_nodes;
+int max_numa_node;
 NodeInfo numa_info[MAX_NODES];
 
 uint8_t qemu_uuid[16];

@@ -2967,10 +2968,12 @@ int main(int argc, char **argv, char **envp)
 
     for (i = 0; i < MAX_NODES; i++) {
         numa_info[i].node_mem = 0;
+        numa_info[i].present = false;
         bitmap_zero(numa_info[i].node_cpu, MAX_CPUMASK_BITS);
     }
 
     nb_numa_nodes = 0;
+    max_numa_node = -1;
     nb_nics = 0;
 
     bdrv_init_with_whitelist();

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Quoted 2 lines · Igor Mammedov

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

<snip>

Quoted 8 lines

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

Quoted 10 lines

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

If that would be preferred, I can move to that. I assume a simple linked-list is fine. Does qemu provide any infrastructure for defining lists? I'll look through the source but any pointers would be helpful.

Generally speaking, sparse NUMA nodes aren't that common and when they exist, the gaps aren't large. But it does seem to make sense if we have sparse IDs at all, we might as well move to a list.

In any case, moving to the list means we'd have a nodeid as part of the structure instead.

Quoted 2 lines

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Sounds like a good idea to me. I'll respin the patch soon.

Quoted 37 lines · Nishanth Aravamudan

Quoted 2 lines · Igor Mammedov

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

<snip>

Quoted 8 lines

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

Quoted 10 lines

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

If that would be preferred, I can move to that. I assume a simple linked-list is fine. Does qemu provide any infrastructure for defining lists? I'll look through the source but any pointers would be helpful.

Generally speaking, sparse NUMA nodes aren't that common and when they exist, the gaps aren't large. But it does seem to make sense if we have sparse IDs at all, we might as well move to a list.

In any case, moving to the list means we'd have a nodeid as part of the structure instead.

Quoted 2 lines

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Quoted 1 line

Sounds like a good idea to me. I'll respin the patch soon.

Having a list makes sense, the only difference is that keeping a sparse array sorted is much easier than making a sorted list (because the ACPI tables are nodeid-ordered). That's why I suggested keeping the array initially.

Adding a "present" field to the array is a trivial and easy-to-review change. Changing NodeInfo to use linked lists is a more complex change that I wouldn't want to include after soft freeze.

In other words:
* Having a list is better than a sparse array; but:
* Having a small sparse array with the "present" field is better than broken sparse nodeid support (IMO).

Quoted 38 lines · Eduardo Habkost

Quoted 37 lines · Nishanth Aravamudan

Quoted 2 lines · Igor Mammedov

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

<snip>

Quoted 8 lines

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

Quoted 10 lines

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

If that would be preferred, I can move to that. I assume a simple linked-list is fine. Does qemu provide any infrastructure for defining lists? I'll look through the source but any pointers would be helpful.

Generally speaking, sparse NUMA nodes aren't that common and when they exist, the gaps aren't large. But it does seem to make sense if we have sparse IDs at all, we might as well move to a list.

In any case, moving to the list means we'd have a nodeid as part of the structure instead.

Quoted 2 lines

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Quoted 6 lines

Quoted 1 line

Sounds like a good idea to me. I'll respin the patch soon.

Having a list makes sense, the only difference is that keeping a sparse array sorted is much easier than making a sorted list (because the ACPI tables are nodeid-ordered). That's why I suggested keeping the array initially.

And for non-ACPI platforms, it does feel like keeping the list sorted is ideal, as it simplifies various loops, etc.

Quoted 8 lines

Adding a "present" field to the array is a trivial and easy-to-review change. Changing NodeInfo to use linked lists is a more complex change that I wouldn't want to include after soft freeze.

In other words:
* Having a list is better than a sparse array; but:
* Having a small sparse array with the "present" field is better than broken sparse nodeid support (IMO).

Perhaps as a compromise I can work on the list conversion as a follow-on patch?

Thanks,
Nish

Quoted 38 lines · Eduardo Habkost

Quoted 37 lines · Nishanth Aravamudan

Quoted 2 lines · Igor Mammedov

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

<snip>

Quoted 8 lines

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

Quoted 10 lines

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

If that would be preferred, I can move to that. I assume a simple linked-list is fine. Does qemu provide any infrastructure for defining lists? I'll look through the source but any pointers would be helpful.

Generally speaking, sparse NUMA nodes aren't that common and when they exist, the gaps aren't large. But it does seem to make sense if we have sparse IDs at all, we might as well move to a list.

In any case, moving to the list means we'd have a nodeid as part of the structure instead.

Quoted 2 lines

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Quoted 6 lines

Quoted 1 line

Sounds like a good idea to me. I'll respin the patch soon.

Having a list makes sense, the only difference is that keeping a sparse array sorted is much easier than making a sorted list (because the ACPI tables are nodeid-ordered). That's why I suggested keeping the array initially.

And for non-ACPI platforms, it does feel like keeping the list sorted is ideal, as it simplifies various loops, etc.

Quoted 4 lines

Adding a "present" field to the array is a trivial and easy-to-review change. Changing NodeInfo to use linked lists is a more complex change that I wouldn't want to include after soft freeze.

Quoted 4 lines

In other words:
* Having a list is better than a sparse array; but:
* Having a small sparse array with the "present" field is better than broken sparse nodeid support (IMO).

Perhaps as a compromise I can work on the list conversion as a follow-on patch?

Thanks,
Nish

Quoted 31 lines · Nishanth Aravamudan

Quoted 2 lines · Igor Mammedov

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

<snip>

Quoted 8 lines

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

Quoted 10 lines

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

If that would be preferred, I can move to that. I assume a simple linked-list is fine. Does qemu provide any infrastructure for defining lists? I'll look through the source but any pointers would be helpful.

Generally speaking, sparse NUMA nodes aren't that common and when they exist, the gaps aren't large. But it does seem to make sense if we have sparse IDs at all, we might as well move to a list.

Quoted 7 lines

In any case, moving to the list means we'd have a nodeid as part of the structure instead.

Quoted 2 lines

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Sounds like a good idea to me. I'll respin the patch soon.

Having a list makes sense, the only difference is that keeping a sparse array sorted is much easier than making a sorted list (because the ACPI tables are nodeid-ordered). That's why I suggested keeping the array initially.

Adding a "present" field to the array is a trivial and easy-to-review change. Changing NodeInfo to use linked lists is a more complex change that I wouldn't want to include after soft freeze.

In other words:
* Having a list is better than a sparse array; but:
* Having a small sparse array with the "present" field is better than broken sparse nodeid support (IMO).

<rant>
Improving the code is always welcome (and adding a list would be an improvement), but we don't need to implement all refactoring changes we can dream of, before including a new feature. I like to evaluate how the code looks like compared to what we have today, not with what we want it to look like in one year.

Could we (the QEMU community) please learn to sometimes accept simple features or fixes (which don't make the code worse) without requiring the whole code to be rewritten first?
</rant>

Signature · 1 line

Eduardo

Quoted 49 lines · Eduardo Habkost

Quoted 31 lines · Nishanth Aravamudan

Quoted 2 lines · Igor Mammedov

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

<snip>

Quoted 8 lines

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

Quoted 10 lines

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

If that would be preferred, I can move to that. I assume a simple linked-list is fine. Does qemu provide any infrastructure for defining lists? I'll look through the source but any pointers would be helpful.

Generally speaking, sparse NUMA nodes aren't that common and when they exist, the gaps aren't large. But it does seem to make sense if we have sparse IDs at all, we might as well move to a list.

Quoted 7 lines

In any case, moving to the list means we'd have a nodeid as part of the structure instead.

Quoted 2 lines

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Sounds like a good idea to me. I'll respin the patch soon.

Having a list makes sense, the only difference is that keeping a sparse array sorted is much easier than making a sorted list (because the ACPI tables are nodeid-ordered). That's why I suggested keeping the array initially.

Adding a "present" field to the array is a trivial and easy-to-review change. Changing NodeInfo to use linked lists is a more complex change that I wouldn't want to include after soft freeze.

Quoted 4 lines

In other words:
* Having a list is better than a sparse array; but:
* Having a small sparse array with the "present" field is better than broken sparse nodeid support (IMO).

I agree here. This patchset is still RFC but if that's the only issue I might apply it.

Quoted 55 lines · Michael S. Tsirkin

Quoted 53 lines · Eduardo Habkost

Quoted 31 lines · Nishanth Aravamudan

Quoted 2 lines · Igor Mammedov

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

<snip>

Quoted 8 lines

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

Quoted 10 lines

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

If that would be preferred, I can move to that. I assume a simple linked-list is fine. Does qemu provide any infrastructure for defining lists? I'll look through the source but any pointers would be helpful.

Generally speaking, sparse NUMA nodes aren't that common and when they exist, the gaps aren't large. But it does seem to make sense if we have sparse IDs at all, we might as well move to a list.

Quoted 7 lines

In any case, moving to the list means we'd have a nodeid as part of the structure instead.

Quoted 2 lines

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Sounds like a good idea to me. I'll respin the patch soon.

Having a list makes sense, the only difference is that keeping a sparse array sorted is much easier than making a sorted list (because the ACPI tables are nodeid-ordered). That's why I suggested keeping the array initially.

Adding a "present" field to the array is a trivial and easy-to-review change. Changing NodeInfo to use linked lists is a more complex change that I wouldn't want to include after soft freeze.

In other words:
* Having a list is better than a sparse array; but:
* Having a small sparse array with the "present" field is better than broken sparse nodeid support (IMO).

Quoted 2 lines

I agree here. This patchset is still RFC but if that's the only issue I might apply it.

I don't think it is ready for 2.1 since it lacks arch-specific changes. The patch itself only makes qemu be able to bring up guest with sparse numa nodes, without arch-specific changes, guest sees only one node(at least on x86).

Quoted 56 lines · Hu Tao

Quoted 55 lines · Michael S. Tsirkin

Quoted 53 lines · Eduardo Habkost

Quoted 31 lines · Nishanth Aravamudan

Quoted 2 lines · Igor Mammedov

On Tue, 24 Jun 2014 10:40:38 -0700
Nishanth Aravamudan <[email protected]> wrote:

<snip>

Quoted 8 lines

Quoted 8 lines

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 

Quoted 10 lines

include/sysemu/sysemu.h extern int mem_prealloc; · line 154

Quoted 8 lines

include/sysemu/sysemu.h +3 -1

-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;

How about dropping 'present' and replacing array with a list of only present nodes?

If that would be preferred, I can move to that. I assume a simple linked-list is fine. Does qemu provide any infrastructure for defining lists? I'll look through the source but any pointers would be helpful.

Generally speaking, sparse NUMA nodes aren't that common and when they exist, the gaps aren't large. But it does seem to make sense if we have sparse IDs at all, we might as well move to a list.

Quoted 7 lines

In any case, moving to the list means we'd have a nodeid as part of the structure instead.

Quoted 2 lines

That way it will be one more step closer to converting numa infrastructure to a set of QOM objects.

Sounds like a good idea to me. I'll respin the patch soon.

Having a list makes sense, the only difference is that keeping a sparse array sorted is much easier than making a sorted list (because the ACPI tables are nodeid-ordered). That's why I suggested keeping the array initially.

Adding a "present" field to the array is a trivial and easy-to-review change. Changing NodeInfo to use linked lists is a more complex change that I wouldn't want to include after soft freeze.

In other words:
* Having a list is better than a sparse array; but:
* Having a small sparse array with the "present" field is better than broken sparse nodeid support (IMO).

Quoted 7 lines

Quoted 2 lines

I agree here. This patchset is still RFC but if that's the only issue I might apply it.

I don't think it is ready for 2.1 since it lacks arch-specific changes. The patch itself only makes qemu be able to bring up guest with sparse numa nodes, without arch-specific changes, guest sees only one node(at least on x86).

Well, it does no harm or foul to have it, on it's own. In that, nothing is worse or better than it was. Although I would argue the qemu core is better than it was :) I have sent the powerpc enabling patch, but even with that, we need Alexey's series of 6 patches (which do fix a regression) to fully support the sparse numbering. Maybe that means I've convinced myself it's best to wait until after 2.1.

I honestly am not sure how to "fix" the x86 code to support the sparse numbering -- the APIC code seems to rely on it being continuous. From some googling, it seems like the most common reason for non-continuous numbering on x86 is faulty hardware. I will keep fiddling if no one else steps up, but I'm not an x86 NUMA expert :)

Sparse node numbering occurs on powerpc in practice under PowerVM. In order to emulate the same NUMA topology under qemu, the assumption that NUMA nodes are linearly ordered has to be removed. qemu actually already supports (inasmuch as it doesn't throw an error) sparse node numbering by the end-user, but the information is effectively ignored and the nodes are populated linearly starting at 0. This means a user's node ID requests are not honored in the Linux kernel, and that the topology may not be as requested (in terms of the CPU/memory placement).

Add a present field to NodeInfo which indicates if a given nodeid was present on the command-line or not. Current code relies on a memory value being passed for a node to indicate presence, which is insufficient in the presence of memoryless nodes or sparse numbering. Adjust the iteration of various NUMA loops to use the maximum known NUMA ID rather than the number of NUMA nodes.

numa.c::set_numa_nodes() has become a bit more convoluted for round-robin'ing the CPUs over known nodes when not specified by the user.

Note that architecture-specific code still possibly needs changes (forthcoming) for both sparse node numbering and memoryless nodes. This only puts in the generic infrastructure to support that.

Examples:

(1) qemu-system-x86_64 -enable-kvm -m 4096 -numa node,nodeid=3 -numa node,nodeid=2 -smp 16

Before:

node 0 cpus: 0 2 4 6 8 10 12 14
node 0 size: 2048 MB
node 1 cpus: 1 3 5 7 9 11 13 15
node 1 size: 2048 MB

After:

node 2 cpus: 0 2 4 6 8 10 12 14 |
node 2 size: 2048 MB |
node 3 cpus: 1 3 5 7 9 11 13 15 |
node 3 size: 2048 MB

(2) qemu-system-x86_64 -enable-kvm -m 4096 -numa node,nodeid=0 -numa node,nodeid=1 -numa node,nodeid=2 -numa node,nodeid=3 -smp 16

node 0 cpus: 0 4 8 12 |
node 0 size: 1024 MB |
node 1 cpus: 1 5 9 13 |
node 1 size: 1024 MB |
node 2 cpus: 2 6 10 14 |
node 2 size: 1024 MB |
node 3 cpus: 3 7 11 15 |
node 3 size: 1024 MB

(qemu) info numa
4 nodes
node 0 cpus: 0 4 8 12
node 0 size: 1024 MB
node 1 cpus: 1 5 9 13
node 1 size: 1024 MB
node 2 cpus: 2 6 10 14
node 2 size: 1024 MB
node 3 cpus: 3 7 11 15
node 3 size: 1024 MB

Signed-off-by Nishanth Aravamudan <[email protected]>

Tested-by Hu Tao <[email protected]>

---
Based off mst's for_upstream tag, which has the NodeInfo changes.

v1 -> v2:
Modify set_numa_nodes loop for round-robin'ing CPUs.

v2 -> v3:
Updated changelog to indicate problem being solved. Updated memory_region_allocate_system_memory based upon feedback from

    Hu.

Updated set_numa_nodes loop again to be simpler based upon feedback
from Hu.
Fixed bug with a mix of nodes with nodeid specified and without, where
the same nodeid would be used by the explicit specification and the auto-numbering code.

include/sysemu/sysemu.h +0 -0

index 277230d..b90bf66 100644

include/sysemu/sysemu.h +1 -0

 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;
 } NodeInfo;
 extern NodeInfo numa_info[MAX_NODES];
 void set_numa_nodes(void);

monitor.c +4 -1

index c7f8797..4721996 100644
@@ -2003,7 +2003,10 @@ static void do_info_numa(Monitor *mon, const QDict *qdict)
     CPUState *cpu;
 
     monitor_printf(mon, "%d nodes\n", nb_numa_nodes);
-    for (i = 0; i < nb_numa_nodes; i++) {
+    for (i = 0; i < max_numa_node; i++) {
+        if (!numa_info[i].present) {
+            continue;
+        }
         monitor_printf(mon, "node %d cpus:", i);
         CPU_FOREACH(cpu) {
             if (cpu->numa_node == i) {

numa.c +4 -1

index e471afe..d6b86ab 100644
@@ -53,7 +53,10 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
     if (node->has_nodeid) {
         nodenr = node->nodeid;
     } else {
-        nodenr = nb_numa_nodes;
+        nodenr = 0;
+        while (numa_info[nodenr].present) {
+            nodenr++;
+        }
     }
 
     if (nodenr >= MAX_NODES) {

numa.c +3 -4

         if (nb_numa_nodes > MAX_NODES) {
             nb_numa_nodes = MAX_NODES;

@@ -164,27 +171,29 @@ void set_numa_nodes(void)
         /* If no memory size if given for any node, assume the default case
          * and distribute the available memory equally across all nodes
          */
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (numa_info[i].node_mem != 0) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present && numa_info[i].node_mem != 0) {
                 break;
             }
         }
-        if (i == nb_numa_nodes) {
+        if (i == max_numa_node) {
             uint64_t usedmem = 0;
 
             /* On Linux, the each node's border has to be 8MB aligned,
              * the final node gets the rest.
              */
-            for (i = 0; i < nb_numa_nodes - 1; i++) {

numa.c +4 -3

         }
 
         numa_total = 0;
-        for (i = 0; i < nb_numa_nodes; i++) {
+        for (i = 0; i < max_numa_node; i++) {
             numa_total += numa_info[i].node_mem;
         }
         if (numa_total != ram_size) {

@@ -194,8 +203,9 @@ void set_numa_nodes(void)
             exit(1);
         }
 
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present &&
+                !bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
                 break;
             }
         }

numa.c +8 -3

+                set_bit(i, numa_info[j].node_cpu);
             }
         }
     }

@@ -217,8 +230,9 @@ void set_numa_modes(void)
     int i;
 
     CPU_FOREACH(cpu) {
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present &&
+                test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
                 cpu->numa_node = i;
             }
         }

@@ -266,10 +280,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
     }
 
     memory_region_init(mr, owner, name, ram_size);
-    for (i = 0; i < MAX_NODES; i++) {
+    for (i = 0; i < max_numa_node; i++) {
         Error *local_err = NULL;
         uint64_t size = numa_info[i].node_mem;
         HostMemoryBackend *backend = numa_info[i].node_memdev;
+        if (!numa_info[i].present) {
+            continue;
+        }
         if (!backend) {
             continue;
         }

vl.c +1 -0

index 54b4627..e1a6ab8 100644
@@ -195,6 +195,7 @@ static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
     QTAILQ_HEAD_INITIALIZER(fw_boot_order);
 
 int nb_numa_nodes;
+int max_numa_node;
 NodeInfo numa_info[MAX_NODES];
 
 uint8_t qemu_uuid[16];

On Tue, Jun 24, 2014 at 10:40:38AM -0700, Nishanth Aravamudan wrote:

include/sysemu/sysemu.h extern int mem_prealloc; · line 148

Quoted 10 lines

include/sysemu/sysemu.h +1 -1

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 
-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */

I would rename it to num_numa_nodes, so we can easily ensure all code using nb_numa_nodes will be converted appropriately.

include/sysemu/sysemu.h extern int mem_prealloc; · line 149

Quoted 1 line

include/sysemu/sysemu.h +1 -0

+extern int max_numa_node; /* Highest specified NUMA node ID */

I would rename it max_numa_nodeid, to make it clear it is the maximum ID, not the maximum number of nodes.

include/sysemu/sysemu.h void set_numa_nodes(void) · line 165

Quoted 7 lines

include/sysemu/sysemu.h +1 -1

 int numa_init_func(QemuOpts *opts, void *opaque)

@@ -155,7 +162,7 @@ void set_numa_nodes(void)
 {
     if (nb_numa_nodes > 0) {
         uint64_t numa_total;
-        int i;
+        int i, j = -1;

Can you please initialize j closer to the loop where it is used?

Quoted 4 lines

include/sysemu/sysemu.h +0 -1

             /* On Linux, the each node's border has to be 8MB aligned,
              * the final node gets the rest.
              */
-            for (i = 0; i < nb_numa_nodes - 1; i++) {

include/sysemu/sysemu.h void set_numa_nodes(void)

Quoted 10 lines

include/sysemu/sysemu.h +6 -3

-                numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
-                                        ~((1 << 23UL) - 1);
-                usedmem += numa_info[i].node_mem;
+            for (i = 0; i < max_numa_node - 1; i++) {
+                if (numa_info[i].present) {
+                    numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
+                                            ~((1 << 23UL) - 1);
+                    usedmem += numa_info[i].node_mem;
+                }
             }

This part is tricky: the following line works only because numa_info[max_numa_node-1] is always present:

Quoted 1 line

             numa_info[i].node_mem = ram_size - usedmem;

So, what about adding assert(numa_info[i].present) here?

Quoted 6 lines

include/sysemu/sysemu.h +1 -1

@@ -203,9 +213,12 @@ void set_numa_nodes(void)
          * must cope with this anyway, because there are BIOSes out there in
          * real machines which also use this scheme.
          */
-        if (i == nb_numa_nodes) {
+        if (i == max_numa_node) {

include/sysemu/sysemu.h void set_numa_nodes(void) · line 220

Quoted 5 lines

include/sysemu/sysemu.h +3 -1

             for (i = 0; i < max_cpus; i++) {
-                set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
+                do {
+                    j = (j + 1) % max_numa_node;
+                } while (!numa_info[j].present);

If you change it from "do { } while" to "while { }", you don't need to initialize j to -1.

Quoted 10 lines

numa.c void set_numa_nodes(void) · line 220

Quoted 7 lines

numa.c +4 -2

-        if (i == nb_numa_nodes) {
+        if (i == max_numa_node) {
             for (i = 0; i < max_cpus; i++) {
-                set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
+                do {
+                    j = (j + 1) % max_numa_node;
+                } while (!numa_info[j].present);

If you change it from "do { } while" to "while { }", you don't need to initialize j to -1.

I don't think that's quite as simple as you make it out to be. If you use a while() loop, we won't always increment j, which means once we've found a present node, we'll always use that node? j here basically represents the *last* used nodeid, which we don't want to use again when we re-enter the for-loop, we want to use the next present nodeid. It seems like the do {} while() does this fine? I could use a while() if I added another increment outside the loop, as follows:

        if (i == max_numa_nodeid) {
            for (i = 0, j = 0; i < max_cpus; i++) {
                while (!numa_info[j].present) {
                    j = (j + 1) % (max_numa_nodeid);
                }
                set_bit(i, numa_info[j].node_cpu);
                j = (j + 1) % (max_numa_nodeid);
            }
        }

If you think that is cleaner, I'll use that version.

Add a present field to NodeInfo which indicates if a given nodeid was present on the command-line or not. Current code relies on a memory value being passed for a node to indicate presence, which is insufficient in the presence of memoryless nodes or sparse numbering. Adjust the iteration of various NUMA loops to use the maximum known NUMA ID rather than the number of NUMA nodes.

numa.c::set_numa_nodes() has become a bit more convoluted for round-robin'ing the CPUs over known nodes when not specified by the user.

Note that architecture-specific code still needs changes (forthcoming) for both sparse node numbering and memoryless nodes.

Examples:

(1) qemu-system-x86_64 -enable-kvm -m 4096 -numa node,nodeid=3 -numa node,nodeid=2 -smp 16

Before:

node 0 cpus: 0 2 4 6 8 10 12 14
node 0 size: 2048 MB
node 1 cpus: 1 3 5 7 9 11 13 15
node 1 size: 2048 MB

After:

node 2 cpus: 0 2 4 6 8 10 12 14 |
node 2 size: 2048 MB |
node 3 cpus: 1 3 5 7 9 11 13 15 |
node 3 size: 2048 MB

(2) qemu-system-x86_64 -enable-kvm -m 4096 -numa node,nodeid=0 -numa node,nodeid=1 -numa node,nodeid=2 -numa node,nodeid=3 -smp 16

Before:

node 0 cpus: 0 4 8 12 |
node 0 size: 1024 MB |
node 1 cpus: 1 5 9 13 |
node 1 size: 1024 MB |
node 2 cpus: 2 6 10 14 |
node 2 size: 1024 MB |
node 3 cpus: 3 7 11 15 |
node 3 size: 1024 MB

After:

node 0 cpus: 0 4 8 12
node 0 size: 1024 MB
node 1 cpus: 1 5 9 13
node 1 size: 1024 MB
node 2 cpus: 2 6 10 14
node 2 size: 1024 MB
node 3 cpus: 3 7 11 15
node 3 size: 1024 MB

Signed-off-by Nishanth Aravamudan <[email protected]>

---
Based off mst's for_upstream tag, which has the NodeInfo changes.

include/sysemu/sysemu.h +3 -1

index 277230d..b90bf66 100644
@@ -145,11 +145,13 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 
-extern int nb_numa_nodes;
+extern int nb_numa_nodes; /* Number of NUMA nodes */
+extern int max_numa_node; /* Highest specified NUMA node ID */
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;
 } NodeInfo;
 extern NodeInfo numa_info[MAX_NODES];
 void set_numa_nodes(void);

monitor.c +4 -1

index c7f8797..4721996 100644
@@ -2003,7 +2003,10 @@ static void do_info_numa(Monitor *mon, const QDict *qdict)
     CPUState *cpu;
 
     monitor_printf(mon, "%d nodes\n", nb_numa_nodes);
-    for (i = 0; i < nb_numa_nodes; i++) {
+    for (i = 0; i < max_numa_node; i++) {
+        if (!numa_info[i].present) {
+            continue;
+        }
         monitor_printf(mon, "node %d cpus:", i);
         CPU_FOREACH(cpu) {
             if (cpu->numa_node == i) {

numa.c +31 -14

index e471afe..c31857d 100644
@@ -106,6 +106,10 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
         numa_info[nodenr].node_mem = object_property_get_int(o, "size", NULL);
         numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
     }
+    numa_info[nodenr].present = true;
+    if (nodenr >= max_numa_node) {
+        max_numa_node = nodenr + 1;
+    }
 }
 
 int numa_init_func(QemuOpts *opts, void *opaque)

@@ -155,7 +159,7 @@ void set_numa_nodes(void)
 {
     if (nb_numa_nodes > 0) {
         uint64_t numa_total;
-        int i;
+        int i, j, last_j = -1;
 
         if (nb_numa_nodes > MAX_NODES) {
             nb_numa_nodes = MAX_NODES;

@@ -164,27 +168,29 @@ void set_numa_nodes(void)
         /* If no memory size if given for any node, assume the default case
          * and distribute the available memory equally across all nodes
          */
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (numa_info[i].node_mem != 0) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present && numa_info[i].node_mem != 0) {
                 break;
             }
         }
-        if (i == nb_numa_nodes) {
+        if (i == max_numa_node) {
             uint64_t usedmem = 0;
 
             /* On Linux, the each node's border has to be 8MB aligned,
              * the final node gets the rest.
              */
-            for (i = 0; i < nb_numa_nodes - 1; i++) {
-                numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
-                                        ~((1 << 23UL) - 1);
-                usedmem += numa_info[i].node_mem;
+            for (i = 0; i < max_numa_node - 1; i++) {
+                if (numa_info[i].present) {
+                    numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
+                                            ~((1 << 23UL) - 1);
+                    usedmem += numa_info[i].node_mem;
+                }
             }
             numa_info[i].node_mem = ram_size - usedmem;
         }
 
         numa_total = 0;
-        for (i = 0; i < nb_numa_nodes; i++) {
+        for (i = 0; i < max_numa_node; i++) {
             numa_total += numa_info[i].node_mem;
         }
         if (numa_total != ram_size) {

@@ -194,8 +200,9 @@ void set_numa_nodes(void)
             exit(1);
         }
 
-        for (i = 0; i < nb_numa_nodes; i++) {
-            if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
+        for (i = 0; i < max_numa_node; i++) {
+            if (numa_info[i].present &&
+                !bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
                 break;
             }
         }

@@ -203,9 +210,19 @@ void set_numa_nodes(void)
          * must cope with this anyway, because there are BIOSes out there in
          * real machines which also use this scheme.
          */
-        if (i == nb_numa_nodes) {
+        if (i == max_numa_node) {
             for (i = 0; i < max_cpus; i++) {
-                set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
+                for (j = last_j + 1;
+                     j != (last_j == 0 ? max_numa_node : last_j - 1); j++) {
+                    if (j >= max_numa_node) {
+                        j = 0;
+                    }
+                    if (numa_info[j].present) {
+                        break;
+                    }
+                }
+                last_j = j;
+                set_bit(i, numa_info[j].node_cpu);
             }
         }
     }

@@ -217,7 +234,7 @@ void set_numa_modes(void)
     int i;
 
     CPU_FOREACH(cpu) {
-        for (i = 0; i < nb_numa_nodes; i++) {
+        for (i = 0; i < max_numa_node; i++) {
             if (test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
                 cpu->numa_node = i;
             }

vl.c +3 -0

index 54b4627..e1a6ab8 100644
@@ -195,6 +195,7 @@ static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
     QTAILQ_HEAD_INITIALIZER(fw_boot_order);
 
 int nb_numa_nodes;
+int max_numa_node;
 NodeInfo numa_info[MAX_NODES];
 
 uint8_t qemu_uuid[16];

@@ -2967,10 +2968,12 @@ int main(int argc, char **argv, char **envp)
 
     for (i = 0; i < MAX_NODES; i++) {
         numa_info[i].node_mem = 0;
+        numa_info[i].present = false;
         bitmap_zero(numa_info[i].node_cpu, MAX_CPUMASK_BITS);
     }
 
     nb_numa_nodes = 0;
+    max_numa_node = -1;
     nb_nics = 0;
 
     bdrv_init_with_whitelist();

Quoted 6 lines · On Mon, 23 Jun 2014 12:33:10 -0700 Nishanth Aravamudan…

Add a present field to NodeInfo which indicates if a given nodeid was present on the command-line or not. Current code relies on a memory value being passed for a node to indicate presence, which is insufficient in the presence of memoryless nodes or sparse numbering. Adjust the iteration of various NUMA loops to use the maximum known NUMA ID rather than the number of NUMA nodes.

Why would you need sparse numbering?
What task exactly are you trying to solve?

Quoted 6 lines · On Mon, 23 Jun 2014 12:33:10 -0700 Nishanth Aravamudan…

Add a present field to NodeInfo which indicates if a given nodeid was present on the command-line or not. Current code relies on a memory value being passed for a node to indicate presence, which is insufficient in the presence of memoryless nodes or sparse numbering. Adjust the iteration of various NUMA loops to use the maximum known NUMA ID rather than the number of NUMA nodes.

Why would you need sparse numbering?

What task exactly are you trying to solve?