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 <ler...@redhat.com>
>> Signed-off-by: Wanlong Gao <gaowanl...@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 <gaowanl...@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
