hw/core/machine.c | 30 ++++++++++++++++++++++-------- hw/i386/pc.c | 33 ++++++++++++++++++++++++--------- qapi/machine.json | 6 +++--- 3 files changed; 49 insertions(+); 20 deletions(-)
In the SMP configuration, we should either specify a topology parameter with a reasonable value (equal to or greater than 1) or just leave it omitted and QEMU will calculate its value. Configurations which explicitly specify the topology parameters as zero like "sockets=0" are meaningless, so disallow them. However; the commit 1e63fe685804d (machine: pass QAPI struct to mc->smp_parse) has documented that '0' has the same semantics as omitting a parameter in the qapi comment for SMPConfiguration. So this patch fixes the doc and also adds the corresponding sanity check in the smp parsers. This patch originly comes form [1], and it was suggested that this patch fixing the doc should be sent for 6.1 to avoid a deprecation process in the future. [1] https://lore.kernel.org/qemu-devel/YPWsThPiZa3mF+zp@redhat.com/ Yanan Wang (1): machine: Disallow specifying topology parameters as zero hw/core/machine.c | 30 ++++++++++++++++++++++-------- hw/i386/pc.c | 33 ++++++++++++++++++++++++--------- qapi/machine.json | 6 +++--- 3 files changed; 49 insertions(+); 20 deletions(-) -- 2.19.1
On Thu, Jul 22 2021, Yanan Wang <wangyanan55@huawei.com> wrote: > In the SMP configuration, we should either specify a topology > parameter with a reasonable value (equal to or greater than 1) > or just leave it omitted and QEMU will calculate its value. > Configurations which explicitly specify the topology parameters > as zero like "sockets=0" are meaningless, so disallow them. > > However; the commit 1e63fe685804d > (machine: pass QAPI struct to mc->smp_parse) has documented that > '0' has the same semantics as omitting a parameter in the qapi > comment for SMPConfiguration. So this patch fixes the doc and > also adds the corresponding sanity check in the smp parsers. Are we expecting any real users to have used that 'parameter=0' behaviour? I expect that libvirt and other management software already did the right thing; unfortunately, sometimes weird configuration lines tend to persist in search results. > > This patch originly comes form [1], and it was suggested that > this patch fixing the doc should be sent for 6.1 to avoid a > deprecation process in the future. > > [1] https://lore.kernel.org/qemu-devel/YPWsThPiZa3mF+zp@redhat.com/ > > Yanan Wang (1): > machine: Disallow specifying topology parameters as zero > > hw/core/machine.c | 30 ++++++++++++++++++++++-------- > hw/i386/pc.c | 33 ++++++++++++++++++++++++--------- > qapi/machine.json | 6 +++--- > 3 files changed; 49 insertions(+); 20 deletions(-)
On 2021/7/22 14:02, Cornelia Huck wrote: > On Thu, Jul 22 2021, Yanan Wang <wangyanan55@huawei.com> wrote: > >> In the SMP configuration, we should either specify a topology >> parameter with a reasonable value (equal to or greater than 1) >> or just leave it omitted and QEMU will calculate its value. >> Configurations which explicitly specify the topology parameters >> as zero like "sockets=0" are meaningless, so disallow them. >> >> However; the commit 1e63fe685804d >> (machine: pass QAPI struct to mc->smp_parse) has documented that >> '0' has the same semantics as omitting a parameter in the qapi >> comment for SMPConfiguration. So this patch fixes the doc and >> also adds the corresponding sanity check in the smp parsers. > Are we expecting any real users to have used that 'parameter=0' > behaviour? I expect that libvirt and other management software already > did the right thing; unfortunately, sometimes weird configuration lines > tend to persist in search results. I think there may not any users who have already used a configuration with explicit "parameter=0", instead it should have been just omitted. Yes, libvirt now rejects "parameter=0" when parsing XML, but we now still allows "parameter=0" in the direct QEMU cmdlines. If we hope to disallow this kind of config thoroughly, we'd better also have a sanity check in QEMU. Thanks, Yanan >> This patch originly comes form [1], and it was suggested that >> this patch fixing the doc should be sent for 6.1 to avoid a >> deprecation process in the future. >> >> [1] https://lore.kernel.org/qemu-devel/YPWsThPiZa3mF+zp@redhat.com/ >> >> Yanan Wang (1): >> machine: Disallow specifying topology parameters as zero >> >> hw/core/machine.c | 30 ++++++++++++++++++++++-------- >> hw/i386/pc.c | 33 ++++++++++++++++++++++++--------- >> qapi/machine.json | 6 +++--- >> 3 files changed; 49 insertions(+); 20 deletions(-) > .
On Thu, Jul 22, 2021 at 08:02:16AM +0200, Cornelia Huck wrote:
> On Thu, Jul 22 2021, Yanan Wang <wangyanan55@huawei.com> wrote:
>
> > In the SMP configuration, we should either specify a topology
> > parameter with a reasonable value (equal to or greater than 1)
> > or just leave it omitted and QEMU will calculate its value.
> > Configurations which explicitly specify the topology parameters
> > as zero like "sockets=0" are meaningless, so disallow them.
> >
> > However; the commit 1e63fe685804d
> > (machine: pass QAPI struct to mc->smp_parse) has documented that
> > '0' has the same semantics as omitting a parameter in the qapi
> > comment for SMPConfiguration. So this patch fixes the doc and
> > also adds the corresponding sanity check in the smp parsers.
>
> Are we expecting any real users to have used that 'parameter=0'
> behaviour? I expect that libvirt and other management software already
> did the right thing; unfortunately, sometimes weird configuration lines
> tend to persist in search results.
I understand this concern. I think the only documentation we had prior to
commit 1e63fe685804 was
DEF("smp", HAS_ARG, QEMU_OPTION_smp,
"-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,dies=dies][,sockets=sockets]\n"
" set the number of CPUs to 'n' [default=1]\n"
" maxcpus= maximum number of total cpus, including\n"
" offline CPUs for hotplug, etc\n"
" cores= number of CPU cores on one socket (for PC, it's on one die)\n"
" threads= number of threads on one CPU core\n"
" dies= number of CPU dies on one socket (for PC only)\n"
" sockets= number of discrete sockets in the system\n",
QEMU_ARCH_ALL)
SRST
``-smp [cpus=]n[,cores=cores][,threads=threads][,dies=dies][,sockets=sockets][,maxcpus=maxcpus]``
Simulate an SMP system with n CPUs. On the PC target, up to 255 CPUs
are supported. On Sparc32 target, Linux limits the number of usable
CPUs to 4. For the PC target, the number of cores per die, the
number of threads per cores, the number of dies per packages and the
total number of sockets can be specified. Missing values will be
computed. If any on the three values is given, the total number of
CPUs n can be omitted. maxcpus specifies the maximum number of
hotpluggable CPUs.
ERST
This doesn't mention zero inputs and even implies non-zero inputs.
I'm not sure if we need to worry about the odd command line that used zero
for some parameters. What do you think?
Thanks,
drew
>
> >
> > This patch originly comes form [1], and it was suggested that
> > this patch fixing the doc should be sent for 6.1 to avoid a
> > deprecation process in the future.
> >
> > [1] https://lore.kernel.org/qemu-devel/YPWsThPiZa3mF+zp@redhat.com/
> >
> > Yanan Wang (1):
> > machine: Disallow specifying topology parameters as zero
> >
> > hw/core/machine.c | 30 ++++++++++++++++++++++--------
> > hw/i386/pc.c | 33 ++++++++++++++++++++++++---------
> > qapi/machine.json | 6 +++---
> > 3 files changed; 49 insertions(+); 20 deletions(-)
>
On Thu, Jul 22 2021, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Jul 22, 2021 at 08:02:16AM +0200, Cornelia Huck wrote:
>> On Thu, Jul 22 2021, Yanan Wang <wangyanan55@huawei.com> wrote:
>>
>> > In the SMP configuration, we should either specify a topology
>> > parameter with a reasonable value (equal to or greater than 1)
>> > or just leave it omitted and QEMU will calculate its value.
>> > Configurations which explicitly specify the topology parameters
>> > as zero like "sockets=0" are meaningless, so disallow them.
>> >
>> > However; the commit 1e63fe685804d
>> > (machine: pass QAPI struct to mc->smp_parse) has documented that
>> > '0' has the same semantics as omitting a parameter in the qapi
>> > comment for SMPConfiguration. So this patch fixes the doc and
>> > also adds the corresponding sanity check in the smp parsers.
>>
>> Are we expecting any real users to have used that 'parameter=0'
>> behaviour? I expect that libvirt and other management software already
>> did the right thing; unfortunately, sometimes weird configuration lines
>> tend to persist in search results.
>
> I understand this concern. I think the only documentation we had prior to
> commit 1e63fe685804 was
>
> DEF("smp", HAS_ARG, QEMU_OPTION_smp,
> "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,dies=dies][,sockets=sockets]\n"
> " set the number of CPUs to 'n' [default=1]\n"
> " maxcpus= maximum number of total cpus, including\n"
> " offline CPUs for hotplug, etc\n"
> " cores= number of CPU cores on one socket (for PC, it's on one die)\n"
> " threads= number of threads on one CPU core\n"
> " dies= number of CPU dies on one socket (for PC only)\n"
> " sockets= number of discrete sockets in the system\n",
> QEMU_ARCH_ALL)
> SRST
> ``-smp [cpus=]n[,cores=cores][,threads=threads][,dies=dies][,sockets=sockets][,maxcpus=maxcpus]``
> Simulate an SMP system with n CPUs. On the PC target, up to 255 CPUs
> are supported. On Sparc32 target, Linux limits the number of usable
> CPUs to 4. For the PC target, the number of cores per die, the
> number of threads per cores, the number of dies per packages and the
> total number of sockets can be specified. Missing values will be
> computed. If any on the three values is given, the total number of
> CPUs n can be omitted. maxcpus specifies the maximum number of
> hotpluggable CPUs.
> ERST
>
> This doesn't mention zero inputs and even implies non-zero inputs.
Yes, hopefully that kept people away from using 0 magic, unless they
read the code.
>
> I'm not sure if we need to worry about the odd command line that used zero
> for some parameters. What do you think?
I did a cursory search for bad examples, and nothing popped up. So this
should be reasonably painless.
On 22/07/21 15:37, Andrew Jones wrote: > This doesn't mention zero inputs and even implies non-zero inputs. > > I'm not sure if we need to worry about the odd command line that used zero > for some parameters. What do you think? I think I agree as well, however the patch that Yanan sent has unnecessary duplication between smp_parse and pc_smp_parse. machine_set_smp is a better place to implement this kind of check. Paolo
On 2021/7/22 21:55, Paolo Bonzini wrote: > On 22/07/21 15:37, Andrew Jones wrote: >> This doesn't mention zero inputs and even implies non-zero inputs. >> >> I'm not sure if we need to worry about the odd command line that used >> zero >> for some parameters. What do you think? > > I think I agree as well, however the patch that Yanan sent has > unnecessary duplication between smp_parse and pc_smp_parse. > machine_set_smp is a better place to implement this kind of check. > The smp_parse and pc_smp_parse are going to be converted into a generic parser, and the added sanity-check in this patch will also be tested in an unit test. So is it probably better to keep the check in the parser instead of the caller? The duplication will be eliminated anyway when there is one single parser. But I can also implement the check in machine_set_smp as you mentioned if it's more reasonable and preferred. :) Thanks, Yanan .
On 22/07/21 16:12, wangyanan (Y) wrote: > The smp_parse and pc_smp_parse are going to be converted into a > generic parser, and the added sanity-check in this patch will also be > tested in an unit test. So is it probably better to keep the check in the > parser instead of the caller? The duplication will be eliminated anyway > when there is one single parser. > > But I can also implement the check in machine_set_smp as you mentioned > if it's more reasonable and preferred. :) Yes, I would prefer to avoid having duplicate code. There are some common checks already in machine_set_smp, e.g. comparing ms->smp.cpus against mc->min_cpus and mc->max_cpus. Paolo
On 2021/7/22 22:38, Paolo Bonzini wrote: > On 22/07/21 16:12, wangyanan (Y) wrote: >> The smp_parse and pc_smp_parse are going to be converted into a >> generic parser, and the added sanity-check in this patch will also be >> tested in an unit test. So is it probably better to keep the check in >> the >> parser instead of the caller? The duplication will be eliminated anyway >> when there is one single parser. >> >> But I can also implement the check in machine_set_smp as you mentioned >> if it's more reasonable and preferred. :) > > Yes, I would prefer to avoid having duplicate code. There are some > common checks already in machine_set_smp, e.g. comparing ms->smp.cpus > against mc->min_cpus and mc->max_cpus. > > Ok, I will send a v2. Thanks, Yanan .
© 2016 - 2024 Red Hat, Inc.