Re: [Qemu-devel] [PATCH V6 02/11] NUMA: split -numa option

[Wanlong Gao]

On 07/30/2013 07:14 PM, Laszlo Ersek wrote:
> On 07/30/13 09:32, Wanlong Gao wrote:
> 
>> +static int numa_node_parse(NumaNodeOptions *opts)
>> +{
>> +    uint16_t nodenr;
>> +    UInt16List *cpus = NULL;
>> +
>> +    if (opts->has_nodeid) {
>> +        nodenr = opts->nodeid;
>> +        if (nodenr >= MAX_NODES) {
>> +            fprintf(stderr, "qemu: Max number of NUMA nodes reached: %"
>> +                    PRIu16 "\n", nodenr);
>> +            return -1;
>> +        }
>> +    } else {
>> +        nodenr = nb_numa_nodes;
>> +    }
>> +
>> +    for (cpus = opts->cpus; cpus; cpus = cpus->next) {
>> +        bitmap_set(node_cpumask[nodenr], cpus->value->u16, 1);
>> +    }
>> +
>> +    if (opts->has_mem) {
>> +        int64_t mem_size;
>> +        char *endptr;
>> +        mem_size = strtosz(opts->mem, &endptr);
>> +        if (mem_size < 0 || *endptr) {
>> +            fprintf(stderr, "qemu: invalid numa mem size: %s\n", opts->mem);
>> +            return -1;
>> +        }
>> +        node_mem[nodenr] = mem_size;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int numa_mem_parse(NumaMemOptions *opts)
>> +{
>> +    uint16_t nodenr;
>> +    uint64_t mem_size;
>> +
>> +    if (opts->has_nodeid) {
>> +        nodenr = opts->nodeid;
>> +        if (nodenr >= MAX_NODES) {
>> +            fprintf(stderr, "qemu: Max number of NUMA nodes reached: %"
>> +                    PRIu16 "\n", nodenr);
>> +            return -1;
>> +        }
>> +    } else {
>> +        nodenr = nb_numa_mem_nodes;
>> +    }
>> +
>> +    if (opts->has_size) {
>> +        mem_size = opts->size;
>> +        node_mem[nodenr] = mem_size;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> Unless I'm missing something:
> 
> Just like "NumaMemOptions.size" (which has type 'size' in the JSON),
> OptsVisitor could parse "NumaNodeOptions.mem" for you as well, if you
> switched the latter's type to 'size' too.
> 
> ... Hm, not really. This is probably a compatibility thing. Without any
> specific suffix, strtosz() passes STRTOSZ_DEFSUFFIX_MB to
> strtosz_suffix() (ie. the legacy optarg is expressed in megabytes, if
> the user doesn't specify a suffix him/herself), while opts_type_size()
> passes STRTOSZ_DEFSUFFIX_B.

Yeah, right.

> 
> I think this patch is good; if you adapt it to the ['UInt16'] ->
> ['uint16'] change I proposed for 01/11, you can add

Done, thank you.

Wanlong Gao

> 
> Reviewed-by: Laszlo Ersek <address@hidden>
> 
> Thanks!
> Laszlo
>