Re: [Qemu-devel] [PATCH V4 01/10] NUMA: Support multiple CPU ranges on -numa option

[Eduardo Habkost]

On Thu, Jul 04, 2013 at 05:53:08PM +0800, Wanlong Gao wrote:
> From: Bandan Das <address@hidden>
> 
> This allows us to use the "cpus" property multiple times
> to specify multiple cpu (ranges) to the -numa option :
> 
> -numa node,cpus=1,cpus=2,cpus=4
> or
> -numa node,cpus=1-3,cpus=5
> 
> Note that after this patch, the defalut suffix of "-numa node,mem=N"
> will no longer be "M". So we must add the suffix "M" like "-numa node,mem=NM"
> when assigning "N MB" of node memory size.

Such an incompatible change is not acceptable, as it would break
existing configurations. libvirt doesn't specify any suffix and expects
it to always mean "MB".

> 
> Signed-off-by: Bandan Das <address@hidden>
> Signed-off-by: Wanlong Gao <address@hidden>
> ---
>  qemu-options.hx |   3 +-
>  vl.c            | 108 
> ++++++++++++++++++++++++++++++++++----------------------
>  2 files changed, 67 insertions(+), 44 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 137a39b..449cf36 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -100,7 +100,8 @@ 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 "-cpus" property may be specified multiple times

The option is not named "-cpus", but just "cpus". And I believe it is
normally called an "option", not a "property".

> +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 6d9fd7d..6f2e17a 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 = "cpus",
> +            .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 QemuOptsList qemu_smp_opts = {
> @@ -2933,6 +2940,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();
>  
> @@ -3119,7 +3127,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");

Why not do this inside numa_init_func()?

> +                    exit(1);
> +                }
>                  break;
>              case QEMU_OPTION_display:
>                  display_type = select_display(optarg);
> @@ -4195,6 +4212,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.3.2.634.g7a3187e
> 
> 

-- 
Eduardo