[libvirt] [PATCH v7 0/2] deprecate incorrect CPUs topolog
Changelog since v5: * add(v6) and then remove(v7) Notes section to/from deprication doc (Eduardo Habkost <ehabkost@redhat.com>) * fix up wording and math formating in deprication doc (Eduardo Habkost <ehabkost@redhat.com>) * drop !socket check as it always evaluates to true at that point (Eduardo Habkost <ehabkost@redhat.com>) Changelog since v4: * extend deprication doc, adding that maxcpus should be multiple of present on CLI [sockets/cores/threads] options (Eduardo Habkost <ehabkost@redhat.com>) series bundles together 2 related patches posted separately earlier: vl.c deprecate incorrect CPUs topology vl:c: make sure that sockets are calculated correctly in '-smp X' case Goal is to depricate invalid topologies so we could make sure that topology configuration is correct by forbidding invalid input once deprecation period ends. --- CC: libvir-list@redhat.com CC: ehabkost@redhat.com Igor Mammedov (2): vl.c deprecate incorrect CPUs topology vl:c: make sure that sockets are calculated correctly in '-smp X' case qemu-deprecated.texi | 13 +++++++++++++ vl.c | 12 +++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) -- 2.7.4
-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@redhat.com> Reviewed-by: Andrew Jones <drjones@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> --- v7: - add corrections to deprication doc formulas/math/section title - drop Note section (Eduardo Habkost <ehabkost@redhat.com>) v6: - s/socket/sockets/, the same for core, thread in subsection (Andrew Jones <drjones@redhat.com>) v5: - extend deprecation doc, adding that maxcpus should be multiple of present on CLI [sockets/cores/threads] options (Eduardo Habkost <ehabkost@redhat.com>) v4: - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>) v3: - more spelling fixes (Andrew Jones <drjones@redhat.com>) - place deprecation check after (sockets * cores * threads > max_cpus) check (Eduardo Habkost <ehabkost@redhat.com>) v2: - spelling&&co fixes (Andrew Jones <drjones@redhat.com>) --- qemu-deprecated.texi | 13 +++++++++++++ vl.c | 7 +++++++ 2 files changed, 20 insertions(+) diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi index 1b9c007..31dc1b3 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -139,6 +139,19 @@ 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 (invalid topologies) (since 3.1) + +CPU topology properties should describe whole machine topology including +possible CPUs. In other words: @var{maxcpus} should be equal to +@math{@var{sockets} * @var{cores} * @var{threads}}. + +However, historically it was possible to start QEMU with an incorrect topology +where @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. + @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}. + @section QEMU Machine Protocol (QMP) commands @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0) diff --git a/vl.c b/vl.c index 5ba06ad..7fd700e 100644 --- a/vl.c +++ b/vl.c @@ -1246,6 +1246,13 @@ static void smp_parse(QemuOpts *opts) exit(1); } + if (sockets * cores * threads != max_cpus) { + warn_report("Invalid CPU topology deprecated: " + "sockets (%u) * cores (%u) * threads (%u) " + "!= maxcpus (%u)", + sockets, cores, threads, max_cpus); + } + smp_cpus = cpus; smp_cores = cores; smp_threads = threads; -- 2.7.4
On 9/12/18 11:19 AM, 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@redhat.com> Reviewed-by: Andrew Jones <drjones@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> ---
+++ b/qemu-deprecated.texi @@ -139,6 +139,19 @@ 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 (invalid topologies) (since 3.1) + +CPU topology properties should describe whole machine topology including +possible CPUs. In other words: @var{maxcpus} should be equal to +@math{@var{sockets} * @var{cores} * @var{threads}}.
This sentence...
+ +However, historically it was possible to start QEMU with an incorrect topology +where @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. + @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
...and this recommendation sound repetitive. I'd be okay if you removed the "In other words" sentence above, leaving just the intro statement (what -cpu is supposed to do), then the second paragraph (what we also allowed in the past, and how you should fix it before we stop allowing that). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
commit (5cdc9b76e3 vl.c: Remove dead assignment) removed sockets calculation when 'sockets' weren't provided on CLI since there wasn't any users for it back then. Exiting checks are neither reachable } else if (sockets * cores * threads < cpus) { or nor triggable if (sockets * cores * threads > max_cpus) so we weren't noticing wrong topology since then, since users recalculate sockets adhoc on their own. However with deprecation check it becomes noticable, for example -smp 2 will start printing warning: "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" calculating sockets if they weren't specified. Fix it by returning back sockets calculation if it's omited on CLI. Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: Andrew Jones <drjones@redhat.com> --- vl.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/vl.c b/vl.c index 7fd700e..9e56696 100644 --- a/vl.c +++ b/vl.c @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts) /* compute missing values, prefer sockets over cores over threads */ if (cpus == 0 || sockets == 0) { - sockets = sockets > 0 ? sockets : 1; cores = cores > 0 ? cores : 1; threads = threads > 0 ? threads : 1; if (cpus == 0) { + sockets = sockets > 0 ? sockets : 1; cpus = cores * threads * sockets; + } else { + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); + sockets = max_cpus / (cores * threads); } } else if (cores == 0) { threads = threads > 0 ? threads : 1; -- 2.7.4
On 9/12/18 11:19 AM, Igor Mammedov wrote:
commit (5cdc9b76e3 vl.c: Remove dead assignment) removed sockets calculation when 'sockets' weren't provided on CLI since there wasn't any users for it back then. Exiting checks are neither reachable } else if (sockets * cores * threads < cpus) { or nor triggable
s/triggable/triggerable/
if (sockets * cores * threads > max_cpus) so we weren't noticing wrong topology since then, since users recalculate sockets adhoc on their own.
However with deprecation check it becomes noticable, for example -smp 2 will start printing warning: "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)" calculating sockets if they weren't specified.
Fix it by returning back sockets calculation if it's omited on CLI.
s/omited/omitted/
Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: Andrew Jones <drjones@redhat.com> --- vl.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c index 7fd700e..9e56696 100644 --- a/vl.c +++ b/vl.c @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
/* compute missing values, prefer sockets over cores over threads */ if (cpus == 0 || sockets == 0) { - sockets = sockets > 0 ? sockets : 1; cores = cores > 0 ? cores : 1; threads = threads > 0 ? threads : 1; if (cpus == 0) { + sockets = sockets > 0 ? sockets : 1; cpus = cores * threads * sockets; + } else { + max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); + sockets = max_cpus / (cores * threads); } } else if (cores == 0) { threads = threads > 0 ? threads : 1;
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Igor Mammedov