[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: |
Igor Mammedov |
Subject: |
Re: [Qemu-ppc] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr |
Date: |
Mon, 22 May 2017 08:39:31 +0200 |
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
> 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
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.
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.
> > #endif
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index c7c8159..ce676df 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
> > for (n = 0; n < possible_cpus->len; n++) {
> > Object *cpuobj;
> > CPUState *cs;
> > - int node_id;
> >
> > if (n >= smp_cpus) {
> > break;
> > @@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
> > cs = CPU(cpuobj);
> > cs->cpu_index = n;
> >
> > - node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
> > - if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
> > - /* by default CPUState::numa_node was 0 if it's not set via CLI
> > - * keep it this way for now but in future we probably should
> > - * refuse to start up with incomplete numa mapping */
> > - node_id = 0;
> > - }
> > - if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> > - cs->numa_node = node_id;
> > - } else {
> > - /* CPU isn't device_add compatible yet, this shouldn't happen
> > */
> > - error_setg(&error_abort, "user set node-id not implemented");
> > - }
> > + numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index],
> > DEVICE(cpuobj),
> > + &error_fatal);
> >
> > if (!vms->secure) {
> > object_property_set_bool(cpuobj, false, "has_el3", NULL);
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e36a375..d83c158 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler
> > *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > int idx;
> > - int node_id;
> > CPUState *cs;
> > CPUArchId *cpu_slot;
> > X86CPUTopoInfo topo;
> > @@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler
> > *hotplug_dev,
> > cs = CPU(cpu);
> > cs->cpu_index = idx;
> >
> > - node_id = cpu_slot->props.node_id;
> > - if (!cpu_slot->props.has_node_id) {
> > - /* by default CPUState::numa_node was 0 if it's not set via CLI
> > - * keep it this way for now but in future we probably should
> > - * refuse to start up with incomplete numa mapping */
> > - node_id = 0;
> > - }
> > - if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> > - cs->numa_node = node_id;
> > - } else if (cs->numa_node != node_id) {
> > - error_setg(errp, "node-id %d must match numa node specified"
> > - "with -numa option for cpu-index %d",
> > - cs->numa_node, cs->cpu_index);
> > - return;
> > - }
> > + numa_cpu_pre_plug(cpu_slot, dev, errp);
> > }
> >
> > static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0980d73..c7fee8b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
> > Error *local_err = NULL;
> > CPUCore *cc = CPU_CORE(dev);
> > - sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> > char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> > const char *type = object_get_typename(OBJECT(dev));
> > CPUArchId *core_slot;
> > - int node_id;
> > int index;
> >
> > if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> > @@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > goto out;
> > }
> >
> > - node_id = core_slot->props.node_id;
> > - if (!core_slot->props.has_node_id) {
> > - /* by default CPUState::numa_node was 0 if it's not set via CLI
> > - * keep it this way for now but in future we probably should
> > - * refuse to start up with incomplete numa mapping */
> > - node_id = 0;
> > - }
> > - if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
> > - sc->node_id = node_id;
> > - } else if (sc->node_id != node_id) {
> > - error_setg(&local_err, "node-id %d must match numa node specified"
> > - "with -numa option for cpu-index %d", sc->node_id,
> > cc->core_id);
> > - goto out;
> > - }
> > + numa_cpu_pre_plug(core_slot, dev, &local_err);
> >
> > out:
> > g_free(base_core_type);
> > diff --git a/numa.c b/numa.c
> > index ca73145..0115bfd 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
> > }
> > }
> >
> > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error
> > **errp)
> > +{
> > + int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);
>
> You don't check for errors here. If they will never happen,
> should we use &error_abort instead?
sure, I'll use &error_abort in v2
> > +
> > + if (node_id == CPU_UNSET_NUMA_NODE_ID) {
> > + /* by default CPUState::numa_node was 0 if it wasn't set explicitly
> > + * TODO: make it error when incomplete numa mapping support is
> > removed
> > + */
> > + node_id = 0;
> > +
> > + /* due to bug in libvirt, it doesn't pass node-id from props on
> > + * device_add as expected, so we have to fix it up here */
> > + if (slot->props.has_node_id) {
> > + node_id = slot->props.node_id;
> > + }
> > + object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
> > + } 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)
>
> > + }
> > +}
> > +
> > static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
> > const char *name,
> > uint64_t ram_size)
> > --
> > 2.7.4
> >
>
- 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