Re: [Qemu-devel] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 22 May 2017 09:58:45 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, May 22, 2017 at 08:39:31AM +0200, Igor Mammedov wrote:
> > On Thu, 18 May 2017 15:19:13 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:  
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  include/sysemu/numa.h |  1 +
> > > >  hw/arm/virt.c         | 16 ++--------------
> > > >  hw/i386/pc.c          | 17 +----------------
> > > >  hw/ppc/spapr.c        | 17 +----------------
> > > >  numa.c                | 22 ++++++++++++++++++++++
> > > >  5 files changed, 27 insertions(+), 46 deletions(-)
> > > > 
> > > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > > index 7ffde5b..610eece 100644
> > > > --- a/include/sysemu/numa.h
> > > > +++ b/include/sysemu/numa.h
> > > > @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > > >                                   int nb_nodes, ram_addr_t size);
> > > >  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);    
> > > 
> > > I understand an explicitly call to numa_cpu_pre_plug() is needed
> > > on spapr_core_pre_plug() because it is not handling a TYPE_CPU
> > > object. But why not adding a numa_cpu_pre_plug() call to
> > > cpu_common_realizefn(), so the explicit calls in machvirt_init()
> > > and pc_cpu_pre_plug() are not necessary?  
> > 1. of the reasons is not to pollute all cpus with numa code  
> 
> I understand this goal...
> 
> >   
> > > Adding the code to cpu_common_realizefn() would also ensure
> > > CPUState::node_id is set consistently, even if hotplug was not
> > > done at thread level.  
> > 2. not all CPUs have node-id property  
> 
> ...and this. But: we already have the CPUState::numa_node field.
> If we don't handle it in common code, we risk leaving the field
> uninitialized, which is a problem if other code tries to use the
> field for something.
> 
> Maybe that's an argument for removing the CPUState::numa_node
> field too.
> 
> 
> > 3. call site of thread_realize() in encapsulating object (spapr_core)
> >    might be somewhere in the middle of parent's realize and likely
> >    failure would need proper parent state rollback/cleanup.  
> 
> I don't see why this could be a problem, if the code setting
> realized=true is already supposed to handle errors on the realize
> method.
> 
> > 4. and finely it's not cpu's responsibility to assign/check
> >    node-id property. It's machine's job that owns/manages topology
> >    layout. It' the same like with socket/core/thread properties.
> >    So for the sake of small optimization, I'm not really willing
> >    to violate that.  
> 
> I don't disagree with that, but in that case I would like to
> remove the CPUState::numa_node field soon, if possible.
ok, I'll try add a patch on respin to do it (I think that I've
tried this at some RFC time)