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

[Igor Mammedov]

On Mon, 29 May 2017 10:36:47 -0300
Eduardo Habkost <address@hidden> wrote:

> On Mon, May 29, 2017 at 03:12:45PM +0200, Igor Mammedov wrote:
> > 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.
> > > 
> > > The initialization ordering between possible_cpus and NUMA data
> > > structures looks very subtle and fragile.  I still don't see an
> > > obvious way to untangle that.  
> > It's unfixable unless we require specific ordering on CLI,
> > i.e. first go all '-numa node,nodeid=[...]' options and only then
> > the rest of [-numa node,cpus|cpu].
> > We can do that for '-numa cpu' (probably should do enforce it for
> > this new option anyway for saner CLI)
> > 
> > but not for '-numa node,cpus' as it will break existing users
> > that do not declare nodes first.
> >   
> > > I suggest moving the default-NUMA-mapping code to a separate
> > > machine class method, instead of relying on
> > > possible_cpu_arch_ids() to initialize node_id.  
> > So as you suggest we have to postpone default values initialization
> > till all the options are parsed:
> > 
> > 1: strait-forward additional machine callback called from
> >    machine_run_board_init()
> > 
> > or:
> > 
> > 2: save extra callback and recalculate not yet set props.node_id-s
> >    in possible_cpu_arch_ids() if nb_numa_nodes is changed since
> >    the last invocation of possible_cpu_arch_ids()
> > 
> > which one would you prefer?  
> 
> Option 1 sounds simpler to me.
ok, I'll include fix on cleanups series respin.