Re: [Qemu-devel] [PATCH v3 06/18] numa: mirror cpu to node mapping in MachineState::possible_cpus

[Igor Mammedov]

On Fri, 26 May 2017 12:46:25 -0300
Eduardo Habkost <address@hidden> wrote:

> On Wed, May 10, 2017 at 01:29:50PM +0200, Igor Mammedov wrote:
> [...]
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 2482c63..420c8c4 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -389,6 +389,102 @@ HotpluggableCPUList 
> > *machine_query_hotpluggable_cpus(MachineState *machine)  
> [...]
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               const CpuInstanceProperties *props, Error 
> > **errp)
> > +{  
> [...]
> > +    /* force board to initialize possible_cpus if it hasn't been done yet 
> > */
> > +    mc->possible_cpu_arch_ids(machine);  
> [...]
> > diff --git a/numa.c b/numa.c
> > index 7182481..7db5dde 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -170,6 +170,7 @@ static void parse_numa_node(MachineState *ms, 
> > NumaNodeOptions *node,
> >          exit(1);
> >      }
> >      for (cpus = node->cpus; cpus; cpus = cpus->next) {
> > +        CpuInstanceProperties props;
> >          if (cpus->value >= max_cpus) {
> >              error_setg(errp,
> >                         "CPU index (%" PRIu16 ")"
> > @@ -178,6 +179,10 @@ static void parse_numa_node(MachineState *ms, 
> > NumaNodeOptions *node,
> >              return;
> >          }
> >          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
> > +        props = mc->cpu_index_to_instance_props(ms, cpus->value);
> > +        props.node_id = nodenr;
> > +        props.has_node_id = true;
> > +        machine_set_cpu_numa_node(ms, &props, &error_fatal);  
> 
> This triggers a call to possible_cpu_arch_ids() before
> nb_numa_nodes is set to the actual number of NUMA nodes in the
> machine, breaking the "node_id = ... % nb_numa_nodes"
> initialization logic in pc, virt, and spapr.
Looking at it more, in current code this problem theoretically
affects only partial mapping case. And even there it's masked by 
fixup to 0 node for non explicitly mapped CPUs and the bug which
 *) "[PATCH v2 3/5] numa: make sure that all cpus in has  has_node_id set if 
numa is enabled"
fixes. So invalid default values for non mapped cpus are stored
in possible_cpus but not used nor visible elsewhere.

[*] as side effect fixes invalid defaults in possible_cpus as well
by fixup to 0 node that's moved from pre_plug handler.

In future we are going to remove partial mapping with fixup to 0
and fetch default mappings from possible_cpus where these wrong
defaults won't exists (or matter) as CPUs are going to either
 1) be all explicitly mapped (so all node_id values are overwritten)
or
 2) all default mapped (where defaults will be correct since no
    -numa cpu=/-numa node,cpus= were called), but that's fragile
    so I'd add calculate defaults at machine_run_board_init()
    time anyway.