| Message ID | 1470383429-11526-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 05, 2016 at 05:50:29PM +1000, David Gibson wrote: > Prior to c8721d3 "spapr: Error out when CPU hotplug is attempted on older > pseries machines", attempting to use query-hotpluggable-cpus on pseries-2.6 > and earlier machine types would SEGV. > > That change fixed that, but due to some unexpected interactions in init > order and a brown-paper-bag worthy failure to test, it accidentally > disabled query-hotpluggable-cpus for all pseries machine types, including > the current one which should allow it. > > In fact, query_hotpluggable_cpus needs to be non-NULL when and only when > the dr_cpu_enabled flag in sPAPRMachineClass is set, which makes > dr_cpu_enabled itself redundant. > > This patch removes dr_cpu_enabled, instead directly setting > query_hotpluggable_cpus from the machine class_init functions, and using > that to determine the availability of CPU hotplug when necessary. dr_cpu_enabled actually determines if CPU hotplug feature is present or not. It also controls the creation of DRC-specific properties in /cpus DT node like ibm,drc-indexes etc query_hotpluggable_cpus just tells us if the machine supports the querying of hotpluggable CPUS. query_hotpluggable_cpus definitely implies dr_cpu_enabled but dr_cpu_enabled can exist on its own (theoretically at the least) without query_hotpluggable_cpus. So I think we should not replace dr_cpu_enabled with query_hotpluggable_cpus. However, I tested this patch and it works as intended. Regards, Bharata.
On Fri, Aug 05, 2016 at 08:21:59PM +0530, Bharata B Rao wrote: > On Fri, Aug 05, 2016 at 05:50:29PM +1000, David Gibson wrote: > > Prior to c8721d3 "spapr: Error out when CPU hotplug is attempted on older > > pseries machines", attempting to use query-hotpluggable-cpus on pseries-2.6 > > and earlier machine types would SEGV. > > > > That change fixed that, but due to some unexpected interactions in init > > order and a brown-paper-bag worthy failure to test, it accidentally > > disabled query-hotpluggable-cpus for all pseries machine types, including > > the current one which should allow it. > > > > In fact, query_hotpluggable_cpus needs to be non-NULL when and only when > > the dr_cpu_enabled flag in sPAPRMachineClass is set, which makes > > dr_cpu_enabled itself redundant. > > > > This patch removes dr_cpu_enabled, instead directly setting > > query_hotpluggable_cpus from the machine class_init functions, and using > > that to determine the availability of CPU hotplug when necessary. > > dr_cpu_enabled actually determines if CPU hotplug feature is present > or not. It also controls the creation of DRC-specific properties > in /cpus DT node like ibm,drc-indexes etc > > query_hotpluggable_cpus just tells us if the machine supports the > querying of hotpluggable CPUS. query_hotpluggable_cpus definitely > implies dr_cpu_enabled but dr_cpu_enabled can exist on its own > (theoretically at the least) without query_hotpluggable_cpus. Um, so that certainly can't happen in practice. Just like query_hotpluggable_cpus, dr_cpu_enabled is set for pseries-2.7 and no earlier machine types. There's no way to override it in configuration. And even in theory I can't see how dr_cpu_enabled could make any sense without query_hotpluggable_cpus. The whole hotplug infrastructure is based around the core objects, which don't exist with the earlier machine types. > So I think we should not replace dr_cpu_enabled with query_hotpluggable_cpus. > > However, I tested this patch and it works as intended. > > Regards, > Bharata. >
On Fri, 5 Aug 2016 20:21:59 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Fri, Aug 05, 2016 at 05:50:29PM +1000, David Gibson wrote: > > Prior to c8721d3 "spapr: Error out when CPU hotplug is attempted on older > > pseries machines", attempting to use query-hotpluggable-cpus on pseries-2.6 > > and earlier machine types would SEGV. > > > > That change fixed that, but due to some unexpected interactions in init > > order and a brown-paper-bag worthy failure to test, it accidentally > > disabled query-hotpluggable-cpus for all pseries machine types, including > > the current one which should allow it. > > > > In fact, query_hotpluggable_cpus needs to be non-NULL when and only when > > the dr_cpu_enabled flag in sPAPRMachineClass is set, which makes > > dr_cpu_enabled itself redundant. > > > > This patch removes dr_cpu_enabled, instead directly setting > > query_hotpluggable_cpus from the machine class_init functions, and using > > that to determine the availability of CPU hotplug when necessary. > > dr_cpu_enabled actually determines if CPU hotplug feature is present > or not. It also controls the creation of DRC-specific properties > in /cpus DT node like ibm,drc-indexes etc > > query_hotpluggable_cpus just tells us if the machine supports the > querying of hotpluggable CPUS. query_hotpluggable_cpus definitely > implies dr_cpu_enabled but dr_cpu_enabled can exist on its own > (theoretically at the least) without query_hotpluggable_cpus. > > So I think we should not replace dr_cpu_enabled with query_hotpluggable_cpus. I agree, hotplug capability shouldn't depend on availability of interface to operate it but rather on some platform specific bits which dr_cpu_enabled is (or at least looks like it's). > > However, I tested this patch and it works as intended. > > Regards, > Bharata. > >
On Mon, Aug 08, 2016 at 10:46:37AM +0200, Igor Mammedov wrote: > On Fri, 5 Aug 2016 20:21:59 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > On Fri, Aug 05, 2016 at 05:50:29PM +1000, David Gibson wrote: > > > Prior to c8721d3 "spapr: Error out when CPU hotplug is attempted on older > > > pseries machines", attempting to use query-hotpluggable-cpus on pseries-2.6 > > > and earlier machine types would SEGV. > > > > > > That change fixed that, but due to some unexpected interactions in init > > > order and a brown-paper-bag worthy failure to test, it accidentally > > > disabled query-hotpluggable-cpus for all pseries machine types, including > > > the current one which should allow it. > > > > > > In fact, query_hotpluggable_cpus needs to be non-NULL when and only when > > > the dr_cpu_enabled flag in sPAPRMachineClass is set, which makes > > > dr_cpu_enabled itself redundant. > > > > > > This patch removes dr_cpu_enabled, instead directly setting > > > query_hotpluggable_cpus from the machine class_init functions, and using > > > that to determine the availability of CPU hotplug when necessary. > > > > dr_cpu_enabled actually determines if CPU hotplug feature is present > > or not. It also controls the creation of DRC-specific properties > > in /cpus DT node like ibm,drc-indexes etc > > > > query_hotpluggable_cpus just tells us if the machine supports the > > querying of hotpluggable CPUS. query_hotpluggable_cpus definitely > > implies dr_cpu_enabled but dr_cpu_enabled can exist on its own > > (theoretically at the least) without query_hotpluggable_cpus. > > > > So I think we should not replace dr_cpu_enabled with query_hotpluggable_cpus. > I agree, hotplug capability shouldn't depend on availability of > interface to operate it but rather on some platform specific bits > which dr_cpu_enabled is (or at least looks like it's). I still don't understand the objection here. Once query_hotpluggable_cpus was set correctly it really was non-NULL if and only if dr_cpu_enabled was also true. Are you saying there should be an option to disable hotplug even in the newer machine types? For now, Peter has already merged my pull request including this patch.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index bce2371..57564e5 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -938,6 +938,7 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr, hwaddr rtas_size) { MachineState *machine = MACHINE(qdev_get_machine()); + MachineClass *mc = MACHINE_GET_CLASS(machine); sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine); const char *boot_device = machine->boot_order; int ret, i; @@ -1020,7 +1021,7 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr, _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB)); } - if (smc->dr_cpu_enabled) { + if (mc->query_hotpluggable_cpus) { int offset = fdt_path_offset(fdt, "/cpus"); ret = spapr_drc_populate_dt(fdt, offset, NULL, SPAPR_DR_CONNECTOR_TYPE_CPU); @@ -1712,6 +1713,7 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp) static void ppc_spapr_init(MachineState *machine) { sPAPRMachineState *spapr = SPAPR_MACHINE(machine); + MachineClass *mc = MACHINE_GET_CLASS(machine); sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine); const char *kernel_filename = machine->kernel_filename; const char *kernel_cmdline = machine->kernel_cmdline; @@ -1733,7 +1735,7 @@ static void ppc_spapr_init(MachineState *machine) int spapr_cores = smp_cpus / smp_threads; int spapr_max_cores = max_cpus / smp_threads; - if (smc->dr_cpu_enabled) { + if (mc->query_hotpluggable_cpus) { if (smp_cpus % smp_threads) { error_report("smp_cpus (%u) must be multiple of threads (%u)", smp_cpus, smp_threads); @@ -1810,7 +1812,7 @@ static void ppc_spapr_init(MachineState *machine) machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; } - if (smc->dr_cpu_enabled) { + if (mc->query_hotpluggable_cpus) { char *type = spapr_get_cpu_core_type(machine->cpu_model); spapr->cores = g_new0(Object *, spapr_max_cores); @@ -2333,12 +2335,12 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { error_setg(errp, "Memory hot unplug not supported by sPAPR"); } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { - if (!smc->dr_cpu_enabled) { + if (!mc->query_hotpluggable_cpus) { error_setg(errp, "CPU hot unplug not supported on this machine"); return; } @@ -2376,11 +2378,8 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine) int i; HotpluggableCPUList *head = NULL; sPAPRMachineState *spapr = SPAPR_MACHINE(machine); - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine); int spapr_max_cores = max_cpus / smp_threads; - g_assert(smc->dr_cpu_enabled); - for (i = 0; i < spapr_max_cores; i++) { HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1); HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1); @@ -2435,12 +2434,9 @@ 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; - if (smc->dr_cpu_enabled) { - mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus; - } smc->dr_lmb_enabled = true; - smc->dr_cpu_enabled = true; + mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus; fwc->get_dev_path = spapr_get_fw_dev_path; nc->nmi_monitor_handler = spapr_nmi; } @@ -2521,10 +2517,8 @@ static void spapr_machine_2_6_instance_options(MachineState *machine) static void spapr_machine_2_6_class_options(MachineClass *mc) { - sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc); - spapr_machine_2_7_class_options(mc); - smc->dr_cpu_enabled = false; + mc->query_hotpluggable_cpus = NULL; SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_6); } diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index 170ed15..704803a 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -153,7 +153,6 @@ void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev)); sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev)); sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev)); CPUCore *cc = CPU_CORE(dev); @@ -166,8 +165,6 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, int index = cc->core_id / smp_threads; int smt = kvmppc_smt_threads(); - g_assert(smc->dr_cpu_enabled); - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt); spapr->cores[index] = OBJECT(dev); @@ -209,7 +206,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { MachineState *machine = MACHINE(OBJECT(hotplug_dev)); - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev)); + MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev); sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev)); int spapr_max_cores = max_cpus / smp_threads; int index; @@ -218,7 +215,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model); const char *type = object_get_typename(OBJECT(dev)); - if (!smc->dr_cpu_enabled) { + if (!mc->query_hotpluggable_cpus) { error_setg(&local_err, "CPU hotplug not supported for this machine"); goto out; } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index bd8ac28..caf7be9 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -38,7 +38,6 @@ struct sPAPRMachineClass { /*< public >*/ bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */ - bool dr_cpu_enabled; /* enable dynamic-reconfig/hotplug of CPUs */ bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */ };
Prior to c8721d3 "spapr: Error out when CPU hotplug is attempted on older pseries machines", attempting to use query-hotpluggable-cpus on pseries-2.6 and earlier machine types would SEGV. That change fixed that, but due to some unexpected interactions in init order and a brown-paper-bag worthy failure to test, it accidentally disabled query-hotpluggable-cpus for all pseries machine types, including the current one which should allow it. In fact, query_hotpluggable_cpus needs to be non-NULL when and only when the dr_cpu_enabled flag in sPAPRMachineClass is set, which makes dr_cpu_enabled itself redundant. This patch removes dr_cpu_enabled, instead directly setting query_hotpluggable_cpus from the machine class_init functions, and using that to determine the availability of CPU hotplug when necessary. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 24 +++++++++--------------- hw/ppc/spapr_cpu_core.c | 7 ++----- include/hw/ppc/spapr.h | 1 - 3 files changed, 11 insertions(+), 21 deletions(-)