On Mon, 22 May 2017 09:58:45 -0300 Eduardo Habkost <ehabk...@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 <ehabk...@redhat.com> wrote: > > > > > On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote: > > > > Signed-off-by: Igor Mammedov <imamm...@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)
