Re: [Qemu-devel] [PATCH v3 7/7] numa: cpu: calculate/set default node-ids after all -numa CLI options are parsed

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 30 May 2017 17:03:32 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, May 30, 2017 at 06:24:02PM +0200, Igor Mammedov wrote:
[...]
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 76ce021..063f329 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -94,6 +94,8 @@ typedef struct {
> >   *    Returns an array of @CPUArchId architecture-dependent CPU IDs
> >   *    which includes CPU IDs for present and possible to hotplug CPUs.
> >   *    Caller is responsible for freeing returned list.
> > + * @get_default_cpu_node_id:
> > + *    returns default board specific node_id value for CPU
> >   * @has_hotpluggable_cpus:
> >   *    If true, board supports CPUs creation with -device/device_add.
> >   * @minimum_page_bits:
> > @@ -151,6 +153,7 @@ struct MachineClass {
> >      CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
> >                                                           unsigned cpu_index);
> >      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> > +    int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);  
> 
> Aren't we trying to move away from cpu_index-based CPU
> identification?  Shouldn't we make this get a CPUArchId argument?
it's not cpu-index but index in possible_cpus[],
it's possible to pass in CPUArchId argument and then in callbacks
compute it's index in possible_cpus[] but that seems
a bit unnecessary and would complicate callback impl.

Probably I should add doc comment explaining 'idx' meaning.

> >  };
> >  
> >  /**
> > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > index 610eece..ea123ef 100644
> > --- a/include/sysemu/numa.h
> > +++ b/include/sysemu/numa.h
> > @@ -36,4 +36,13 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> >  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> >                                    int nb_nodes, ram_addr_t size);
> >  void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
> > +
> > +static inline void assert_nb_numa_nodes_change(void)  
> 
> Can you document the purpose of this function?
> 
> > +{
> > +    static int saved_nb_numa_nodes;
> > +    assert(nb_numa_nodes);
> > +    assert(saved_nb_numa_nodes == 0 || saved_nb_numa_nodes == nb_numa_nodes);
> > +    saved_nb_numa_nodes = nb_numa_nodes;
> > +}
> > +
> >  #endif
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index ce676df..74f1453 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1553,6 +1553,13 @@ virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
> >      return possible_cpus->cpus[cpu_index].props;
> >  }
> >  
> > +static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx)
> > +{
> > +    assert(nb_numa_nodes);
> > +    assert_nb_numa_nodes_change();  
> 
> I would move this assert to common code instead of copying it to
> all implementations.
> 
> A machine_get_default_cpu_node_id() wrapper that calls
> assert_nb_numa_nodes_change() and mc->get_default_cpu_node_id()
> would be enough, and it wouldn't even need to be declared in a
> header as the only caller would be at hw/core/machine.c.
> 
> (Or, if you prefer to simply drop the assert(), I think that
> would be OK too.)
Callbacks are 'public' API and potentially could be called from anywhere.
Putting asserts inside of callbacks instead of the current single call site
should help to catch errors/wrong usage in future if callback were called
from elsewhere.

So I'd prefer to keep it where it's now or drop it altogether
as it's not much of use at the current call site.
 
> > +    return idx % nb_numa_nodes;
> > +}
> > +
> >  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >  {
> >      int n;
[...]