Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

[Zhao Liu <zhao1.liu@linux.intel.com>]

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