Re: [Qemu-devel] [PATCH for-2.10 10/23] numa: mirror cpu to node mapping in MachineState::possible_cpus

[Igor Mammedov]

On Thu, 13 Apr 2017 10:58:05 -0300
Eduardo Habkost <address@hidden> wrote:

> On Wed, Mar 22, 2017 at 02:32:35PM +0100, Igor Mammedov wrote:
> > Introduce machine_set_cpu_numa_node() helper that stores
> > node mapping for CPU in MachineState::possible_cpus.
> > CPU and node it belongs to is specified by 'props' argument.
> > 
> > Patch doesn't remove old way of storing mapping in
> > numa_info[X].node_cpu as removing it at the same time
> > makes patch rather big. Instead it just mirrors mapping
> > in possible_cpus and follow up per target patches will
> > switch to possible_cpus and numa_info[X].node_cpu will
> > be removed once there isn't any users left.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  include/hw/boards.h |  2 ++
> >  hw/core/machine.c   | 68 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  numa.c              |  8 +++++++
> >  3 files changed, 78 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 1dd0fde..40f30f1 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
> >  bool machine_mem_merge(MachineState *machine);
> >  void machine_register_compat_props(MachineState *machine);
> >  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState 
> > *machine);
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp);
> >  
> >  /**
> >   * CPUArchId:
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 0d92672..6ff0b45 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -388,6 +388,74 @@ HotpluggableCPUList 
> > *machine_query_hotpluggable_cpus(MachineState *machine)
> >      return head;
> >  }
> >  
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp) 
> >  
> 
> If you change this to:
> 
>   void cpu_slot_set_numa_node(CPUArchId *slot, uint64_t node_id,
>                               Error **errp);
> 
> and move the CPU slot lookup code from machine_set_cpu_numa_node()
> to a helper:
> 
>   CPUArchId *machine_get_cpu_slot(MachineState *machine,
>                                   CpuInstanceProperties *props, Error **errp);
it would work in case of exact 1:1 lookup, but Paolo asked for
wildcard support (i.e. -numa cpu,node-id=x,socket-id=y should set
mapping for all cpus in socket y).
So I'd prefer to keep machine_set_cpu_numa_node() as is,
with it series splits nicely in clean and bisectable patches
without breaking anything in the middle.


> and change cpu_index_to_cpu_instance_props to return CPUArchId:
> 
>   CPUArchId *cpu_index_to_cpu_slot(MachineState *machine, int cpu_index);
> 
> We could simply have this on "-numa cpu" code:
> 
>     slot = machine_get_cpu_slot(machine, props);
>     cpu_slot_set_numa_node(slot, node_id);
> 
> and this on the legacy "-numa node,cpu=..." code:
> 
>     slot = mc->cpu_index_to_cpu_slot(machine, i);
>     cpu_slot_set_numa_node(slot, node_id);
> 
> I believe we will be able to reuse machine_get_cpu_slot() to
> replace pc_find_cpu_slot() and spapr_find_cpu_slot() later.
As I already replied to David, xxx_find_cpu_slot() possibly might
be generalized but I'd like to postpone it until ARM topology
is materialized and merged.

Another reason I'd like to postpone generalization is that
xxx_find_cpu_slot() could be optimized in target specific way
replacing CPU lookup in array with computational expression.
It will make lookup O(1) function and it could be used as
a better replacement for qemu_get_cpu()/cpu_exists() but
I haven't looked into this yet.

 
> (I also suggest renaming "CPUArchId" and "possible CPUs" to
> "CPUSlot" and "CPU slots" in the code and comments. This would
> help people reviewing the code, but it can be done later if you
> prefer.)
I'm fine with changing CPUArchId to CPUSlot but I'd leave
"possible CPUs" as is, since it precisely describes what it is,
"CPU slots" is too ambiguous.

> 
> [...]
> > @@ -177,6 +178,10 @@ static void numa_node_parse(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);
> >      }
> >  
> >      if (node->has_mem && node->has_memdev) {
> > @@ -393,9 +398,12 @@ void parse_numa_opts(MachineState *ms)
> >          if (i == nb_numa_nodes) {
> >              for (i = 0; i < max_cpus; i++) {
> >                  CpuInstanceProperties props;
> > +                /* fetch default mapping from board and enable it */
> >                  props = mc->cpu_index_to_instance_props(ms, i);
> > +                props.has_node_id = true;
> >  
> >                  set_bit(i, numa_info[props.node_id].node_cpu);
> > +                machine_set_cpu_numa_node(ms, &props, &error_fatal);
> >              }
> >          }
> >  
> > -- 
> > 2.7.4
> > 
> >   
>