From: Andrew Jones <drjones@redhat.com>
To: Yanan Wang <wangyanan55@huawei.com>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
"Thomas Huth" <thuth@redhat.com>,
"Daniel P . Berrangé" <berrange@redhat.com>,
"Eduardo Habkost" <ehabkost@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
wanghaibin.wang@huawei.com,
"Richard Henderson" <richard.henderson@linaro.org>,
qemu-devel@nongnu.org, "Greg Kurz" <groug@kaod.org>,
"Halil Pasic" <pasic@linux.ibm.com>,
"Igor Mammedov" <imammedo@redhat.com>,
yuzenghui@huawei.com, "Paolo Bonzini" <pbonzini@redhat.com>,
"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [PATCH for-6.2 v2 02/11] machine: Make smp_parse generic enough for all arches
Date: Mon, 19 Jul 2021 18:28:46 +0200 [thread overview]
Message-ID: <20210719162846.dcltg22pbdpnyjcl@gator> (raw)
In-Reply-To: <20210719032043.25416-3-wangyanan55@huawei.com>
On Mon, Jul 19, 2021 at 11:20:34AM +0800, Yanan Wang wrote:
> Currently the only difference between smp_parse and pc_smp_parse
> is the support of multi-dies and the related error reporting code.
> With an arch compat variable "bool smp_dies_supported", we can
> easily make smp_parse generic enough for all arches and the PC
> specific one can be removed.
>
> Making smp_parse() generic enough can reduce code duplication and
> ease the code maintenance, and also allows extending the topology
> with more arch specific members (e.g., clusters) in the future.
>
> No functional change intended.
>
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
> hw/core/machine.c | 28 ++++++++++-------
> hw/i386/pc.c | 76 +--------------------------------------------
> include/hw/boards.h | 1 +
> 3 files changed, 19 insertions(+), 86 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d73daa10f4..ed6712e964 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -743,6 +743,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>
> static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> {
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
> unsigned cpus = config->has_cpus ? config->cpus : 0;
> unsigned sockets = config->has_sockets ? config->sockets : 0;
> unsigned dies = config->has_dies ? config->dies : 1;
> @@ -761,7 +762,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> return;
> }
>
> - if (dies > 1) {
> + if (!mc->smp_dies_supported && dies > 1) {
Won't this allow a user on an arch with !mc->smp_dies_supported to specify
dies=1? To not allow that, can we do
if (!mc->smp_dies_supported && config->has_dies)
instead?
> error_setg(errp, "dies not supported by this machine's CPU topology");
> return;
> }
> @@ -772,23 +773,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> threads = threads > 0 ? threads : 1;
> if (cpus == 0) {
> sockets = sockets > 0 ? sockets : 1;
> - cpus = cores * threads * sockets;
> + cpus = sockets * dies * cores * threads;
> } else {
> maxcpus = maxcpus > 0 ? maxcpus : cpus;
> - sockets = maxcpus / (cores * threads);
> + sockets = maxcpus / (dies * cores * threads);
> }
> } else if (cores == 0) {
> threads = threads > 0 ? threads : 1;
> - cores = cpus / (sockets * threads);
> + cores = cpus / (sockets * dies * threads);
> cores = cores > 0 ? cores : 1;
> } else if (threads == 0) {
> - threads = cpus / (cores * sockets);
> + threads = cpus / (sockets * dies * cores);
> threads = threads > 0 ? threads : 1;
> - } else if (sockets * cores * threads < cpus) {
> + } else if (sockets * dies * cores * threads < cpus) {
> + g_autofree char *dies_msg = g_strdup_printf(
> + mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> error_setg(errp, "cpu topology: "
> - "sockets (%u) * cores (%u) * threads (%u) < "
> + "sockets (%u)%s * cores (%u) * threads (%u) < "
> "smp_cpus (%u)",
> - sockets, cores, threads, cpus);
> + sockets, dies_msg, cores, threads, cpus);
> return;
> }
>
> @@ -799,17 +802,20 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> return;
> }
>
> - if (sockets * cores * threads != maxcpus) {
> + if (sockets * dies * cores * threads != maxcpus) {
> + g_autofree char *dies_msg = g_strdup_printf(
> + mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> error_setg(errp, "Invalid CPU topology: "
> - "sockets (%u) * cores (%u) * threads (%u) "
> + "sockets (%u)%s * cores (%u) * threads (%u) "
> "!= maxcpus (%u)",
> - sockets, cores, threads,
> + sockets, dies_msg, cores, threads,
> maxcpus);
> return;
> }
>
> ms->smp.cpus = cpus;
> ms->smp.sockets = sockets;
> + ms->smp.dies = dies;
> ms->smp.cores = cores;
> ms->smp.threads = threads;
> ms->smp.max_cpus = maxcpus;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c6b63c00a5..d94ef582b5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -708,80 +708,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> }
> }
>
> -/*
> - * This function is very similar to smp_parse()
> - * in hw/core/machine.c but includes CPU die support.
> - */
> -static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> -{
> - unsigned cpus = config->has_cpus ? config->cpus : 0;
> - unsigned sockets = config->has_sockets ? config->sockets : 0;
> - unsigned dies = config->has_dies ? config->dies : 1;
> - unsigned cores = config->has_cores ? config->cores : 0;
> - unsigned threads = config->has_threads ? config->threads : 0;
> - unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
> -
> - if ((config->has_cpus && config->cpus == 0) ||
> - (config->has_sockets && config->sockets == 0) ||
> - (config->has_dies && config->dies == 0) ||
> - (config->has_cores && config->cores == 0) ||
> - (config->has_threads && config->threads == 0) ||
> - (config->has_maxcpus && config->maxcpus == 0)) {
> - error_setg(errp, "parameters must be equal to or greater than one"
> - "if provided");
> - return;
> - }
> -
> - /* compute missing values, prefer sockets over cores over threads */
> - if (cpus == 0 || sockets == 0) {
> - cores = cores > 0 ? cores : 1;
> - threads = threads > 0 ? threads : 1;
> - if (cpus == 0) {
> - sockets = sockets > 0 ? sockets : 1;
> - cpus = cores * threads * dies * sockets;
> - } else {
> - maxcpus = maxcpus > 0 ? maxcpus : cpus;
> - sockets = maxcpus / (cores * threads * dies);
> - }
> - } else if (cores == 0) {
> - threads = threads > 0 ? threads : 1;
> - cores = cpus / (sockets * dies * threads);
> - cores = cores > 0 ? cores : 1;
> - } else if (threads == 0) {
> - threads = cpus / (cores * dies * sockets);
> - threads = threads > 0 ? threads : 1;
> - } else if (sockets * dies * cores * threads < cpus) {
> - error_setg(errp, "cpu topology: "
> - "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
> - "smp_cpus (%u)",
> - sockets, dies, cores, threads, cpus);
> - return;
> - }
> -
> - maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
> - if (maxcpus < cpus) {
> - error_setg(errp, "maxcpus must be equal to or greater than smp");
> - return;
> - }
> -
> - if (sockets * dies * cores * threads != maxcpus) {
> - error_setg(errp, "Invalid CPU topology deprecated: "
> - "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> - "!= maxcpus (%u)",
> - sockets, dies, cores, threads,
> - maxcpus);
> - return;
> - }
> -
> - ms->smp.cpus = cpus;
> - ms->smp.sockets = sockets;
> - ms->smp.dies = dies;
> - ms->smp.cores = cores;
> - ms->smp.threads = threads;
> - ms->smp.max_cpus = maxcpus;
> -}
> -
> static
> void pc_machine_done(Notifier *notifier, void *data)
> {
> @@ -1735,7 +1661,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> mc->auto_enable_numa_with_memdev = true;
> mc->has_hotpluggable_cpus = true;
> mc->default_boot_order = "cad";
> - mc->smp_parse = pc_smp_parse;
Can we also get rid of MachineClass::smp_parse now?
> mc->block_default_type = IF_IDE;
> mc->max_cpus = 255;
> mc->reset = pc_machine_reset;
> @@ -1746,6 +1671,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> hc->unplug = pc_machine_device_unplug_cb;
> mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> mc->nvdimm_supported = true;
> + mc->smp_dies_supported = true;
> mc->default_ram_id = "pc.ram";
>
> object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index accd6eff35..b6161cee88 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -246,6 +246,7 @@ struct MachineClass {
> bool smbus_no_migration_support;
> bool nvdimm_supported;
> bool numa_mem_supported;
> + bool smp_dies_supported;
> bool auto_enable_numa;
> const char *default_ram_id;
>
> --
> 2.19.1
>
Thanks,
drew
next prev parent reply other threads:[~2021-07-19 16:29 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-19 3:20 [PATCH for-6.2 v2 00/11] machine: smp parsing fixes and improvement Yanan Wang
2021-07-19 3:20 ` [PATCH for-6.2 v2 01/11] machine: Disallow specifying topology parameters as zero Yanan Wang
2021-07-19 16:11 ` Andrew Jones
2021-07-21 12:34 ` wangyanan (Y)
2021-07-19 16:46 ` Daniel P. Berrangé
2021-07-21 12:35 ` wangyanan (Y)
2021-07-19 3:20 ` [PATCH for-6.2 v2 02/11] machine: Make smp_parse generic enough for all arches Yanan Wang
2021-07-19 16:28 ` Andrew Jones [this message]
2021-07-19 16:36 ` Daniel P. Berrangé
2021-07-19 16:48 ` Andrew Jones
2021-07-19 16:50 ` Daniel P. Berrangé
2021-07-19 16:53 ` Daniel P. Berrangé
2021-07-22 7:18 ` wangyanan (Y)
2021-07-20 6:57 ` Cornelia Huck
2021-07-22 7:12 ` wangyanan (Y)
2021-07-19 3:20 ` [PATCH for-6.2 v2 03/11] machine: Uniformly use maxcpus to calculate the omitted parameters Yanan Wang
2021-07-19 16:36 ` Andrew Jones
2021-07-22 3:00 ` wangyanan (Y)
2021-07-19 3:20 ` [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus Yanan Wang
2021-07-19 16:42 ` Andrew Jones
2021-07-22 4:42 ` wangyanan (Y)
2021-07-22 12:27 ` Andrew Jones
2021-07-22 14:59 ` wangyanan (Y)
2021-07-22 15:05 ` Andrew Jones
2021-07-22 15:45 ` wangyanan (Y)
2021-07-19 3:20 ` [PATCH for-6.2 v2 05/11] machine: Improve the error reporting of smp parsing Yanan Wang
2021-07-19 16:53 ` Andrew Jones
2021-07-22 8:10 ` wangyanan (Y)
2021-07-22 12:47 ` Andrew Jones
2021-07-19 3:20 ` [PATCH for-6.2 v2 06/11] hw: Add compat machines for 6.2 Yanan Wang
2021-07-19 3:38 ` David Gibson
2021-07-19 17:00 ` Andrew Jones
2021-07-19 17:03 ` Cornelia Huck
2021-07-19 23:45 ` Pankaj Gupta
2021-07-19 3:20 ` [PATCH for-6.2 v2 07/11] machine: Prefer cores over sockets in smp parsing since 6.2 Yanan Wang
2021-07-19 3:40 ` David Gibson
2021-07-22 5:22 ` wangyanan (Y)
2021-07-19 17:13 ` Andrew Jones
2021-07-22 5:32 ` wangyanan (Y)
2021-07-19 3:20 ` [PATCH for-6.2 v2 08/11] machine: Use ms instead of global current_machine in sanity-check Yanan Wang
2021-07-19 17:14 ` Andrew Jones
2021-07-19 3:20 ` [PATCH for-6.2 v2 09/11] machine: Tweak the order of topology members in struct CpuTopology Yanan Wang
2021-07-19 3:20 ` [PATCH for-6.2 v2 10/11] machine: Split out the smp parsing code Yanan Wang
2021-07-19 17:20 ` Andrew Jones
2021-07-22 6:24 ` wangyanan (Y)
2021-07-22 13:07 ` Andrew Jones
2021-07-22 14:29 ` wangyanan (Y)
2021-07-19 3:20 ` [PATCH for-6.2 v2 11/11] tests/unit: Add a unit test for smp parsing Yanan Wang
2021-07-19 18:57 ` Andrew Jones
2021-07-22 6:15 ` wangyanan (Y)
2021-07-22 13:12 ` Andrew Jones
2021-07-22 14:18 ` wangyanan (Y)
2021-07-19 16:57 ` [PATCH for-6.2 v2 00/11] machine: smp parsing fixes and improvement Cornelia Huck
2021-07-21 12:38 ` wangyanan (Y)
2021-07-21 13:52 ` Pankaj Gupta
2021-07-22 2:22 ` wangyanan (Y)
2021-07-22 7:51 ` Pierre Morel
2021-07-22 8:32 ` wangyanan (Y)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210719162846.dcltg22pbdpnyjcl@gator \
--to=drjones@redhat.com \
--cc=berrange@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=ehabkost@redhat.com \
--cc=groug@kaod.org \
--cc=imammedo@redhat.com \
--cc=mst@redhat.com \
--cc=pasic@linux.ibm.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=thuth@redhat.com \
--cc=wanghaibin.wang@huawei.com \
--cc=wangyanan55@huawei.com \
--cc=yuzenghui@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).