hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

Commit Message

Zhao Liu March 4, 2024, 4:45 a.m. UTC

From: Zhao Liu <[e-mail]>

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 <[e-mail]>
---
 docs/about/deprecated.rst       | 16 ----------------
 docs/about/removed-features.rst | 15 +++++++++++++++
 hw/core/machine-smp.c           |  5 +++--
 3 files changed, 18 insertions(+), 18 deletions(-)

Comments

Thomas Huth March 4, 2024, 5:50 a.m. UTC | #1

On 04/03/2024 05.45, Zhao Liu wrote:
> From: Zhao Liu <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   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 <[e-mail]>

Prasad Pandit March 4, 2024, 5:53 a.m. UTC | #2

On Mon, 4 Mar 2024 at 10:02, Zhao Liu <[e-mail]> 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

Zhao Liu March 4, 2024, 7:03 a.m. UTC | #3

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 <[e-mail]>
> Subject: Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0"
>  SMP configurations
> 
> On Mon, 4 Mar 2024 at 10:02, Zhao Liu <[e-mail]> 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

Prasad Pandit March 4, 2024, 8:21 a.m. UTC | #4

Hello Zhao,

On Mon, 4 Mar 2024 at 12:19, Zhao Liu <[e-mail]> 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

Zhao Liu March 5, 2024, 7:42 a.m. UTC | #5

Hi Prasad,

> On Mon, 4 Mar 2024 at 12:19, Zhao Liu <[e-mail]> 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

Prasad Pandit March 5, 2024, 12:37 p.m. UTC | #6

Hi,

On Tue, 5 Mar 2024 at 12:59, Zhao Liu <[e-mail]> 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

Zhao Liu March 6, 2024, 3:33 a.m. UTC | #7

Hi Prasad,

> On Tue, 5 Mar 2024 at 12:59, Zhao Liu <[e-mail]> 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

Prasad Pandit March 6, 2024, 4:49 a.m. UTC | #8

Hello Zhao,

On Wed, 6 Mar 2024 at 08:49, Zhao Liu <[e-mail]> 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 <[e-mail]>

Thank you.
---
  - Prasad

Zhao Liu March 6, 2024, 6:27 a.m. UTC | #9

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 <[e-mail]>
> 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 <[e-mail]> 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 <[e-mail]>

Thanks!

-Zhao

Patch

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;
     }
 
     /*