vl.c: Support multiple CPU ranges on -numa option

This allows us to use the cpu property multiple times to specify multiple cpu (ranges) to the -numa option :

-numa node,cpu=1,cpu=2,cpu=4
or
-numa node,cpu=1-3,cpu=5

Signed-off-by Bandan Das <[email protected]>

---
v3: Convert to using QemuOpts
Use -cpu rather than -cpus which probably probably makes it more
meaningful for non-range arguments

Sorry for reviving this up :)

This is a follow up to earlier proposals sent by Eduardo.

References :
1. http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03832.html
2. https://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03857.html

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

---
 qemu-options.hx |   5 +--
 vl.c            | 108 ++++++++++++++++++++++++++++++++++----------------------
 2 files changed, 68 insertions(+), 45 deletions(-)

qemu-options.hx +3 -2

index bf94862..0e46f5e 100644
@@ -95,12 +95,13 @@ specifies the maximum number of hotpluggable CPUs.
 ETEXI
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
-    "-numa node[,mem=size][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
+    "-numa node[,mem=size][,cpu=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
 STEXI
 @item -numa @var{opts}
 @findex -numa
 Simulate a multi node NUMA system. If mem and cpus are omitted, resources
-are split equally.
+are split equally. The "-cpu" property may be specified multiple times
+to denote multiple cpus or cpu ranges.
 ETEXI
 
 DEF("add-fd", HAS_ARG, QEMU_OPTION_add_fd,

vl.c +65 -43

index f94ec9c..519ca4c 100644
@@ -516,6 +516,32 @@ static QemuOptsList qemu_realtime_opts = {
     },
 };
 
+static QemuOptsList qemu_numa_opts = {
+    .name = "numa",
+    .implied_opt_name = "type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_numa_opts.head),
+    .desc = {
+        {
+            .name = "type",
+            .type = QEMU_OPT_STRING,
+            .help = "node type"
+        },{
+            .name = "nodeid",
+            .type = QEMU_OPT_NUMBER,
+            .help = "node ID"
+        },{
+            .name = "mem",
+            .type = QEMU_OPT_SIZE,
+            .help = "memory size"
+        },{
+            .name = "cpu",
+            .type = QEMU_OPT_STRING,
+            .help = "cpu number or range"
+        },
+        { /* end of list */ }
+    },
+};
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;

@@ -1349,56 +1375,37 @@ error:
     exit(1);
 }
 
-static void numa_add(const char *optarg)
+
+static int numa_add_cpus(const char *name, const char *value, void *opaque)
 {
-    char option[128];
-    char *endptr;
-    unsigned long long nodenr;
+    int *nodenr = opaque;
 
-    optarg = get_opt_name(option, 128, optarg, ',');
-    if (*optarg == ',') {
-        optarg++;
+    if (!strcmp(name, "cpu")) {
+        numa_node_parse_cpus(*nodenr, value);
     }
-    if (!strcmp(option, "node")) {
-
-        if (nb_numa_nodes >= MAX_NODES) {
-            fprintf(stderr, "qemu: too many NUMA nodes\n");
-            exit(1);
-        }
+    return 0;
+}
 
-        if (get_param_value(option, 128, "nodeid", optarg) == 0) {
-            nodenr = nb_numa_nodes;
-        } else {
-            if (parse_uint_full(option, &nodenr, 10) < 0) {
-                fprintf(stderr, "qemu: Invalid NUMA nodeid: %s\n", option);
-                exit(1);
-            }
-        }
+static int numa_init_func(QemuOpts *opts, void *opaque)
+{
+    uint64_t nodenr, mem_size;
 
-        if (nodenr >= MAX_NODES) {
-            fprintf(stderr, "qemu: invalid NUMA nodeid: %llu\n", nodenr);
-            exit(1);
-        }
+    nodenr = qemu_opt_get_number(opts, "nodeid", nb_numa_nodes++);
 
-        if (get_param_value(option, 128, "mem", optarg) == 0) {
-            node_mem[nodenr] = 0;
-        } else {
-            int64_t sval;
-            sval = strtosz(option, &endptr);
-            if (sval < 0 || *endptr) {
-                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
-                exit(1);
-            }
-            node_mem[nodenr] = sval;
-        }
-        if (get_param_value(option, 128, "cpus", optarg) != 0) {
-            numa_node_parse_cpus(nodenr, option);
-        }
-        nb_numa_nodes++;
-    } else {
-        fprintf(stderr, "Invalid -numa option: %s\n", option);
+    if (nodenr >= MAX_NODES) {
+        fprintf(stderr, "qemu: Max number of NUMA nodes reached : %d\n",
+                (int)nodenr);
         exit(1);
     }
+
+    mem_size = qemu_opt_get_size(opts, "mem", 0);
+    node_mem[nodenr] = mem_size;
+
+    if (qemu_opt_foreach(opts, numa_add_cpus, &nodenr, 1) < 0) {
+        return -1;
+    }
+
+    return 0;
 }
 
 static void smp_parse(const char *optarg)

@@ -2901,6 +2908,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_object_opts);
     qemu_add_opts(&qemu_tpmdev_opts);
     qemu_add_opts(&qemu_realtime_opts);
+    qemu_add_opts(&qemu_numa_opts);
 
     runstate_init();
 

@@ -3087,7 +3095,16 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_numa:
-                numa_add(optarg);
+                olist = qemu_find_opts("numa");
+                opts = qemu_opts_parse(olist, optarg, 1);
+                if (!opts) {
+                    exit(1);
+                }
+                optarg = qemu_opt_get(opts, "type");
+                if (!optarg || strcmp(optarg, "node")) {
+                    fprintf(stderr, "qemu: Incorrect format for numa option\n");
+                    exit(1);
+                }
                 break;
             case QEMU_OPTION_display:
                 display_type = select_display(optarg);

@@ -4217,6 +4234,11 @@ int main(int argc, char **argv, char **envp)
 
     register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
 
+    if (qemu_opts_foreach(qemu_find_opts("numa"), numa_init_func,
+                          NULL, 1) != 0) {
+        exit(1);
+    }
+
     if (nb_numa_nodes > 0) {
         int i;
 

Quoted 18 lines · On Tue, 18 Jun 2013 16:09:49 -0400 Bandan Das <[email protected]> wrote:

This allows us to use the cpu property multiple times to specify multiple cpu (ranges) to the -numa option :

-numa node,cpu=1,cpu=2,cpu=4
or
-numa node,cpu=1-3,cpu=5

Signed-off-by Bandan Das <[email protected]>

---
v3: Convert to using QemuOpts
Use -cpu rather than -cpus which probably probably makes it more
meaningful for non-range arguments

Sorry for reviving this up :)

This is a follow up to earlier proposals sent by Eduardo.

Quoted 8 lines

References :
1. http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03832.html
2. https://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03857.html

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

In context of x86, allowing to specify CPU threads using cpu_index isn't correct, since node calculated from APIC ID and node it gets from ACPI table could differ.

It could be better for CLI interface to accept socket number and build always correct NUMA mapping internally using APIC IDs from CPUs, as it's done in real hardware.

In addition it would allow to deprecate use of cpu_index on CLI interface, and simplify following re-factoring to use socket/[core/]thread as means to address/ specify specific CPUs there and later in monitor/qmp interface as well.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

libvirt uses the "cpus" option already, so we have to keep it working.

Sure, we can leave it as it's now for some time while a new interface is introduced/adopted. And than later deprecate "cpus".

Quoted 25 lines · Igor Mammedov

Quoted 22 lines · On Tue, 18 Jun 2013 16:09:49 -0400 Bandan Das <[email protected]> wrote:

This allows us to use the cpu property multiple times to specify multiple cpu (ranges) to the -numa option :

-numa node,cpu=1,cpu=2,cpu=4
or
-numa node,cpu=1-3,cpu=5

Signed-off-by Bandan Das <[email protected]>

---
v3: Convert to using QemuOpts
Use -cpu rather than -cpus which probably probably makes it more
meaningful for non-range arguments

Sorry for reviving this up :)

This is a follow up to earlier proposals sent by Eduardo.

References :
1. http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03832.html
2. https://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03857.html

Quoted 4 lines

Quoted 4 lines

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

libvirt uses the "cpus" option already, so we have to keep it working.

Quoted 8 lines

In context of x86, allowing to specify CPU threads using cpu_index isn't correct, since node calculated from APIC ID and node it gets from ACPI table could differ.

It could be better for CLI interface to accept socket number and build always correct NUMA mapping internally using APIC IDs from CPUs, as it's done in real hardware.

Quoted 3 lines

In addition it would allow to deprecate use of cpu_index on CLI interface, and simplify following re-factoring to use socket/[core/]thread as means to address/ specify specific CPUs there and later in monitor/qmp interface as well.

What about simply accepting a QOM object path? Today we could only accept CPU thread objects (because there are no socket/core objects yet), but the day we introduce CPU socket objects, we can change the code to accept them without changing the syntax.

Il 20/06/2013 11:30, Igor Mammedov ha scritto:

Quoted 3 lines

Quoted 1 line

Quoted 1 line

libvirt uses the "cpus" option already, so we have to keep it working.

Sure, we can leave it as it's now for some time while a new interface is introduced/adopted. And than later deprecate "cpus".

So, you used a new name because the new behavior of "-numa node,cpus=1-2,cpus=3-4" would be incompatible with the old.

Quoted 7 lines · Paolo Bonzini

Il 20/06/2013 11:30, Igor Mammedov ha scritto:

Quoted 3 lines

Quoted 1 line

Quoted 1 line

libvirt uses the "cpus" option already, so we have to keep it working.

Sure, we can leave it as it's now for some time while a new interface is introduced/adopted. And than later deprecate "cpus".

So, you used a new name because the new behavior of "-numa node,cpus=1-2,cpus=3-4" would be incompatible with the old.

BTW, as I tried to touch exactly the same place yesterday (trying to convert it to QemuOpts) -- what does this "node" mean?

For example, with

-device [type=]devicetype,foo=bar,xzy=abc

this creates a new device for each "invocation" of option. But what does this `-numa node' mean? Can there be anything else besides node? Why it is needed/used for?

This -numa option is the last one which uses the old option parsing mechanism (there's also some smbios-related thing but it's simple to convert, I almost got it ready yesterday),

Il 20/06/2013 11:30, Igor Mammedov ha scritto:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

Quoted 3 lines

Quoted 1 line

Quoted 1 line

libvirt uses the "cpus" option already, so we have to keep it working.

Sure, we can leave it as it's now for some time while a new interface is introduced/adopted. And than later deprecate "cpus".

So, you used a new name because the new behavior of "-numa node,cpus=1-2,cpus=3-4" would be incompatible with the old.

Personally I don't think that's a problem, but I remember a long discussion in the past. Igor/Eduardo, do you remember the conclusions?

Quoted 10 lines

Il 20/06/2013 11:30, Igor Mammedov ha scritto:

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

libvirt uses the "cpus" option already, so we have to keep it working.

Sure, we can leave it as it's now for some time while a new interface is introduced/adopted. And than later deprecate "cpus".

Quoted 2 lines

So, you used a new name because the new behavior of "-numa node,cpus=1-2,cpus=3-4" would be incompatible with the old.

I don't think anybody uses "cpus=1-2,cpus=3-4" today, so I believe we can change its behavior. The problem was to get agreement on the syntax to represent multiple CPU ranges.

Quoted 2 lines

Personally I don't think that's a problem, but I remember a long discussion in the past. Igor/Eduardo, do you remember the conclusions?

I don't remember seeing the discussion reach any conclusion, unfortunately.

Il 20/06/2013 15:26, Eduardo Habkost ha scritto:

Quoted 14 lines · Paolo Bonzini

Quoted 12 lines

Il 20/06/2013 11:30, Igor Mammedov ha scritto:

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

libvirt uses the "cpus" option already, so we have to keep it working.

Sure, we can leave it as it's now for some time while a new interface is introduced/adopted. And than later deprecate "cpus".

So, you used a new name because the new behavior of "-numa node,cpus=1-2,cpus=3-4" would be incompatible with the old.

Quoted 3 lines

I don't think anybody uses "cpus=1-2,cpus=3-4" today, so I believe we can change its behavior. The problem was to get agreement on the syntax to represent multiple CPU ranges.

Ok. I think almost everyone agreed on "cpus=1-2,cpus=3-4", which is basically what Bandan's patch does minus s/cpu/cpus/. It matches what already happens with other options (SLIRP), so it's hardly surprising.

Let's go on with that.

Paolo

Quoted 6 lines

Quoted 2 lines

Personally I don't think that's a problem, but I remember a long discussion in the past. Igor/Eduardo, do you remember the conclusions?

I don't remember seeing the discussion reach any conclusion, unfortunately.

Quoted 19 lines · Paolo Bonzini

Il 20/06/2013 15:26, Eduardo Habkost ha scritto:

Quoted 17 lines · Paolo Bonzini

Quoted 6 lines

Il 20/06/2013 11:30, Igor Mammedov ha scritto:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

libvirt uses the "cpus" option already, so we have to keep it working.

Sure, we can leave it as it's now for some time while a new interface is introduced/adopted. And than later deprecate "cpus".

So, you used a new name because the new behavior of "-numa node,cpus=1-2,cpus=3-4" would be incompatible with the old.

I don't think anybody uses "cpus=1-2,cpus=3-4" today, so I believe we can change its behavior. The problem was to get agreement on the syntax to represent multiple CPU ranges.

Quoted 3 lines

Ok. I think almost everyone agreed on "cpus=1-2,cpus=3-4", which is basically what Bandan's patch does minus s/cpu/cpus/. It matches what already happens with other options (SLIRP), so it's hardly surprising.

Good, so should I spin a new version with "cpus" ?

Also note that this patch actually doesn't add any extra code to support multiple cpus arguments. It all happens automatically as part of conversion to QemuOpts. So, if we need to revisit the syntax later, we can always do that.

Bandan

Quoted 10 lines

Let's go on with that.

Paolo

Quoted 6 lines

Quoted 2 lines

Personally I don't think that's a problem, but I remember a long discussion in the past. Igor/Eduardo, do you remember the conclusions?

I don't remember seeing the discussion reach any conclusion, unfortunately.

Quoted 25 lines · Bandan Das

Quoted 22 lines · Paolo Bonzini

Il 20/06/2013 15:26, Eduardo Habkost ha scritto:

Quoted 11 lines · Paolo Bonzini

Quoted 10 lines

Il 20/06/2013 11:30, Igor Mammedov ha scritto:

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

So, basically the format seemed easier to work with if we are thinking of using QemuOpts for -numa. Using -cpu rather than cpus probably makes it less ambiguous as well IMO. However, it's probably not a good idea if the current syntax is well established ?

libvirt uses the "cpus" option already, so we have to keep it working.

Sure, we can leave it as it's now for some time while a new interface is introduced/adopted. And than later deprecate "cpus".

Quoted 6 lines

Quoted 2 lines

So, you used a new name because the new behavior of "-numa node,cpus=1-2,cpus=3-4" would be incompatible with the old.

I don't think anybody uses "cpus=1-2,cpus=3-4" today, so I believe we can change its behavior. The problem was to get agreement on the syntax to represent multiple CPU ranges.

Ok. I think almost everyone agreed on "cpus=1-2,cpus=3-4", which is basically what Bandan's patch does minus s/cpu/cpus/. It matches what already happens with other options (SLIRP), so it's hardly surprising.

Quoted 1 line

Good, so should I spin a new version with "cpus" ?

I already merged your patch to my patch set "Add support for binding guest numa nodes to host numa nodes" since I should base on that.

Thanks,
Wanlong Gao

Quoted 18 lines

Also note that this patch actually doesn't add any extra code to support multiple cpus arguments. It all happens automatically as part of conversion to QemuOpts. So, if we need to revisit the syntax later, we can always do that.

Bandan

Quoted 10 lines

Let's go on with that.

Paolo

Quoted 6 lines

Quoted 2 lines

Personally I don't think that's a problem, but I remember a long discussion in the past. Igor/Eduardo, do you remember the conclusions?

I don't remember seeing the discussion reach any conclusion, unfortunately.