[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology |
Date: |
Wed, 12 Sep 2018 18:15:37 +0200 |
On Mon, 10 Sep 2018 14:49:23 -0300
Eduardo Habkost <address@hidden> wrote:
> On Thu, Sep 06, 2018 at 10:02:13AM +0200, Igor Mammedov wrote:
> > On Wed, 5 Sep 2018 10:45:12 -0300
> > Eduardo Habkost <address@hidden> wrote:
> >
> > > On Wed, Sep 05, 2018 at 11:25:11AM +0200, Igor Mammedov wrote:
> > > > On Tue, 4 Sep 2018 23:12:55 -0300
> > > > Eduardo Habkost <address@hidden> wrote:
> > > >
> > > > > On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:
> > > > > > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > > > > > so that total number of logical CPUs [sockets * cores * threads]
> > > > > > would be equal to [maxcpus], however historically we didn't have
> > > > > > such check in QEMU and it is possible to start VM with an invalid
> > > > > > topology.
> > > > > > Deprecate invalid options combination so we can make sure that
> > > > > > the topology VM started with is always correct in the future.
> > > > > > Users with an invalid sockets/cores/threads/maxcpus values should
> > > > > > fix their CLI to make sure that
> > > > > > [sockets * cores * threads] == [maxcpus]
> > > > > >
> > > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > > ---
> > > > > > v5:
> > > > > > - extend deprecation doc, adding that maxcpus should be multiple
> > > > > > of
> > > > > > present on CLI [sockets/cores/threads] options
> > > > > > (Eduardo Habkost <address@hidden>)
> > > > > > v4:
> > > > > > - missed dot comment, fix it with s/./,/ (Andrew Jones
> > > > > > <address@hidden>)
> > > > > > v3:
> > > > > > - more spelling fixes (Andrew Jones <address@hidden>)
> > > > > > - place deprecation check after (sockets * cores * threads >
> > > > > > max_cpus) check
> > > > > > (Eduardo Habkost <address@hidden>)
> > > > > > v2:
> > > > > > - spelling&&co fixes (Andrew Jones <address@hidden>)
> > > > > > ---
> > > > > > qemu-deprecated.texi | 19 +++++++++++++++++++
> > > > > > vl.c | 7 +++++++
> > > > > > 2 files changed, 26 insertions(+)
> > > > > >
> > > > > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > > > > > index 87212b6..827c3ce 100644
> > > > > > --- a/qemu-deprecated.texi
> > > > > > +++ b/qemu-deprecated.texi
> > > > > > @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer
> > > > > > appropriate for character or host
> > > > > > devices and will only accept regular files (S_IFREG). The correct
> > > > > > driver
> > > > > > for these file types is 'host_cdrom' or 'host_device' as
> > > > > > appropriate.
> > > > > >
> > > > > > address@hidden -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since
> > > > > > 3.1)
> > > > >
> > > > > Minor: I suggest using @var markup, or maybe just use
> > > > > "-smp (invalid topologies) (since 3.1)" as subsection title for
> > > > > simplicity?
> > > > >
> > > > > > +
> > > > > > +CPU topology properties should describe whole machine topology
> > > > > > including
> > > > > > +possible CPUs, but historically it was possible to start QEMU with
> > > > > > +an incorrect topology where
> > > > > > + sockets * cores * threads >= X && X < maxcpus
> > > > >
> > > > > Minor: this line formatting is lost on the HTML output. I
> > > > > suggest using @var and/or @math.
> > > > >
> > > > > Minor: I suggest not using C syntax.
> > > > >
> > > > > i.e. use something like:
> > > > >
> > > > > @address@hidden <= @var{sockets} * @var{cores} * @var{threads} <
> > > > > @var{maxcpus}},
> > > > >
> > > > > > +which could lead to an incorrect topology enumeration by the guest.
> > > > > > +Support for invalid topologies will be removed, the user must
> > > > > > ensure
> > > > > > +topologies described with -smp include all possible cpus, i.e.
> > > > > > + sockets * cores * threads == maxcpus
> > > > >
> > > > > Minor: same as above. I suggest:
> > > > >
> > > > > @address@hidden * @var{cores} * @var{threads} = @var{maxcpus}}.
> > > > >
> > > > >
> > > > > > +Note: it's assumed that maxcpus value must be multiple of the
> > > > > > topology
> > > > > > +options present on command line to avoid creating an invalid
> > > > > > topology.
> > > > > > +If maxcpus isn't be multiple of present topology options then the
> > > > > > condition
> > > > > > +(sockets * cores * threads == maxcpus) can't be satisfied and it
> > > > > > will
> > > > > > +trigger deprecation warning which later will be converted to a
> > > > > > error.
> > > > > > +If you get deprecation warning it's recommended to explicitly
> > > > > > specify
> > > > > > +a correct topology to make warning go away and ensure that it will
> > > > > > +continue working in the future.
> > > > >
> > > > > I don't understand the purpose of the "Note:" section. It seems to
> > > > > duplicate
> > > > > what was already said in the lines above it. Can we just remove it?
> > > > >
> > > > Well, didn't I say that no additional explanation needed in v4?
> > > > Note section was added per your suggestion to explicitly say that
> > > > maxcpus
> > > > should be multiple of options present on CLI.
> > >
> > > You are right: I did ask for additional clarification on the
> > > documentation, because it was not clear that the warning can
> > > still appear even if some of the sockets/cores/threads options
> > > were omitted. The note didn't make that clearer to me, though.
> > I can respin if necessary (we are not in hurry so no need to merge
> > something that would be removed immediately).
> > Could you suggest a text for clarification for me to include on respin?
>
> I was planning to write a suggestion later, but I didn't want
> this series to be delayed because of me. Just removing the
> existing "Notes:" section would be good enough to me.
>
> Probably the right place to document the subtle details of the
> s/c/t/maxcpus requirements is the "-smp" section on
> qemu-options.hx, not qemu-deprecated.hx.
>
> I was considering something like this:
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 060e015be6..74f6a64b8b 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -139,16 +139,20 @@ The 'file' driver for drives is no longer appropriate
> for character or host
> devices and will only accept regular files (S_IFREG). The correct driver
> for these file types is 'host_cdrom' or 'host_device' as appropriate.
>
> address@hidden -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
> address@hidden -smp (invalid topologies) (since 3.1)
>
> CPU topology properties should describe whole machine topology including
> -possible CPUs, but historically it was possible to start QEMU with
> +possible CPUs. In other words: @var{maxcpus} should be equal to
> address@hidden@var{sockets} * @var{cores} * @var{threads}}.
> +
> +However, historically it was possible to start QEMU with
> an incorrect topology where
> - sockets * cores * threads >= X && X < maxcpus
> address@hidden@var{n} <= @var{sockets} * @var{cores} * @var{threads} <
> @var{maxcpus}},
> which could lead to an incorrect topology enumeration by the guest.
> Support for invalid topologies will be removed, the user must ensure
> topologies described with -smp include all possible cpus, i.e.
> - sockets * cores * threads == maxcpus
> address@hidden@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
> +
Thanks,
suggestions taken in account and I'll drop Note section
> @section QEMU Machine Protocol (QMP) commands
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 654ef484d9..2afa271570 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -155,8 +155,13 @@ to 4.
> For the PC target, the number of @var{cores} per socket, the number
> of @var{threads} per cores and the total number of @var{sockets} can be
> specified. Missing values will be computed. If any on the three values is
> -given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
> -specifies the maximum number of hotpluggable CPUs.
> +given, the total number of CPUs @var{n} can be omitted.
> +
> address@hidden specifies the maximum number of hotpluggable CPUs
> +and should be equal to @address@hidden * @var{cores} * @var{threads}}
> +(even if @var{sockets}, @var{cores}, or @var{threads} is omitted
> +and computed automatically).
Options doc I've left out, it probably should be separate patch.
> ETEXI
>
> DEF("numa", HAS_ARG, QEMU_OPTION_numa,
>
- Re: [Qemu-devel] [PATCH v5 2/2] vl:c: make sure that sockets are calculated correctly in '-smp X' case, (continued)
- [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Igor Mammedov, 2018/09/04
- Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Andrew Jones, 2018/09/04
- [Qemu-devel] [PATCH v6 1/2] vl.c deprecate incorrect CPUs topology, Igor Mammedov, 2018/09/04
- Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Eduardo Habkost, 2018/09/04
- Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Igor Mammedov, 2018/09/05
- Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Eduardo Habkost, 2018/09/05
- Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Igor Mammedov, 2018/09/06
- Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Eduardo Habkost, 2018/09/10
- Re: [Qemu-devel] [libvirt] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Eric Blake, 2018/09/10
- Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology,
Igor Mammedov <=
Re: [Qemu-devel] [libvirt] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology, Eric Blake, 2018/09/10