Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Eduardo Habkost]

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?

These are just suggestions in case you are going to respin the series, not
blockers.

-- 
Eduardo