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

[Eduardo Habkost]

Just noticed I didn't reply to this yet:

On Wed, Apr 19, 2017 at 11:31:05AM +0200, Igor Mammedov wrote:
> 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.

Makes sense to me. And this is also a good reason we won't have a
1:1 mapping from query-hotpluggable-cpus output to -numa cpu
input. (So this answers my question about the intent to change
query-hotpluggable-cpus output)

> 
> 
> > 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.

OK.

> 
> 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 would prefer optimization in generic code to machine-specific
code just for optimization. But both approaches are valid, let's
see how this evolves.

> 
>  
> > (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.

No problem to me. We can discuss that later, anyway.

Thanks!

-- 
Eduardo