[PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations
From: Zhao Liu <zhao1.liu@intel.com> The "parameter=0" SMP configurations have been marked as deprecated since v6.2. For these cases, -smp currently returns the warning and adjusts the zeroed parameters to 1 by default. Remove the above compatibility logic in v9.0, and return error directly if any -smp parameter is set as 0. Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- docs/about/deprecated.rst | 16 ---------------- docs/about/removed-features.rst | 15 +++++++++++++++ hw/core/machine-smp.c | 5 +++-- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 36bd3e15ef06..872974640252 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -36,22 +36,6 @@ and will cause a warning. The replacement for the ``nodelay`` short-form boolean option is ``nodelay=on`` rather than ``delay=off``. -``-smp`` ("parameter=0" SMP configurations) (since 6.2) -''''''''''''''''''''''''''''''''''''''''''''''''''''''' - -Specified CPU topology parameters must be greater than zero. - -In the SMP configuration, users should either provide a CPU topology -parameter with a reasonable value (greater than zero) or just omit it -and QEMU will compute the missing value. - -However, historically it was implicitly allowed for users to provide -a parameter with zero value, which is meaningless and could also possibly -cause unexpected results in the -smp parsing. So support for this kind of -configurations (e.g. -smp 8,sockets=0) is deprecated since 6.2 and will -be removed in the near future, users have to ensure that all the topology -members described with -smp are greater than zero. - Plugin argument passing through ``arg=<string>`` (since 6.1) '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index 417a0e4fa1d9..f9cf874f7b1f 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -489,6 +489,21 @@ The ``-singlestep`` option has been turned into an accelerator property, and given a name that better reflects what it actually does. Use ``-accel tcg,one-insn-per-tb=on`` instead. +``-smp`` ("parameter=0" SMP configurations) (removed in 9.0) +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Specified CPU topology parameters must be greater than zero. + +In the SMP configuration, users should either provide a CPU topology +parameter with a reasonable value (greater than zero) or just omit it +and QEMU will compute the missing value. + +However, historically it was implicitly allowed for users to provide +a parameter with zero value, which is meaningless and could also possibly +cause unexpected results in the -smp parsing. So support for this kind of +configurations (e.g. -smp 8,sockets=0) is removed since 9.0, users have +to ensure that all the topology members described with -smp are greater +than zero. User-mode emulator command line arguments ----------------------------------------- diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c index 25019c91ee36..96533886b14e 100644 --- a/hw/core/machine-smp.c +++ b/hw/core/machine-smp.c @@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms, (config->has_cores && config->cores == 0) || (config->has_threads && config->threads == 0) || (config->has_maxcpus && config->maxcpus == 0)) { - warn_report("Deprecated CPU topology (considered invalid): " - "CPU topology parameters must be greater than zero"); + error_setg(errp, "Invalid CPU topology: " + "CPU topology parameters must be greater than zero"); + return; } /* -- 2.34.1
On 04/03/2024 05.45, Zhao Liu wrote:
From: Zhao Liu <zhao1.liu@intel.com>
The "parameter=0" SMP configurations have been marked as deprecated since v6.2.
For these cases, -smp currently returns the warning and adjusts the zeroed parameters to 1 by default.
Remove the above compatibility logic in v9.0, and return error directly if any -smp parameter is set as 0.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- docs/about/deprecated.rst | 16 ---------------- docs/about/removed-features.rst | 15 +++++++++++++++ hw/core/machine-smp.c | 5 +++-- 3 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 36bd3e15ef06..872974640252 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -36,22 +36,6 @@ and will cause a warning. The replacement for the ``nodelay`` short-form boolean option is ``nodelay=on`` rather than ``delay=off``.
-``-smp`` ("parameter=0" SMP configurations) (since 6.2) -''''''''''''''''''''''''''''''''''''''''''''''''''''''' - -Specified CPU topology parameters must be greater than zero. - -In the SMP configuration, users should either provide a CPU topology -parameter with a reasonable value (greater than zero) or just omit it -and QEMU will compute the missing value. - -However, historically it was implicitly allowed for users to provide -a parameter with zero value, which is meaningless and could also possibly -cause unexpected results in the -smp parsing. So support for this kind of -configurations (e.g. -smp 8,sockets=0) is deprecated since 6.2 and will -be removed in the near future, users have to ensure that all the topology -members described with -smp are greater than zero. - Plugin argument passing through ``arg=<string>`` (since 6.1) ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index 417a0e4fa1d9..f9cf874f7b1f 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -489,6 +489,21 @@ The ``-singlestep`` option has been turned into an accelerator property, and given a name that better reflects what it actually does. Use ``-accel tcg,one-insn-per-tb=on`` instead.
+``-smp`` ("parameter=0" SMP configurations) (removed in 9.0) +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Specified CPU topology parameters must be greater than zero. + +In the SMP configuration, users should either provide a CPU topology +parameter with a reasonable value (greater than zero) or just omit it +and QEMU will compute the missing value. + +However, historically it was implicitly allowed for users to provide +a parameter with zero value, which is meaningless and could also possibly +cause unexpected results in the -smp parsing. So support for this kind of +configurations (e.g. -smp 8,sockets=0) is removed since 9.0, users have +to ensure that all the topology members described with -smp are greater +than zero.
User-mode emulator command line arguments ----------------------------------------- diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c index 25019c91ee36..96533886b14e 100644 --- a/hw/core/machine-smp.c +++ b/hw/core/machine-smp.c @@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms, (config->has_cores && config->cores == 0) || (config->has_threads && config->threads == 0) || (config->has_maxcpus && config->maxcpus == 0)) { - warn_report("Deprecated CPU topology (considered invalid): " - "CPU topology parameters must be greater than zero"); + error_setg(errp, "Invalid CPU topology: " + "CPU topology parameters must be greater than zero"); + return; }
Reviewed-by: Thomas Huth <thuth@redhat.com>
On Mon, 4 Mar 2024 at 10:02, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c index 25019c91ee36..96533886b14e 100644 --- a/hw/core/machine-smp.c +++ b/hw/core/machine-smp.c @@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms, (config->has_cores && config->cores == 0) || (config->has_threads && config->threads == 0) || (config->has_maxcpus && config->maxcpus == 0)) { - warn_report("Deprecated CPU topology (considered invalid): " - "CPU topology parameters must be greater than zero"); + error_setg(errp, "Invalid CPU topology: " + "CPU topology parameters must be greater than zero"); + return; }
unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; ... if (config->has_maxcpus && config->maxcpus == 0) * The check (has_maxcpus && maxcpus == 0) seems to be repeating above, maybe we could check if (maxcpus == 0) error_setg(). And same for other topology parameters? * Also a check to ensure cpus <= maxcpus is required I think. Thank you. --- - Prasad
Hi Prasad, On Mon, Mar 04, 2024 at 11:23:58AM +0530, Prasad Pandit wrote:
Date: Mon, 4 Mar 2024 11:23:58 +0530 From: Prasad Pandit <ppandit@redhat.com> Subject: Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations
On Mon, 4 Mar 2024 at 10:02, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c index 25019c91ee36..96533886b14e 100644 --- a/hw/core/machine-smp.c +++ b/hw/core/machine-smp.c @@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms, (config->has_cores && config->cores == 0) || (config->has_threads && config->threads == 0) || (config->has_maxcpus && config->maxcpus == 0)) { - warn_report("Deprecated CPU topology (considered invalid): " - "CPU topology parameters must be greater than zero"); + error_setg(errp, "Invalid CPU topology: " + "CPU topology parameters must be greater than zero"); + return; }
unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
This indicates the default maxcpus is initialized as 0 if user doesn't specifies it. For this case - no user configuration - maxcpus will be re-calculated as: maxcpus = maxcpus > 0 ? maxcpus : drawers * books * sockets * dies * clusters * cores * threads; (*)
... if (config->has_maxcpus && config->maxcpus == 0)
This check only wants to identify the case that user sets the 0.
* The check (has_maxcpus && maxcpus == 0) seems to be repeating above, maybe we could check if (maxcpus == 0) error_setg().
If the default maxcpus is initialized as 0, then (maxcpus == 0) will fail if user doesn't set maxcpus. However, we could initialize maxcpus as other default value, e.g., maxcpus = config->has_maxcpus ? config->maxcpus : 1. But it is still necessary to distinguish whether maxcpus is user-set or auto-initialized. If it is user-set, -smp should fail is there's invalid maxcpus/invalid topology. Otherwise, if it is auto-initialized, its value should be adjusted based on other topology components as the above calculation in (*).
And same for other topology parameters?
Other parameters also have the similar needs to distinguish if they're set by user. So the check needs to also cover has_* fields.
* Also a check to ensure cpus <= maxcpus is required I think.
Yes, the valid topology needs this. This code block already covers this case ;-): if (maxcpus < cpus) { g_autofree char *topo_msg = cpu_hierarchy_to_string(ms); error_setg(errp, "Invalid CPU topology: " "maxcpus must be equal to or greater than smp: " "%s == maxcpus (%u) < smp_cpus (%u)", topo_msg, maxcpus, cpus); return; } Thanks, Zhao
Hello Zhao, On Mon, 4 Mar 2024 at 12:19, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
This indicates the default maxcpus is initialized as 0 if user doesn't specifies it.
* 'has_maxcpus' should be set only if maxcpus > 0. If maxcpus == 0, then setting 'has_maxcpus=1' seems convoluted.
However, we could initialize maxcpus as other default value, e.g.,
maxcpus = config->has_maxcpus ? config->maxcpus : 1. === hw/core/machine.c machine_initfn /* default to mc->default_cpus */ ms->smp.cpus = mc->default_cpus; ms->smp.max_cpus = mc->default_cpus;
static void machine_class_base_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); mc->max_cpus = mc->max_cpus ?: 1; mc->min_cpus = mc->min_cpus ?: 1; mc->default_cpus = mc->default_cpus ?: 1; } === * Looking at the above bits, it seems smp.cpus & smp.max_cpus are initialised to 1 via default_cpus in MachineClass object.
if (config->has_maxcpus && config->maxcpus == 0) This check only wants to identify the case that user sets the 0. If the default maxcpus is initialized as 0, then (maxcpus == 0) will fail if user doesn't set maxcpus.
But it is still necessary to distinguish whether maxcpus is user-set or auto-initialized.
* If it is set to zero(0) either by user or by auto-initialise, it is still invalid, right?
If it is user-set, -smp should fail is there's invalid maxcpus/invalid topology.
Otherwise, if it is auto-initialized, its value should be adjusted based on other topology components as the above calculation in (*).
* Why have such diverging ways? * Could we simplify it as - If cpus/maxcpus==0, it is invalid, show an error and exit. - If cpus/maxcpus > 0, but incorrect for topology, then re-calculate the correct value based on topology parameters. If the re-calculated value is still incorrect or unsatisfactory, then show an error and exit. * Saying that user setting cpu/maxcpus=0 is invalid and auto-initialising it to zero(0) is valid, is not consistent. ...wdyt? --- - Prasad
Hi Prasad,
On Mon, 4 Mar 2024 at 12:19, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
This indicates the default maxcpus is initialized as 0 if user doesn't specifies it.
* 'has_maxcpus' should be set only if maxcpus > 0. If maxcpus == 0, then setting 'has_maxcpus=1' seems convoluted.
After simple test, if user sets maxcpus as 0, the has_maxcpus will be true as well...I think it's related with QAPI code generation logic.
However, we could initialize maxcpus as other default value, e.g.,
maxcpus = config->has_maxcpus ? config->maxcpus : 1. === hw/core/machine.c machine_initfn /* default to mc->default_cpus */ ms->smp.cpus = mc->default_cpus; ms->smp.max_cpus = mc->default_cpus;
static void machine_class_base_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); mc->max_cpus = mc->max_cpus ?: 1; mc->min_cpus = mc->min_cpus ?: 1; mc->default_cpus = mc->default_cpus ?: 1; } === * Looking at the above bits, it seems smp.cpus & smp.max_cpus are initialised to 1 via default_cpus in MachineClass object.
Yes. The maxcpus I mentioned is a local virable in machine_parse_smp_config(), whihc is used to do sanity-check check. In machine_parse_smp_config(), when we can confirm the topology is valid, then ms->smp.cpus and ms->smp.max_cpus are set with the valid virables (cpus and maxcpus).
if (config->has_maxcpus && config->maxcpus == 0) This check only wants to identify the case that user sets the 0. If the default maxcpus is initialized as 0, then (maxcpus == 0) will fail if user doesn't set maxcpus.
But it is still necessary to distinguish whether maxcpus is user-set or auto-initialized.
* If it is set to zero(0) either by user or by auto-initialise, it is still invalid, right?
The latter, "auto-initialise", means user could omit "cpus" and "maxcpus" parameters in -smp. Even though the local variable "cpus" and "maxcpus" are initialized as 0, eventually ms->smp.cpus and ms->smp.max_cpus will still have the valid values.
If it is user-set, -smp should fail is there's invalid maxcpus/invalid topology.
Otherwise, if it is auto-initialized, its value should be adjusted based on other topology components as the above calculation in (*).
* Why have such diverging ways? * Could we simplify it as - If cpus/maxcpus==0, it is invalid, show an error and exit.
Hmm, the origial behavior means if user doesn't set cpus=*/maxcpus=* in -smp, then QEMU will auto-complete these 2 fields. If we also return error for the above case that user omits cpus and maxcpus parameters, then this change the QEMU's API and we need to mark feature that the cpus/maxcpus parameter can be omitted as deprecated and remove it out. Just like what I did in this patch for zeroed-parameter case. I feel if there's no issue then it's not necessary to change the API. Do you agree?
- If cpus/maxcpus > 0, but incorrect for topology, then re-calculate the correct value based on topology parameters. If the re-calculated value is still incorrect or unsatisfactory, then show an error and exit.
Yes, this case is right.
* Saying that user setting cpu/maxcpus=0 is invalid and auto-initialising it to zero(0) is valid, is not consistent.
I think "auto-initialising it to zero(0)" doesn't means we re-initialize ms->smp.cpus and ms->smp.max_cpus as 0 (these 2 fields store actual basic topology information and they're defult as 1 as you said above). Does my explaination address your concern? ;-) Thanks, Zhao
Hi, On Tue, 5 Mar 2024 at 12:59, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
After simple test, if user sets maxcpus as 0, the has_maxcpus will be true as well...I think it's related with QAPI code generation logic.
* Right. [Maybe we digressed a bit in the discussion, so I snipped much of the details here. Sorry about that.] * "if user sets maxcpus as 0, the has_maxcpus will be true as well", ie if 'has_*' fields are always set unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0; then checking 'config->has_maxcpus ?' above is probably not required I think. It could just be maxcpus = config->maxcpus If a user does not specify config->maxcpus with -smp option, then it could default to zero(0) in 'config' parameter? (same for other config fields) * If such change requires API changes (I'm not sure how), then probably it is outside the scope of this patch. ...wdyt? Thank you. --- - Prasad
Hi Prasad,
On Tue, 5 Mar 2024 at 12:59, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
After simple test, if user sets maxcpus as 0, the has_maxcpus will be true as well...I think it's related with QAPI code generation logic.
* Right.
[Maybe we digressed a bit in the discussion, so I snipped much of the details here. Sorry about that.]
* "if user sets maxcpus as 0, the has_maxcpus will be true as well", ie if 'has_*' fields are always set
unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
then checking 'config->has_maxcpus ?' above is probably not required I think. It could just be
maxcpus = config->maxcpus
Yes.
If a user does not specify config->maxcpus with -smp option, then it could default to zero(0) in 'config' parameter? (same for other config fields)
Yes. I could post another series for this cleanup soon.
* If such change requires API changes (I'm not sure how), then probably it is outside the scope of this patch.
...wdyt?
The above change you suggested doesn't require API changes ;-). Thanks, Zhao
Hello Zhao, On Wed, 6 Mar 2024 at 08:49, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
then checking 'config->has_maxcpus ?' above is probably not required I think. It could just be
maxcpus = config->maxcpus
Yes.
If a user does not specify config->maxcpus with -smp option, then it could default to zero(0) in 'config' parameter? (same for other config fields)
Yes. I could post another series for this cleanup soon. The above change you suggested doesn't require API changes ;-).
* Great! (Communication is the most difficult skill to master. :)) * If you plan to send a separate patch for above refactoring, then I'd add Reviewed-by for this one. Reviewed-by: Prasad Pandit <pjp@fedoraproject.org> Thank you. --- - Prasad
On Wed, Mar 06, 2024 at 10:19:41AM +0530, Prasad Pandit wrote:
Date: Wed, 6 Mar 2024 10:19:41 +0530 From: Prasad Pandit <ppandit@redhat.com> Subject: Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations
Hello Zhao,
On Wed, 6 Mar 2024 at 08:49, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
then checking 'config->has_maxcpus ?' above is probably not required I think. It could just be
maxcpus = config->maxcpus
Yes.
If a user does not specify config->maxcpus with -smp option, then it could default to zero(0) in 'config' parameter? (same for other config fields)
Yes. I could post another series for this cleanup soon. The above change you suggested doesn't require API changes ;-).
* Great! (Communication is the most difficult skill to master. :))
* If you plan to send a separate patch for above refactoring, then I'd add Reviewed-by for this one.
Yeah, I will send a series, which will also include this patch, to avoid trivial smp cleanup fragmentation.
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Thanks! -Zhao
participants (3)
-
Prasad Pandit -
Thomas Huth -
Zhao Liu