On Tue, 4 Sep 2018 23:12:55 -0300
Eduardo Habkost <ehabkost@xxxxxxxxxx> 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 <imammedo@xxxxxxxxxx>
> > ---
> > v5:
> > - extend deprecation doc, adding that maxcpus should be multiple of
> > present on CLI [sockets/cores/threads] options
> > (Eduardo Habkost <ehabkost@xxxxxxxxxx>)
> > v4:
> > - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@xxxxxxxxxx>)
> > v3:
> > - more spelling fixes (Andrew Jones <drjones@xxxxxxxxxx>)
> > - place deprecation check after (sockets * cores * threads > max_cpus) check
> > (Eduardo Habkost <ehabkost@xxxxxxxxxx>)
> > v2:
> > - spelling&&co fixes (Andrew Jones <drjones@xxxxxxxxxx>)
> > ---
> > 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.
> >
> > +@subsection -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:
>
> @math{@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
>
> Minor: same as above. I suggest:
>
> @math{@var{sockets} * @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.
>
> These are just suggestions in case you are going to respin the series, not
> blockers.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
