Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus
On 2021/7/22 23:05, Andrew Jones wrote:
On Thu, Jul 22, 2021 at 10:59:11PM +0800, wangyanan (Y) wrote:
Ok. If we remove the rounding, then the calculation code has to be modified
to be like the following. We have to separately consider the case that cpus
and maxcpus are both omitted (e.g. -smp sockets=16).
maxcpus = maxcpus > 0 ? maxcpus : cpus;
if (cpus == 0 && maxcpus == 0) {
sockets = sockets > 0 ? sockets : 1;
cores = cores > 0 ? cores : 1;
threads = threads > 0 ? threads : 1;
goto cal;
}
if (sockets == 0) {
...
} else if (cores == 0) {
...
} else if (threads == 0) {
...
}
cal:
maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
cpus = cpus > 0 ? cpus : maxcpus;
Whatever works, but hopefully you can avoid an ugly goto.
Well, it can be avoided.
Thanks,
Yanan
Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus
On Thu, Jul 22, 2021 at 10:59:11PM +0800, wangyanan (Y) wrote:
> Ok. If we remove the rounding, then the calculation code has to be modified
> to be like the following. We have to separately consider the case that cpus
> and maxcpus are both omitted (e.g. -smp sockets=16).
>
> maxcpus = maxcpus > 0 ? maxcpus : cpus;
>
> if (cpus == 0 && maxcpus == 0) {
> sockets = sockets > 0 ? sockets : 1;
> cores = cores > 0 ? cores : 1;
> threads = threads > 0 ? threads : 1;
> goto cal;
> }
>
> if (sockets == 0) {
> ...
> } else if (cores == 0) {
> ...
> } else if (threads == 0) {
> ...
> }
>
> cal:
> maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
> cpus = cpus > 0 ? cpus : maxcpus;
Whatever works, but hopefully you can avoid an ugly goto.
Thanks,
drew
Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus
On 2021/7/22 20:27, Andrew Jones wrote:
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
---
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.
Ok. If we remove the rounding, then the calculation code has to be modified
to be like the following. We have to separately consider the case that cpus
and maxcpus are both omitted (e.g. -smp sockets=16).
maxcpus = maxcpus > 0 ? maxcpus : cpus;
if (cpus == 0 && maxcpus == 0) {
sockets = sockets > 0 ? sockets : 1;
cores = cores > 0 ? cores : 1;
threads = threads > 0 ? threads : 1;
goto cal;
}
if (sockets == 0) {
...
} else if (cores == 0) {
...
} else if (threads == 0) {
...
}
cal:
maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
cpus = cpus > 0 ? cpus : maxcpus;
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
Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus
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
> > > ---
> > > 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
Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus
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
---
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). :)
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" ?
Thanks,
Yanan
.
+
if (sockets * dies * cores * threads < cpus) {
g_autofree char *dies_msg = g_strdup_printf(
mc->smp_dies_supported ? " * dies (%u)" : "", dies);
--
2.19.1
Thanks,
drew
.
Re: [PATCH for-6.2 v2 04/11] machine: Use the computed parameters to calculate omitted cpus
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
> ---
> 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.
>
> +/* 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;
> +
> if (sockets * dies * cores * threads < cpus) {
> g_autofree char *dies_msg = g_strdup_printf(
> mc->smp_dies_supported ? " * dies (%u)" : "", dies);
> --
> 2.19.1
>
Thanks,
drew
