[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks f
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-ppc] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr |
Date: |
Mon, 22 May 2017 09:58:45 -0300 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Mon, May 22, 2017 at 08:39:31AM +0200, Igor Mammedov wrote:
> On Thu, 18 May 2017 15:19:13 -0300
> Eduardo Habkost <address@hidden> wrote:
>
> > On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > > 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.
[...]
> > > + } else if (node_id != slot->props.node_id) {
> > > + error_setg(errp, "node-id=%d must match numa node specified "
> > > + "with -numa option", node_id);
> >
> > There's less detail on the error message, now. Probably harmless,
> > but I would like to understand when exactly this can be
> > triggered: is device_add the only way to trigger this error
> > message?
> error is triggered only during -device/device_add so there were no
> need in more verbose error as device_add will report its arguments
> (affected cpu in this case)
OK.
--
Eduardo
- Re: [Qemu-ppc] [PATCH 2/3] numa: move default mapping init to machine, (continued)
[Qemu-ppc] [PATCH 3/3] numa: silence incomplete mapping warning under qtest, Igor Mammedov, 2017/05/18
[Qemu-ppc] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr, Igor Mammedov, 2017/05/18
Re: [Qemu-ppc] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr, David Gibson, 2017/05/18