Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus

[Andrew Jones <drjones@redhat.com>]

On Thu, Jul 22, 2021 at 12:42:55PM +0800, wangyanan (Y) wrote:
> On 2021/7/20 0:42, Andrew Jones wrote:
> > On Mon, Jul 19, 2021 at 11:20:36AM +0800, Yanan Wang wrote:
> > > Currently we directly calculate the omitted cpus based on the already
> > > provided parameters. This makes some cmdlines like:
> > >    -smp maxcpus=16
> > >    -smp sockets=2,maxcpus=16
> > >    -smp sockets=2,dies=2,maxcpus=16
> > >    -smp sockets=2,cores=4,maxcpus=16
> > > not work. We should probably use the computed paramters to calculate
> > > cpus when maxcpus is provided while cpus is omitted, which will make
> > > above configs start to work.
> > > 
> > > Note: change in this patch won't affect any existing working cmdlines
> > > but allows more incomplete configs to be valid.
> > > 
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > ---
> > >   hw/core/machine.c | 17 +++++++++--------
> > >   1 file changed, 9 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index c9f15b15a5..668f0a1553 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -767,26 +767,27 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> > >           return;
> > >       }
> > > -    /* compute missing values, prefer sockets over cores over threads */
> > >       maxcpus = maxcpus > 0 ? maxcpus : cpus;
> > > -    if (cpus == 0) {
> > > -        sockets = sockets > 0 ? sockets : 1;
> > > -        cores = cores > 0 ? cores : 1;
> > > -        threads = threads > 0 ? threads : 1;
> > > -        cpus = sockets * dies * cores * threads;
> > > -        maxcpus = maxcpus > 0 ? maxcpus : cpus;
> > > -    } else if (sockets == 0) {
> > > +    /* compute missing values, prefer sockets over cores over threads */
> > > +    if (sockets == 0) {
> > >           cores = cores > 0 ? cores : 1;
> > >           threads = threads > 0 ? threads : 1;
> > >           sockets = maxcpus / (dies * cores * threads);
> > > +        sockets = sockets > 0 ? sockets : 1;
> > >       } else if (cores == 0) {
> > >           threads = threads > 0 ? threads : 1;
> > >           cores = maxcpus / (sockets * dies * threads);
> > > +        cores = cores > 0 ? cores : 1;
> > >       } else if (threads == 0) {
> > >           threads = maxcpus / (sockets * dies * cores);
> > > +        threads = threads > 0 ? threads : 1;
> > >       }
> > I didn't think we wanted this rounding which this patch adds back into
> > cores and threads and now also sockets.
> Firstly, I think we can agree that with or without the rounding, the invalid
> configs will still be reported as invalid. So the only difference is in the
> err
> message (either report 0 or 1 of a fractional parameter). :)

An error message that says the inputs produced a fractional parameter
would be even better. If the code gets too hairy because some parameters
may be zero and without additional checks we'd risk divide by zeros,
then maybe we should output ("fractional %s", param_name) and exit at the
same places we're currently rounding up.

> 
> I added back the rounding because this patch indeed need it, we start
> to use the computed parameters to calculate cpus, so we have to ensure
> that the computed parameters are at least 1. If both cpus and maxcpus
> are omitted (e.g. -smp sockets=16), without the rounding we will get
> zeroed cpus and maxcpus, and with the rounding we will get valid result
> like "cpus=16,sockets=16,cores=1,threads=1,maxcpus=16".
> 
> If a "0" or "1" in the error message doesn't make so much difference as
> assumed for the error reporting, I suggest that we probably can keep the
> rounding which makes the parser code concise.
> > > +    /* use the computed parameters to calculate the omitted cpus */
> > > +    cpus = cpus > 0 ? cpus : sockets * dies * cores * threads;
> > > +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> > It doesn't really matter, but I think I'd rather write this like
> > 
> >   maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
> >   cpus = cpus > 0 ? cpus : maxcpus;
> Yes, there is no functional difference. But I think maybe we'd better keep
> some consistence with the "maxcpus = maxcpus > 0 ? maxcpus : cpus"
> at top this function and also with the smp doc in qemu-option.hx i.e.
> "If omitted the maximum number of CPUs will be set to match the initial
> CPU count" ?

I won't argue it too much, but I think we should be thinking about maxcpus
more than cpus if we're thinking about cpu topologies. I'd rather have
users keep in mind that whatever their topology generates is the max and
if they don't want to expose that many cpus to the guest then they should
provide an additional, smaller number 'cpus'. To get that mindset we may
need to adjust the documentation.

Thanks,
drew