[Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Bandan Das]

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 <bsd@redhat.com>
---
 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(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index bf94862..0e46f5e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -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,
diff --git a/vl.c b/vl.c
index f94ec9c..519ca4c 100644
--- a/vl.c
+++ b/vl.c
@@ -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;
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Igor Mammedov]

On Tue, 18 Jun 2013 16:09:49 -0400
Bandan Das <bsd@redhat.com> 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 <bsd@redhat.com>
> ---
>  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 ?
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.

> 
> ---
>  qemu-options.hx |   5 +--
>  vl.c            | 108 ++++++++++++++++++++++++++++++++++----------------------
>  2 files changed, 68 insertions(+), 45 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index bf94862..0e46f5e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -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,
> diff --git a/vl.c b/vl.c
> index f94ec9c..519ca4c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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;
>  

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Eduardo Habkost]

On Wed, Jun 19, 2013 at 01:42:52PM +0200, Igor Mammedov wrote:
> On Tue, 18 Jun 2013 16:09:49 -0400
> Bandan Das <bsd@redhat.com> 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 <bsd@redhat.com>
> > ---
> >  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 ?

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

> 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.

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Igor Mammedov]

On Wed, 19 Jun 2013 10:26:42 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jun 19, 2013 at 01:42:52PM +0200, Igor Mammedov wrote:
> > On Tue, 18 Jun 2013 16:09:49 -0400
> > Bandan Das <bsd@redhat.com> 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 <bsd@redhat.com>
> > > ---
> > >  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 ?
> 
> 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".

> 
> > 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.
> 
> 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.
It doesn't matter if it's socket=N or QOM path, the idea is not to allow
individual CPU threads there to avoid misconfiguration, but use socket entities 
in some form in interface part. Sockets could be dummy containers for initial
implementation so not to delay sanitizing NUMA code.

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Paolo Bonzini]

Il 20/06/2013 11:30, Igor Mammedov ha scritto:
>>>> > > > 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.

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?

Paolo

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Michael Tokarev]

20.06.2013 13:52, Paolo Bonzini wrote:
> Il 20/06/2013 11:30, Igor Mammedov ha scritto:
>>>> 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),
but it is rather non-standard.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Paolo Bonzini]

Il 20/06/2013 13:34, Michael Tokarev ha scritto:
> 20.06.2013 13:52, Paolo Bonzini wrote:
>> Il 20/06/2013 11:30, Igor Mammedov ha scritto:
>>>>> 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?

Nothing, I think it's just that somebody took inspiration from "-device". :)

> 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),

That would be awesome.

Paolo

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Eduardo Habkost]

On Thu, Jun 20, 2013 at 11:52:42AM +0200, Paolo Bonzini wrote:
> Il 20/06/2013 11:30, Igor Mammedov ha scritto:
> >>>> > > > 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.

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.

> 
> 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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Paolo Bonzini]

Il 20/06/2013 15:26, Eduardo Habkost ha scritto:
> On Thu, Jun 20, 2013 at 11:52:42AM +0200, Paolo Bonzini wrote:
>> Il 20/06/2013 11:30, Igor Mammedov ha scritto:
>>>>>>>>> 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.
> 
> 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

>> 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.
> 

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 20/06/2013 15:26, Eduardo Habkost ha scritto:
>> On Thu, Jun 20, 2013 at 11:52:42AM +0200, Paolo Bonzini wrote:
>>> Il 20/06/2013 11:30, Igor Mammedov ha scritto:
>>>>>>>>>> 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.
>> 
>> 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.

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
> Let's go on with that.
>
> Paolo
>
>>> 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.
>> 

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Wanlong Gao]

On 06/21/2013 12:02 AM, Bandan Das wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 20/06/2013 15:26, Eduardo Habkost ha scritto:
>>> On Thu, Jun 20, 2013 at 11:52:42AM +0200, Paolo Bonzini wrote:
>>>> Il 20/06/2013 11:30, Igor Mammedov ha scritto:
>>>>>>>>>>> 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.
>>>
>>> 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.
> 
> 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

> 
> 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
>> Let's go on with that.
>>
>> Paolo
>>
>>>> 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.
>>>
> 
> 

Re: [Qemu-devel] [PATCH v3] vl.c: Support multiple CPU ranges on -numa option

[Bandan Das]

Wanlong Gao <gaowanlong@cn.fujitsu.com> writes:

> On 06/21/2013 12:02 AM, Bandan Das wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 20/06/2013 15:26, Eduardo Habkost ha scritto:
>>>> On Thu, Jun 20, 2013 at 11:52:42AM +0200, Paolo Bonzini wrote:
>>>>> Il 20/06/2013 11:30, Igor Mammedov ha scritto:
>>>>>>>>>>>> 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.
>>>>
>>>> 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.
>> 
>> 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

Oh, great! Thank you for taking care of the "cpus" change.


>> 
>> 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
>>> Let's go on with that.
>>>
>>> Paolo
>>>
>>>>> 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.
>>>>
>> 
>>