[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [RFC PATCH v0 5/6] qmp, spapr: Show hot-plug
|
From: |
Igor Mammedov |
|
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [RFC PATCH v0 5/6] qmp, spapr: Show hot-plugged/pluggable CPU slots in the Machine |
|
Date: |
Tue, 1 Mar 2016 14:55:02 +0100 |
On Tue, 1 Mar 2016 14:39:51 +0530
Bharata B Rao <address@hidden> wrote:
> On Mon, Feb 29, 2016 at 11:46:42AM +0100, Igor Mammedov wrote:
> > On Thu, 25 Feb 2016 21:52:41 +0530
> > Bharata B Rao <address@hidden> wrote:
> >
> > > Implement query cpu-slots that provides information about hot-plugged
> > > as well as hot-pluggable CPU slots that the machine supports.
> > >
> > > TODO: As Eric suggested use enum for type instead of str.
> > > TODO: @hotplug-granularity probably isn't required.
> > >
> > > Signed-off-by: Bharata B Rao <address@hidden>
> > > ---
> > > hw/core/machine.c | 19 +++++++++
> > > hw/ppc/spapr.c | 112
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > include/hw/boards.h | 4 ++
> > > qapi-schema.json | 85 +++++++++++++++++++++++++++++++++++++++
> > > qmp-commands.hx | 47 ++++++++++++++++++++++
> > > 5 files changed, 267 insertions(+)
> > >
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 6d1a0d8..3055ef8 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -17,6 +17,25 @@
> > > #include "hw/sysbus.h"
> > > #include "sysemu/sysemu.h"
> > > #include "qemu/error-report.h"
> > > +#include "qmp-commands.h"
> > > +
> > > +/*
> > > + * QMP: query-cpu-slots
> > > + *
> > > + * TODO: Ascertain if this is the right place to for this arch-neutral
> > > routine.
> > > + */
> > > +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> > > +{
> > > + MachineState *ms = MACHINE(qdev_get_machine());
> > > + MachineClass *mc = MACHINE_GET_CLASS(ms);
> > > +
> > > + if (!mc->cpu_slots) {
> > > + error_setg(errp, QERR_UNSUPPORTED);
> > > + return NULL;
> > > + }
> > > +
> > > + return mc->cpu_slots(ms);
> > > +}
> > >
> > > static char *machine_get_accel(Object *obj, Error **errp)
> > > {
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 780cd00..b76ed85 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2453,6 +2453,117 @@ static unsigned
> > > spapr_cpu_index_to_socket_id(unsigned cpu_index)
> > > return cpu_index / smp_threads / smp_cores;
> > > }
> > >
> > > +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> > > +{
> > > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > + CPUInfoList ***prev = opaque;
> > > +
> > > + if (object_dynamic_cast(obj, TYPE_CPU)) {
> > > + CPUInfoList *elem = g_new0(CPUInfoList, 1);
> > > + CPUInfo *s = g_new0(CPUInfo, 1);
> > > + CPUState *cpu = CPU(obj);
> > > + PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> > > +
> > > + s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> > > + s->type = g_strdup(object_get_typename(obj));
> > > + s->thread = cpu->cpu_index;
> > > + s->has_thread = true;
> > > + s->core = cpu->cpu_index / smp_threads;
> > > + s->has_core = true;
> > > + if (mc->cpu_index_to_socket_id) {
> > > + s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> > > + } else {
> > > + s->socket = cpu->cpu_index / smp_threads / smp_cores;
> > > + }
> > > + s->has_socket = true;
> > > + s->node = cpu->numa_node;
> > > + s->has_node = true;
> > > + s->qom_path = object_get_canonical_path(obj);
> > > + s->has_qom_path = true;
> > > +
> > > + elem->value = s;
> > > + elem->next = NULL;
> > > + **prev = elem;
> > > + *prev = &elem->next;
> > > + }
> > > + object_child_foreach(obj, spapr_cpuinfo_list, opaque);
> > > + return 0;
> > > +}
> > > +
> > > +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> > > +{
> > > + CPUSlotInfoList *head = NULL;
> > > + CPUSlotInfoList **prev = &head;
> > > + Object *root_container;
> > > + ObjectProperty *prop;
> > > + ObjectPropertyIterator iter;
> > > +
> > > + /*
> > > + * TODO: There surely must be a better/easier way to walk all
> > > + * the link properties of an object ?
> > > + */
> > > + root_container = container_get(object_get_root(), "/machine");
> > > + object_property_iter_init(&iter, root_container);
> > > +
> > > + while ((prop = object_property_iter_next(&iter))) {
> > > + Object *obj;
> > > + DeviceState *dev;
> > > + CPUSlotInfoList *elem;
> > > + CPUSlotInfo *s;
> > > + CPUInfoList *cpu_head = NULL;
> > > + CPUInfoList **cpu_prev = &cpu_head;
> > > + sPAPRCPUCore *core;
> > > +
> > > + if (!strstart(prop->type, "link<", NULL)) {
> > > + continue;
> > > + }
> > > +
> > > + if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> > > + continue;
> > > + }
> > > +
> > > + elem = g_new0(CPUSlotInfoList, 1);
> > > + s = g_new0(CPUSlotInfo, 1);
> > > +
> > > + obj = object_property_get_link(root_container, prop->name, NULL);
> > > + if (obj) {
> > > + /* Slot populated */
> > > + dev = DEVICE(obj);
> > > + core = SPAPR_CPU_CORE(obj);
> > > +
> > > + if (dev->id) {
> > > + s->has_id = true;
> > > + s->id = g_strdup(dev->id);
> > > + }
> > > + s->realized = object_property_get_bool(obj, "realized",
> > > NULL);
> > > + s->nr_cpus = core->nr_threads;
> > > + s->has_nr_cpus = true;
> > > + s->qom_path = object_get_canonical_path(obj);
> > > + s->has_qom_path = true;
> > > + if (s->realized) {
> > > + spapr_cpuinfo_list(obj, &cpu_prev);
> > > + }
> > > + s->has_cpus = true;
> > > + } else {
> > > + /* Slot empty */
> > > + s->has_id = false;
> > > + s->has_nr_cpus = false;
> > > + s->has_qom_path = false;
> > > + s->has_cpus = false;
> > > + s->realized = false;
> > > + }
> > > + s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> > > + s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> > > + s->slot_id = g_strdup(prop->name);
> > > + s->cpus = cpu_head;
> > > + elem->value = s;
> > > + elem->next = NULL;
> > > + *prev = elem;
> > > + prev = &elem->next;
> > > + }
> > > + return head;
> > > +}
> > > +
> > > static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > {
> > > MachineClass *mc = MACHINE_CLASS(oc);
> > > @@ -2482,6 +2593,7 @@ static void spapr_machine_class_init(ObjectClass
> > > *oc, void *data)
> > > hc->plug = spapr_machine_device_plug;
> > > hc->unplug = spapr_machine_device_unplug;
> > > mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > > + mc->cpu_slots = spapr_cpu_slots;
> > >
> > > smc->dr_lmb_enabled = true;
> > > smc->dr_cpu_enabled = true;
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index 0f30959..d888a02 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -57,6 +57,9 @@ bool machine_mem_merge(MachineState *machine);
> > > * Set only by old machines because they need to keep
> > > * compatibility on code that exposed QEMU_VERSION to guests in
> > > * the past (and now use qemu_hw_version()).
> > > + * @cpu_slots:
> > > + * Provides information about populated and yet-to-be populated
> > > + * CPU slots in the machine. Used by QMP query-cpu-slots.
> > > */
> > > struct MachineClass {
> > > /*< private >*/
> > > @@ -99,6 +102,7 @@ struct MachineClass {
> > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > DeviceState *dev);
> > > unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> > > + CPUSlotInfoList *(*cpu_slots)(MachineState *machine);
> > > };
> > >
> > > /**
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 8d04897..e9a52a2 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -4083,3 +4083,88 @@
> > > ##
> > > { 'enum': 'ReplayMode',
> > > 'data': [ 'none', 'record', 'play' ] }
> > > +
> > > +##
> > > +# @CPUInfo:
> > > +#
> > > +# Information about CPUs
> > > +#
> > > +# @arch-id: Arch-specific ID for the CPU.
> > > +#
> > > +# @type: QOM type of the CPU.
> > > +#
> > > +# @thread: Thread ID of the CPU.
> > > +#
> > > +# @core: Core ID of the CPU.
> > > +#
> > > +# @socket: Socket ID of the CPU.
> > > +#
> > > +# @node: NUMA node to which the CPU belongs.
> > > +#
> > > +# @qom-path: QOM path of the CPU object
> > > +#
> > > +# Since: 2.6
> > > +##
> > > +
> > > +{ 'struct': 'CPUInfo',
> > > + 'data': { 'arch-id': 'int',
> > > + 'type': 'str',
> > > + '*thread': 'int',
> > > + '*core': 'int',
> > > + '*socket' : 'int',
> > > + '*node' : 'int',
> > > + '*qom-path': 'str'
> > > + }
> > > +}
> > > +
> > > +##
> > > +# @CPUSlotInfo:
> > > +#
> > > +# Information about CPU Slots
> > > +#
> > > +# @id: Device ID of the CPU composite object that occupies the slot.
> > > +#
> > > +# @type: QOM type of the CPU composite object that occupies the slot.
> > > +#
> > > +# @hotplug-granularity: Granularity of CPU composite hotplug for this
> > > slot,
> > > +# can be thread, core or socket.
> > > +#
> > > +# @slot-id: Slot's identifier.
> > > +#
> > > +# @qom-path: QOM path of the CPU composite object that occupies the slot.
> > > +#
> > > +# @realized: A boolean that indicates whether the slot is filled or
> > > empty.
> > > +#
> > > +# @nr-cpus: Number of CPUs that are part of CPU composite object that
> > > occupies
> > > +# this slot.
> > > +#
> > > +# @cpus: An array of @CPUInfo elements where each element describes one
> > > +# CPU that is part of this slot's CPU composite object.
> > > +#
> > > +# @type: QOM type
> > > +#
> > > +# Since: 2.6
> > > +##
> > > +
> > > +{ 'struct': 'CPUSlotInfo',
> > > + 'data': { '*id': 'str',
> > > + 'type': 'str',
> > > + 'hotplug-granularity' : 'str',
> > Does it convey any useful info, if yes how it will be used by mgmt?
>
> As I noted in the patch desc, this field is useless, will remove.
>
> >
> > > + 'slot-id' : 'str',
> > > + '*qom-path': 'str',
> > > + 'realized': 'bool',
> > field's redundant, presence of qom-path answers this question
>
> Ok, makes sense.
>
> >
> > > + '*nr-cpus': 'int',
> > > + '*cpus' : ['CPUInfo']
> > I'd suggest to drop above 2 fields as it's impl dependent,
> > qom-path already point's to socket/core/thread object and
> > its internal composition can be explored by other means if needed.
> >
> > Moreover 'CPUInfo' is confusing and sort of conflicts with existing
> > 'CpuInfo'.
>
> Ah I see, should change the naming if we eventually stick with this
> implementation.
>
> > I'd drop CPUInfo altogether and introduce only 'CPUSlotInfo' here,
> > existing thread enumeration's already implemented query-cpus.
>
> Ok.
>
> >
> > > + }
> > > +}
> > What I miss here is that CPUSlotInfo doesn't provide any
> > information to about where CPU might be hotplugged to.
> >
> > Maybe use following tuple instead of slot-id?
> >
> > { 'struct': 'CPUSlotProperties',
> > 'data': { '*node': 'int',
> > '*socket': 'int',
> > '*core': 'int',
> > '*thread': 'int'
> > }
> > }
>
> Hmm not sure. If I undestand Andreas' proposal correctly, slot is the
> place where the CPU sits. Machine determines the type of the slot and it
> can be socket slot, core slot or thread slot based on the granularity
> of the hotplug supported by the machine. With this I don't see why
> anything else apart from slot-id/slot-name is required to figure out where
> to hoplug CPU.
Of cause it's possible to create and keep at board level map of
slot-names to whatever other info needed to create a CPU object
at machine init time, and then make board somehow to apply it
to the new CPU object before realizing it.
But it doesn't give mgmt any information whatsoever about where CPU
is being hotplugged. So for x86/arm we would end up with adding
yet another interface that would tell it. If CPUSlotProperties is used
instead of slot-name, it could convey that information while
keeping interface compatible with a range targets we care about
and extendable to other targets in future.
> In the current implementation, sPAPR hotplug is at core granularity.
> CPUSlotInfo.type advertises the type as spapr-cpu-core. The number of
> threads in the core and the CPU model of the threads are either machine
> default or specified by user during hotplug time.
now imagine extending in future sPAPR to support NUMA, which way would
you extend interface, how would mgmt know which CPU belongs to what node?
As you may note in CPUSlotProperties fields are optional so target would
use only ones it needs. For current sPAPR, query result could be
something like this:
{
{
type: spapr-cpu-core
properties: { core: 0 }
qom-path: /machine/unattached/device[xxx]
}
{
type: spapr-cpu-core
properties: { core: 1 }
}
...
{
type: spapr-cpu-core
properties: { core: 0 }
}
}
Later if numa was needed, the board could set 'numa' property as well.
Would that work for sPAPR?
> I believe NUMA node should be the property of the slot. The information
> about which slots are part of which NUMA node should be known beforehand
> at machine init time. Hotplugging CPU thread, core or socket to a slot will
> make that thread, core or socket part of that NUMA node.
As analogy PCI devices also have 'bus' property which is used to tell
device_add machinery where to attach the device. The same goes for DIMM
device which also has 'numa' option so mgmt could say to which node device
is being plugged.
CPUSlotProperties is basically location where CPU is being plugged
and in some cases location includes 'numa' property or some other.
The difference vs slot-name is that CPUSlotProperties is more
verbose and flexible and tries to describe location in natural
for target therms.
> > it's generic for the most targets, which should work for spapr, s390, x86,
> > ARM
> > and could be extended for other cases adding other board specific
> > properties if it's needed.
>
> I don't about ARM, but s390 doesn't care about topology iiuc.
>
> For sPAPR, as I described above, we don't care about sockets and hence
> I believe we can live with hotplugging a core into a machine slot by
> referring to the slot-id.
>
> I see that CPUSlotProperties that you are suggesting here would make sense
> for x86 depending on the granularity at which hotplug is done. I know there
> has been discussion on thread vs socket hotplug granularity for x86, has
> there been a consensus on what it is going to be ?
I don't agree with switching to socket model on x86 for a number of reasons:
- it can't be done without breaking compatibility.
- from user pov, he/she would like to hotplug CPU at thread level to get
necessary CPU horsepower without over-paying for over-provisioned threads.
Limiting it to socket level would be crippling qemu based hypervisors
in comparison to other hypervisors, which do it currently at thread level.
Practice show that Linux and Windows handle thread level hotplug well
enough perhaps because they try match ACPI spec where hotplug happens
at thread level.