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

[Wanlong Gao <gaowanlong@cn.fujitsu.com>]

On 09/04/2013 09:49 AM, Eduardo Habkost wrote:
> On Fri, Aug 30, 2013 at 11:10:41AM +0800, Wanlong Gao wrote:
>> Change -numa option like following as Paolo suggested:
>>     -numa node,nodeid=0,cpus=0-1 \
>>     -numa mem,nodeid=0,size=1G
>>
>> This new option will make later coming memory hotplug better.
>> And this new option is implemented using OptsVisitor.
>>
>> And just remain "-numa node,mem=xx" as legacy.
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> 
> Would it be possible to first move the existing code as-is to numa.c,
> then introduce qemu_numa_opts, and then introduce "-numa mem"? It would
> make the patch much easier to review.

I thought this patch is straightforward, but if you like I can split as you said. ;)

> 
>> ---
>>  Makefile.target         |   2 +-
>>  include/sysemu/sysemu.h |   3 +
>>  numa.c                  | 144 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-options.hx         |   6 +-
>>  vl.c                    | 113 ++++++-------------------------------
>>  5 files changed, 168 insertions(+), 100 deletions(-)
>>  create mode 100644 numa.c
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index 9a49852..7e1fddf 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -113,7 +113,7 @@ endif #CONFIG_BSD_USER
>>  #########################################################
>>  # System emulator target
>>  ifdef CONFIG_SOFTMMU
>> -obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o
>> +obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o
>>  obj-y += qtest.o
>>  obj-y += hw/
>>  obj-$(CONFIG_FDT) += device_tree.o
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index b1aa059..489b4b6 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -129,8 +129,11 @@ extern QEMUClockType rtc_clock;
>>  #define MAX_NODES 64
>>  #define MAX_CPUMASK_BITS 255
>>  extern int nb_numa_nodes;
>> +extern int nb_numa_mem_nodes;
>>  extern uint64_t node_mem[MAX_NODES];
>>  extern unsigned long *node_cpumask[MAX_NODES];
>> +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
>> new file mode 100644
>> index 0000000..e6924f4
>> --- /dev/null
>> +++ b/numa.c
>> @@ -0,0 +1,144 @@
>> +/*
>> + * QEMU System Emulator
>> + *
>> + * Copyright (c) 2013 Fujitsu Ltd.
>> + * Author: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "sysemu/sysemu.h"
>> +#include "qemu/bitmap.h"
>> +#include "qapi-visit.h"
>> +#include "qapi/opts-visitor.h"
>> +#include "qapi/dealloc-visitor.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 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;
>> +    }
> 
> If you make the (nodenr >= MAX_NODES) check unconditional, you simplify
> the code and you won't need the nb_numa_nodes check at numa_init_func().

Yeah, thank you.

> 
>> +
>> +    for (cpus = opts->cpus; cpus; cpus = cpus->next) {
>> +        bitmap_set(node_cpumask[nodenr], cpus->value, 1);
>> +    }
> 
> What if cpus->value > MAXCPUMASK_BITS?

Will check it.

> 
>> +
>> +    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;
> 
> What if nb_numa_mem_nodes > MAX_NODES?

Yeah, need check unconditional, thank you.

Regards,
Wanlong Gao