[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 02/11] numa: move numa global variable nb_num
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v8 02/11] numa: move numa global variable nb_numa_nodes into MachineState |
Date: |
Tue, 30 Jul 2019 11:11:20 +0200 |
On Tue, 30 Jul 2019 08:53:36 +0800
Tao Xu <address@hidden> wrote:
> On 7/29/2019 9:09 PM, Igor Mammedov wrote:
> > On Mon, 29 Jul 2019 14:31:18 +0800
> > Tao Xu <address@hidden> wrote:
> >
> >> Add struct NumaState in MachineState and move existing numa global
> >> nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> >> numa_support into MachineClass to decide which submachines support NUMA.
> >>
> >> Suggested-by: Igor Mammedov <address@hidden>
> >> Suggested-by: Eduardo Habkost <address@hidden>
> >> Signed-off-by: Tao Xu <address@hidden>
> >> ---
> >>
> >> Changes in v8:
> >> - Add check if numa->numa_state is NULL in pxb_dev_realize_common
> >> - Use nb_nodes in spapr_populate_memory() (Igor)
> >> ---
> > [...]
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 821f0d4a49..1c7c12c415 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -331,7 +331,7 @@ static int spapr_fixup_cpu_dt(void *fdt,
> >> SpaprMachineState *spapr)
> >> return ret;
> >> }
> >>
> >> - if (nb_numa_nodes > 1) {
> >> + if (ms->numa_state->num_nodes > 1) {
> >> ret = spapr_fixup_cpu_numa_dt(fdt, offset, cpu);
> >> if (ret < 0) {
> >> return ret;
> >> @@ -351,9 +351,9 @@ static int spapr_fixup_cpu_dt(void *fdt,
> >> SpaprMachineState *spapr)
> >>
> >> static hwaddr spapr_node0_size(MachineState *machine)
> >> {
> >> - if (nb_numa_nodes) {
> >> + if (machine->numa_state->num_nodes) {
> >> int i;
> >> - for (i = 0; i < nb_numa_nodes; ++i) {
> >> + for (i = 0; i < machine->numa_state->num_nodes; ++i) {
> >> if (numa_info[i].node_mem) {
> >> return MIN(pow2floor(numa_info[i].node_mem),
> >> machine->ram_size);
> >> @@ -398,13 +398,14 @@ static int spapr_populate_memory(SpaprMachineState
> >> *spapr, void *fdt)
> >> {
> >> MachineState *machine = MACHINE(spapr);
> >> hwaddr mem_start, node_size;
> >> - int i, nb_nodes = nb_numa_nodes;
> >> + int i, nb_nodes = machine->numa_state->num_nodes;
> >> NodeInfo *nodes = numa_info;
> >> NodeInfo ramnode;
> >>
> >> /* No NUMA nodes, assume there is just one node with whole RAM */
> >> - if (!nb_numa_nodes) {
> >> + if (!nb_nodes) {
> >> nb_nodes = 1;
> >> + machine->numa_state->num_nodes = nb_nodes;
> > You've not addressed a v7 comment
> > On Tue, 23 Jul 2019 16:56:41 +0200
> > Igor Mammedov <address@hidden> wrote:
> >
> >> I don't like user fixing up generic machine data that came from CLI
> >> (or luck of such)
> > [...]
> >> I'd keep fixup local (i.e. using nb_nodes)
> >
> > i.e. do not modify machine->numa_state->num_nodes and just use value local
> > like current code does.
>
> But modify machine->numa_state->num_nodes can fix the issue:
> spapr_populate_memory() creates a implicit node and info
> temporarily but then spapr_validate_node_memory() will use
> global nb_numa_nodes which is 0 and skip check.
it's not related though, there is nothing wrong with fixing a bug
but it's typically done by separate patch with proper description.
(try not to mix unrelated things in one patch)
But otherwise as you noticed, it might be bug or feature in existing spapr impl,
and should be fixed by separate patch if necessary, CCing PPC folks.
PS:
we already have an implicit node creation in generic numa code (when memory
hotplug
is enabled), so we probably could reuse that and a node should be created from
there
instead of fixing up from the code deep within the board.
> Or if I should modify the check part, i.e.
> static void spapr_validate_node_memory(MachineState *machine, Error **errp)
> {
> [...]
> for (i = 0; machine->numa_state->nodes[i] == NULL; i++) {
> if (machine->numa_state->nodes[i].node_mem %
> SPAPR_MEMORY_BLOCK_SIZE) {
> error_setg(errp,
> "Node %d memory size 0x%" PRIx64
> " is not aligned to %" PRIu64 " MiB",
> i, machine->numa_state->nodes[i].node_mem,
> SPAPR_MEMORY_BLOCK_SIZE / MiB);)
>
> >
> >> ramnode.node_mem = machine->ram_size;
> >> nodes = &ramnode;
> >> }
> > [...]
> >
>
>
[Qemu-devel] [PATCH v8 04/11] numa: move numa global variable numa_info into MachineState, Tao Xu, 2019/07/29
[Qemu-devel] [PATCH v8 08/11] hmat acpi: Build Memory Side Cache Information Structure(s), Tao Xu, 2019/07/29
[Qemu-devel] [PATCH v8 01/11] hw/arm: simplify arm_load_dtb, Tao Xu, 2019/07/29
[Qemu-devel] [PATCH v8 07/11] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s), Tao Xu, 2019/07/29
[Qemu-devel] [PATCH v8 06/11] hmat acpi: Build Memory Proximity Domain Attributes Structure(s), Tao Xu, 2019/07/29
[Qemu-devel] [PATCH v8 10/11] numa: Extend the CLI to provide memory side cache information, Tao Xu, 2019/07/29
[Qemu-devel] [PATCH v8 09/11] numa: Extend the CLI to provide memory latency and bandwidth information, Tao Xu, 2019/07/29
[Qemu-devel] [PATCH v8 11/11] tests/bios-tables-test: add test cases for ACPI HMAT, Tao Xu, 2019/07/29