Re: [Qemu-devel] [PATCH v8 02/11] numa: move numa global variable nb_numa_nodes into MachineState

[Igor Mammedov]

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;
> >>       }
> > [...]
> > 
> 
>