diff mbox

[v4,04/29] NUMA: convert -numa option to use OptsVisitor

Message ID 6c190c63ca7fe6a1d6d5d0944b23240018d76310.1402299637.git.hutao@cn.fujitsu.com
State New
Headers show

Commit Message

Hu Tao June 9, 2014, 10:25 a.m. UTC
From: Wanlong Gao <gaowanlong@cn.fujitsu.com>

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Tested-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 include/sysemu/sysemu.h |   3 +-
 numa.c                  | 145 +++++++++++++++++++++++-------------------------
 qapi-schema.json        |  32 +++++++++++
 vl.c                    |  11 +++-
 4 files changed, 114 insertions(+), 77 deletions(-)

Comments

Eduardo Habkost June 16, 2014, 2:08 p.m. UTC | #1
On Mon, Jun 09, 2014 at 06:25:09PM +0800, Hu Tao wrote:
> From: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Tested-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>

So, this is when the ability to use multiple "cpus" ranges on -numa is
finally enabled, right?

Is there some capability probing mechanism that can be used by
management to detect the new feature?
Paolo Bonzini June 16, 2014, 2:16 p.m. UTC | #2
Il 16/06/2014 16:08, Eduardo Habkost ha scritto:
> Is there some capability probing mechanism that can be used by
> management to detect the new feature?

No, there isn't.

Paolo
Eric Blake June 16, 2014, 2:23 p.m. UTC | #3
On 06/16/2014 08:16 AM, Paolo Bonzini wrote:
> Il 16/06/2014 16:08, Eduardo Habkost ha scritto:
>> Is there some capability probing mechanism that can be used by
>> management to detect the new feature?
> 
> No, there isn't.

Not even query-command-line-options?
Paolo Bonzini June 16, 2014, 2:32 p.m. UTC | #4
Il 16/06/2014 16:23, Eric Blake ha scritto:
> On 06/16/2014 08:16 AM, Paolo Bonzini wrote:
>> Il 16/06/2014 16:08, Eduardo Habkost ha scritto:
>>> Is there some capability probing mechanism that can be used by
>>> management to detect the new feature?
>>
>> No, there isn't.
>
> Not even query-command-line-options?

No, this is a (backwards-compatible) extension to how the option is parsed.

I suppose with QAPI introspection you could check if the QAPI type for 
-numa is defined...

Paolo
Eduardo Habkost June 16, 2014, 3:39 p.m. UTC | #5
On Mon, Jun 16, 2014 at 04:32:09PM +0200, Paolo Bonzini wrote:
> Il 16/06/2014 16:23, Eric Blake ha scritto:
> >On 06/16/2014 08:16 AM, Paolo Bonzini wrote:
> >>Il 16/06/2014 16:08, Eduardo Habkost ha scritto:
> >>>Is there some capability probing mechanism that can be used by
> >>>management to detect the new feature?
> >>
> >>No, there isn't.
> >
> >Not even query-command-line-options?
> 
> No, this is a (backwards-compatible) extension to how the option is parsed.

But numa started appearing on query-command-line-options only after this
patch got applied. It is not the semantics I would expect from
query-command-line-options, but it is a hint that we have a QEMU version
with the new numa option handling code.

> 
> I suppose with QAPI introspection you could check if the QAPI type for -numa
> is defined...

Does this exist, or is it just an idea?
Paolo Bonzini June 16, 2014, 3:46 p.m. UTC | #6
Il 16/06/2014 17:39, Eduardo Habkost ha scritto:
> > No, this is a (backwards-compatible) extension to how the option is parsed.
>
> But numa started appearing on query-command-line-options only after this
> patch got applied. It is not the semantics I would expect from
> query-command-line-options, but it is a hint that we have a QEMU version
> with the new numa option handling code.

Oh, then indeed it's possible.  I had forgotten that -numa was using ad 
hoc parsing code, not QemuOpts.

>> > I suppose with QAPI introspection you could check if the QAPI type for -numa
>> > is defined...
> Does this exist, or is it just an idea?

Amos was working on it.

Paolo
Eric Blake June 16, 2014, 4:05 p.m. UTC | #7
On 06/16/2014 09:46 AM, Paolo Bonzini wrote:
> Il 16/06/2014 17:39, Eduardo Habkost ha scritto:
>> > No, this is a (backwards-compatible) extension to how the option is
>> parsed.
>>
>> But numa started appearing on query-command-line-options only after this
>> patch got applied. It is not the semantics I would expect from
>> query-command-line-options, but it is a hint that we have a QEMU version
>> with the new numa option handling code.
> 
> Oh, then indeed it's possible.  I had forgotten that -numa was using ad
> hoc parsing code, not QemuOpts.

Then we have our witness, even without waiting for introspection :)

> 
>>> > I suppose with QAPI introspection you could check if the QAPI type
>>> for -numa
>>> > is defined...
>> Does this exist, or is it just an idea?
> 
> Amos was working on it.

Although given that soft freeze is tomorrow, it looks very unlikely that
we'll have it in 2.1.
diff mbox

Patch

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 3a9308b..4102be3 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -148,9 +148,10 @@  typedef struct node_info {
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
 } NodeInfo;
 extern NodeInfo numa_info[MAX_NODES];
-void numa_add(const char *optarg);
 void set_numa_nodes(void);
 void set_numa_modes(void);
+extern QemuOptsList qemu_numa_opts;
+int numa_init_func(QemuOpts *opts, void *opaque);
 
 #define MAX_OPTION_ROMS 16
 typedef struct QEMUOptionRom {
diff --git a/numa.c b/numa.c
index fb9bffc..43573c5 100644
--- a/numa.c
+++ b/numa.c
@@ -28,101 +28,96 @@ 
 #include "qom/cpu.h"
 #include "qemu/error-report.h"
 #include "include/exec/cpu-common.h" /* for RAM_ADDR_FMT */
-
-static void numa_node_parse_cpus(int nodenr, const char *cpus)
+#include "qapi-visit.h"
+#include "qapi/opts-visitor.h"
+#include "qapi/dealloc-visitor.h"
+#include "qapi/qmp/qerror.h"
+
+QemuOptsList qemu_numa_opts = {
+    .name = "numa",
+    .implied_opt_name = "type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_numa_opts.head),
+    .desc = { { 0 } } /* validated with OptsVisitor */
+};
+
+static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
 {
-    char *endptr;
-    unsigned long long value, endvalue;
+    uint16_t nodenr;
+    uint16List *cpus = NULL;
 
-    /* Empty CPU range strings will be considered valid, they will simply
-     * not set any bit in the CPU bitmap.
-     */
-    if (!*cpus) {
-        return;
-    }
-
-    if (parse_uint(cpus, &value, &endptr, 10) < 0) {
-        goto error;
-    }
-    if (*endptr == '-') {
-        if (parse_uint_full(endptr + 1, &endvalue, 10) < 0) {
-            goto error;
-        }
-    } else if (*endptr == '\0') {
-        endvalue = value;
+    if (node->has_nodeid) {
+        nodenr = node->nodeid;
     } else {
-        goto error;
+        nodenr = nb_numa_nodes;
     }
 
-    if (endvalue >= MAX_CPUMASK_BITS) {
-        endvalue = MAX_CPUMASK_BITS - 1;
-        fprintf(stderr,
-            "qemu: NUMA: A max of %d VCPUs are supported\n",
-             MAX_CPUMASK_BITS);
+    if (nodenr >= MAX_NODES) {
+        error_setg(errp, "Max number of NUMA nodes reached: %"
+                   PRIu16 "\n", nodenr);
+        return;
     }
 
-    if (endvalue < value) {
-        goto error;
+    for (cpus = node->cpus; cpus; cpus = cpus->next) {
+        if (cpus->value > MAX_CPUMASK_BITS) {
+            error_setg(errp, "CPU number %" PRIu16 " is bigger than %d",
+                       cpus->value, MAX_CPUMASK_BITS);
+            return;
+        }
+        bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
     }
 
-    bitmap_set(numa_info[nodenr].node_cpu, value, endvalue-value+1);
-    return;
-
-error:
-    fprintf(stderr, "qemu: Invalid NUMA CPU range: %s\n", cpus);
-    exit(1);
+    if (node->has_mem) {
+        uint64_t mem_size = node->mem;
+        const char *mem_str = qemu_opt_get(opts, "mem");
+        /* Fix up legacy suffix-less format */
+        if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
+            mem_size <<= 20;
+        }
+        numa_info[nodenr].node_mem = mem_size;
+    }
 }
 
-void numa_add(const char *optarg)
+int numa_init_func(QemuOpts *opts, void *opaque)
 {
-    char option[128];
-    char *endptr;
-    unsigned long long nodenr;
+    NumaOptions *object = NULL;
+    Error *err = NULL;
 
-    optarg = get_opt_name(option, 128, optarg, ',');
-    if (*optarg == ',') {
-        optarg++;
+    {
+        OptsVisitor *ov = opts_visitor_new(opts);
+        visit_type_NumaOptions(opts_get_visitor(ov), &object, NULL, &err);
+        opts_visitor_cleanup(ov);
     }
-    if (!strcmp(option, "node")) {
 
-        if (nb_numa_nodes >= MAX_NODES) {
-            fprintf(stderr, "qemu: too many NUMA nodes\n");
-            exit(1);
-        }
+    if (err) {
+        goto error;
+    }
 
-        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);
-            }
+    switch (object->kind) {
+    case NUMA_OPTIONS_KIND_NODE:
+        numa_node_parse(object->node, opts, &err);
+        if (err) {
+            goto error;
         }
+        nb_numa_nodes++;
+        break;
+    default:
+        abort();
+    }
 
-        if (nodenr >= MAX_NODES) {
-            fprintf(stderr, "qemu: invalid NUMA nodeid: %llu\n", nodenr);
-            exit(1);
-        }
+    return 0;
 
-        if (get_param_value(option, 128, "mem", optarg) == 0) {
-            numa_info[nodenr].node_mem = 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);
-            }
-            numa_info[nodenr].node_mem = 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);
-        exit(1);
+error:
+    qerror_report_err(err);
+    error_free(err);
+
+    if (object) {
+        QapiDeallocVisitor *dv = qapi_dealloc_visitor_new();
+        visit_type_NumaOptions(qapi_dealloc_get_visitor(dv),
+                               &object, NULL, NULL);
+        qapi_dealloc_visitor_cleanup(dv);
     }
+
+    return -1;
 }
 
 void set_numa_nodes(void)
diff --git a/qapi-schema.json b/qapi-schema.json
index 7bc33ea..8ce01cb 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4722,3 +4722,35 @@ 
               'btn'     : 'InputBtnEvent',
               'rel'     : 'InputMoveEvent',
               'abs'     : 'InputMoveEvent' } }
+
+##
+# @NumaOptions
+#
+# A discriminated record of NUMA options. (for OptsVisitor)
+#
+# Since 2.1
+##
+{ 'union': 'NumaOptions',
+  'data': {
+    'node': 'NumaNodeOptions' }}
+
+##
+# @NumaNodeOptions
+#
+# Create a guest NUMA node. (for OptsVisitor)
+#
+# @nodeid: #optional NUMA node ID (increase by 1 from 0 if omitted)
+#
+# @cpus: #optional VCPUs belonging to this node (assign VCPUS round-robin
+#         if omitted)
+#
+# @mem: #optional memory size of this node (equally divide total memory among
+#        nodes if omitted)
+#
+# Since: 2.1
+##
+{ 'type': 'NumaNodeOptions',
+  'data': {
+   '*nodeid': 'uint16',
+   '*cpus':   ['uint16'],
+   '*mem':    'size' }}
diff --git a/vl.c b/vl.c
index a38e6dd..730bd1e 100644
--- a/vl.c
+++ b/vl.c
@@ -2938,6 +2938,7 @@  int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_realtime_opts);
     qemu_add_opts(&qemu_msg_opts);
     qemu_add_opts(&qemu_name_opts);
+    qemu_add_opts(&qemu_numa_opts);
 
     runstate_init();
 
@@ -3133,7 +3134,10 @@  int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_numa:
-                numa_add(optarg);
+                opts = qemu_opts_parse(qemu_find_opts("numa"), optarg, 1);
+                if (!opts) {
+                    exit(1);
+                }
                 break;
             case QEMU_OPTION_display:
                 display_type = select_display(optarg);
@@ -4303,6 +4307,11 @@  int main(int argc, char **argv, char **envp)
     default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
     default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
 
+    if (qemu_opts_foreach(qemu_find_opts("numa"), numa_init_func,
+                          NULL, 1) != 0) {
+        exit(1);
+    }
+
     set_numa_nodes();
 
     if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {