[RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Yanan Wang]

Hi,

This is v4 of the series [1] that I posted to introduce support for
generating cpu topology descriptions to guest. Comments are welcome!

Description:
Once the view of an accurate virtual cpu topology is provided to guest,
with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
e.g., the scheduling performance improvement. See Dario Faggioli's
research and the related performance tests in [2] for reference. So here
we go, this patch series introduces cpu topology support for ARM platform.

In this series, instead of quietly enforcing the support for the latest
machine type, a new parameter "expose=on|off" in -smp command line is
introduced to leave QEMU users a choice to decide whether to enable the
feature or not. This will allow the feature to work on different machine
types and also ideally compat with already in-use -smp command lines.
Also we make much stricter requirement for the topology configuration
with "expose=on".

Furthermore, both cpu-map in DT and ACPI PPTT table are introduced to
present cpu topology to the guest. And an ARM-specific -smp parsing
function virt_smp_parse is introduced, which shares the same logic
with smp_parse() when "expose=off" and follow the stricter parsing
rule when "expose=on".

[1] https://patchwork.kernel.org/project/qemu-devel/cover/20210516102900.28036-1-wangyanan55@huawei.com/
[2] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines-friend-or-foe-dario-faggioli-suse

Test results about exposure of topology:
After applying this patch series, launch an ACPI guest with virt-6.1 on an ARM server.

1) Enable the support:
With cmdline: -smp 96,sockets=2,cores=48,threads=1,expose=on
  or cmdline: -smp 96,maxcpus=96,sockets=2,cores=48,threads=1,expose=on
we get:
Architecture:        aarch64
Byte Order:          Little Endian
CPU(s):              96
On-line CPU(s) list: 0-95
Thread(s) per core:  1
Core(s) per socket:  48
Socket(s):           2
NUMA node(s):        1
Vendor ID:           0x48
Model:               0
Stepping:            0x1
BogoMIPS:            200.00
NUMA node0 CPU(s):   0-95

2) Disable the support:
With cmdline: -smp 96
  or cmdline: -smp 96,expose=off
we get:
Architecture:        aarch64
Byte Order:          Little Endian
CPU(s):              96
On-line CPU(s) list: 0-95
Thread(s) per core:  1
Core(s) per socket:  96
Socket(s):           1
NUMA node(s):        1
Vendor ID:           0x48
Model:               0
Stepping:            0x1
BogoMIPS:            200.00
NUMA node0 CPU(s):   0-95

---

Changelogs:

v3->v4:
- add new -smp parameter "expose=on|off" for users to enable/disable the feature
- add stricter -smp cmdline parsing rules on "expose=on" case
- move build_pptt to generic aml-build.c
- add default cluster node in the cpu-map
- rebase on top of latest upstream master
- v3: https://patchwork.kernel.org/project/qemu-devel/cover/20210516102900.28036-1-wangyanan55@huawei.com/

v2->v3:
- address comments from David, Philippe, and Andrew. Thanks!
- split some change into separate commits for ease of review
- adjust parsing rules of virt_smp_parse to be more strict
  (after discussion with Andrew)
- adjust author credit for the patches
- v2: https://patchwork.kernel.org/project/qemu-devel/cover/20210413080745.33004-1-wangyanan55@huawei.com/

v1->v2:
- Address Andrew Jones's comments
- Address Michael S. Tsirkin's comments
- v1: https://patchwork.kernel.org/project/qemu-devel/cover/20210225085627.2263-1-fangying1@huawei.com/

---

Andrew Jones (2):
  hw/arm/virt: Add cpu-map to device tree
  hw/acpi/aml-build: Generate PPTT table

Yanan Wang (5):
  vl: Add expose=on|off option support in -smp command line
  hw/arm/virt: Add separate -smp parsing function for ARM machines
  machine: disallow -smp expose=on for non-ARM machines
  device_tree: Add qemu_fdt_add_path
  hw/acpi/aml-build: Add Processor hierarchy node structure

 hw/acpi/aml-build.c          |  75 +++++++++++++++
 hw/arm/virt-acpi-build.c     |   8 +-
 hw/arm/virt.c                | 171 +++++++++++++++++++++++++++++++++--
 hw/core/machine.c            |   7 ++
 hw/i386/pc.c                 |   7 ++
 include/hw/acpi/aml-build.h  |   7 ++
 include/hw/boards.h          |   1 +
 include/sysemu/device_tree.h |   1 +
 qemu-options.hx              |  24 +++--
 softmmu/device_tree.c        |  44 ++++++++-
 softmmu/vl.c                 |   3 +
 11 files changed, 326 insertions(+), 22 deletions(-)

-- 
2.23.0

[RFC PATCH v4 1/7] vl: Add expose=on|off option support in -smp command line

[Yanan Wang]

Once the view of virtual cpu topology is provided to guest kernel,
with a well-designed vCPU pinning to the pCPU we could get a huge
benefit, e.g., the scheduling performance improvement. However a
virtual cpu topology view of guest may also have a negative impact
if the pinning is badly-designed. See Dario Faggioli's research
and the related performance tests in [1] for reference.

[1] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-
for-virtual-machines-friend-or-foe-dario-faggioli-suse

So here we go, let's introduce support of generating cpu topology
descriptions to the guest. However, instead of quietly enforcing
the support for the latest machine type, we'd better introduce a
new parameter "expose=on|off" in -smp command line to leave QEMU
users a choice to decide whether to enable the feature or not.
This will allow the feature to work on different machine types
and also ideally compat with already in-use -smp command lines.

Furthermore, based on existing parsing rules of -smp command line
in generic smp_parse() which allows to compute the missing values,
another more strict rule is introduced to follow when exposure of
cpu topology is enabled. With "expose=on", it's important to know
what users actually want, so we require that all of cpus/sockets/
cores/threads must be provided while maxcpus is optional.
Hopefully the new rule will apply to all kinds of architectures
which support the feature.

In conclusion, if a QEMU user doesn't hope to enable the virtual
cpu topology support, then feel free to configure a -smp cmdline
like below and everything will work just like before:
-smp 96
-smp 96,expose=off
-smp 96,sockets=2
-smp 96,sockets=2,expose=off
...

While if a QEMU user is ready to take advantage of the virtual cpu
topology support, then he must configure an accurate -smp cmdline
like below, on different machine types:
-smp 96,sockets=2,cores=48,threads=1,expose=on
-smp 96,maxcpus=96,sockets=2,cores=48,threads=1,expose=on

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 qemu-options.hx | 24 +++++++++++++++---------
 softmmu/vl.c    |  3 +++
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 14258784b3..d18d64958b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -196,25 +196,31 @@ SRST
 ERST
 
 DEF("smp", HAS_ARG, QEMU_OPTION_smp,
-    "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,dies=dies][,sockets=sockets]\n"
+    "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,dies=dies][,sockets=sockets][,expose=on|off]\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",
+    "                sockets= number of discrete sockets in the system\n"
+    "                expose=on|off controls support for exposing cpu topology\n"
+    "                to the guest (default=off)\n",
         QEMU_ARCH_ALL)
 SRST
-``-smp [cpus=]n[,cores=cores][,threads=threads][,dies=dies][,sockets=sockets][,maxcpus=maxcpus]``
+``-smp [cpus=]n[,cores=cores][,threads=threads][,dies=dies][,sockets=sockets][,maxcpus=maxcpus][,expose=on|off]``
     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
+    are supported. On the 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 core, the number of dies per package and the total number
+    of sockets can be specified. maxcpus specifies the maximum number of
     hotpluggable CPUs.
+
+    With "expose=off" or not explicitly specified, missing values will be
+    computed, and the total number of CPUs n can be omitted if any on the
+    three values is given. Otherwise with "expose=on", much more detailed
+    configuration is required: cpus/sockets/cores/threads must be given,
+    while maxcpus is optional.
 ERST
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
diff --git a/softmmu/vl.c b/softmmu/vl.c
index feb4d201f3..f4b59571c7 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -729,6 +729,9 @@ static QemuOptsList qemu_smp_opts = {
         }, {
             .name = "maxcpus",
             .type = QEMU_OPT_NUMBER,
+        }, {
+            .name = "expose",
+            .type = QEMU_OPT_BOOL,
         },
         { /*End of list */ }
     },
-- 
2.23.0

[RFC PATCH v4 2/7] hw/arm/virt: Add separate -smp parsing function for ARM machines

[Yanan Wang]

Add an ARM-specific -smp parsing function named virt_smp_parse.
When support for exposing cpu topology to the guest is disabled,
this function shares the same logic with smp_parse(). While when
the support is enabled, an accurate virtual cpu topology is very
important, so we require that cpus/sockets/cores/threads must be
given, while maxcpus is optional.

Given that ARM cpu hotplug is not supported yet, a configuration
with "maxcpus > cpus" is meaningless, so if exposure is turned on
we also explicitly enforce that maxcpus must be equal to smp cpus
if specified, until cpu hotplug is available.

Given that the virtual cpu topology exposure support may come
to different architectures in the future, a new structure member
"bool expose_cpu_topology" is also added to MachineState to
record whether the feature is supported & enabled.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c       | 113 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/boards.h |   1 +
 2 files changed, 114 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4b96f06014..f29d796f3f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -77,6 +77,8 @@
 #include "hw/virtio/virtio-iommu.h"
 #include "hw/char/pl011.h"
 #include "qemu/guest-random.h"
+#include "qapi/qmp/qerror.h"
+#include "sysemu/replay.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -2584,6 +2586,116 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
     return fixed_ipa ? 0 : requested_pa_size;
 }
 
+/*
+ * virt_smp_parse - Parses the -smp command line for ARM machines.
+ *
+ * When support for exposing cpu topology to the guest is disabled,
+ * this function shares the same parsing logic with smp_parse().
+ * While when the exposure is enabled, an accurate virtual cpu topology
+ * is important, so we required that cpus/sockets/cores/threads must be
+ * provided, while maxcpus can be optional.
+ */
+static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
+{
+    if (opts) {
+        unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
+        unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
+        unsigned cores = qemu_opt_get_number(opts, "cores", 0);
+        unsigned threads = qemu_opt_get_number(opts, "threads", 0);
+        unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0);
+        bool expose = qemu_opt_get_bool(opts, "expose", false);
+
+        if (expose) {
+            if (cpus == 0) {
+                error_report("expose=on: cpus must be specified");
+                exit(1);
+            }
+            if (sockets == 0) {
+                error_report("expose=on: sockets must be specified");
+                exit(1);
+            }
+            if (cores == 0) {
+                error_report("expose=on: cores must be specified");
+                exit(1);
+            }
+            if (threads == 0) {
+                error_report("expose=on: threads must be specified");
+                exit(1);
+            }
+        } else {
+            /*
+             * An accurate cpu topology configuration is not required in this
+             * case, so we compute the missing values preferring sockets over
+             * cores over threads.
+             */
+            if (cpus == 0 || sockets == 0) {
+                cores = cores > 0 ? cores : 1;
+                threads = threads > 0 ? threads : 1;
+                if (cpus == 0) {
+                    sockets = sockets > 0 ? sockets : 1;
+                    cpus = cores * threads * sockets;
+                } else {
+                    maxcpus = maxcpus > 0 ? maxcpus : cpus;
+                    sockets = maxcpus / (cores * threads);
+                }
+            } else if (cores == 0) {
+                threads = threads > 0 ? threads : 1;
+                cores = cpus / (sockets * threads);
+                cores = cores > 0 ? cores : 1;
+            } else if (threads == 0) {
+                threads = cpus / (cores * sockets);
+                threads = threads > 0 ? threads : 1;
+            }
+        }
+
+        maxcpus = maxcpus > 0 ? maxcpus : cpus;
+
+        if (sockets * cores * threads < cpus) {
+            error_report("cpu topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) < "
+                         "smp_cpus (%u)",
+                         sockets, cores, threads, cpus);
+            exit(1);
+        }
+
+        if (maxcpus < cpus) {
+            error_report("maxcpus must be equal to or greater than smp");
+            exit(1);
+        }
+
+        if (sockets * cores * threads != maxcpus) {
+            error_report("Invalid CPU topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) "
+                         "!= maxcpus (%u)",
+                         sockets, cores, threads, maxcpus);
+            exit(1);
+        }
+
+        /*
+         * Given that cpu hotplug is not supported yet, require that maxcpus
+         * must be equal to smp cpus if we are going to expose cpu topology
+         * to the guest.
+         */
+        if (expose == true && maxcpus != cpus) {
+            error_report("cpu hotplug is not supported yet, maxcpus must be"
+                         "equal to smp");
+        }
+
+        ms->smp.cpus = cpus;
+        ms->smp.sockets = sockets;
+        ms->smp.cores = cores;
+        ms->smp.threads = threads;
+        ms->smp.max_cpus = maxcpus;
+        ms->expose_cpu_topology = expose;
+    }
+
+    if (ms->smp.cpus > 1) {
+        Error *blocker = NULL;
+        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
+        replay_add_blocker(blocker);
+    }
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -2611,6 +2723,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+    mc->smp_parse = virt_smp_parse;
     mc->kvm_type = virt_kvm_type;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 3d55d2bd62..54accc810a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -332,6 +332,7 @@ struct MachineState {
     AccelState *accelerator;
     CPUArchIdList *possible_cpus;
     CpuTopology smp;
+    bool expose_cpu_topology;
     struct NVDIMMState *nvdimms_state;
     struct NumaState *numa_state;
 };
-- 
2.23.0

[RFC PATCH v4 3/7] machine: disallow -smp expose=on for non-ARM machines

[Yanan Wang]

Currently, support for exposure of cpu topology to the guest
is only introduced for ARM machines and we also have an ARM
specific parsing function virt_smp_parse(), so we disallow
the "-smp expose=on" configuration for the other platforms.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 7 +++++++
 hw/i386/pc.c      | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 55b9bc7817..23721bb77e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -746,6 +746,13 @@ static void smp_parse(MachineState *ms, QemuOpts *opts)
         unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
         unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
         unsigned threads = qemu_opt_get_number(opts, "threads", 0);
+        bool expose      = qemu_opt_get_bool(opts, "expose", false);
+
+        if (expose) {
+            error_report("expose=on: exposing cpu topology to the guest"
+                         "is not supported yet");
+            exit(1);
+        }
 
         /* compute missing values, prefer sockets over cores over threads */
         if (cpus == 0 || sockets == 0) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c6d8d0d84d..afabfa0566 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -720,6 +720,13 @@ void pc_smp_parse(MachineState *ms, QemuOpts *opts)
         unsigned dies = qemu_opt_get_number(opts, "dies", 1);
         unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
         unsigned threads = qemu_opt_get_number(opts, "threads", 0);
+        bool expose      = qemu_opt_get_bool(opts, "expose", false);
+
+        if (expose) {
+            error_report("expose=on: exposing cpu topology to the guest"
+                         "is not supported yet");
+            exit(1);
+        }
 
         /* compute missing values, prefer sockets over cores over threads */
         if (cpus == 0 || sockets == 0) {
-- 
2.23.0

[RFC PATCH v4 4/7] device_tree: Add qemu_fdt_add_path

[Yanan Wang]

qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except it
also adds all missing subnodes from the given path. We'll use it
in a coming patch where we will add cpu-map to the device tree.

And we also tweak an error message of qemu_fdt_add_subnode().

Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alistair Francis <alistair.francis@wdc.com>
Co-developed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 include/sysemu/device_tree.h |  1 +
 softmmu/device_tree.c        | 44 ++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 8a2fe55622..ef060a9759 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -121,6 +121,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
 uint32_t qemu_fdt_alloc_phandle(void *fdt);
 int qemu_fdt_nop_node(void *fdt, const char *node_path);
 int qemu_fdt_add_subnode(void *fdt, const char *name);
+int qemu_fdt_add_path(void *fdt, const char *path);
 
 #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
     do {                                                                      \
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index b621f63fba..3965c834ca 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -540,8 +540,8 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
 
     retval = fdt_add_subnode(fdt, parent, basename);
     if (retval < 0) {
-        error_report("FDT: Failed to create subnode %s: %s", name,
-                     fdt_strerror(retval));
+        error_report("%s: Failed to create subnode %s: %s",
+                     __func__, name, fdt_strerror(retval));
         exit(1);
     }
 
@@ -549,6 +549,46 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
     return retval;
 }
 
+/*
+ * qemu_fdt_add_path: Like qemu_fdt_add_subnode(), but will add
+ * all missing subnodes from the given path.
+ */
+int qemu_fdt_add_path(void *fdt, const char *path)
+{
+    const char *name;
+    const char *p = path;
+    int namelen, retval;
+    int parent = 0;
+
+    if (path[0] != '/') {
+        return -1;
+    }
+
+    while (p) {
+        name = p + 1;
+        p = strchr(name, '/');
+        namelen = p != NULL ? p - name : strlen(name);
+
+        retval = fdt_subnode_offset_namelen(fdt, parent, name, namelen);
+        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
+            error_report("%s: Unexpected error in finding subnode %.*s: %s",
+                         __func__, namelen, name, fdt_strerror(retval));
+            exit(1);
+        } else if (retval == -FDT_ERR_NOTFOUND) {
+            retval = fdt_add_subnode_namelen(fdt, parent, name, namelen);
+            if (retval < 0) {
+                error_report("%s: Failed to create subnode %.*s: %s",
+                             __func__, namelen, name, fdt_strerror(retval));
+                exit(1);
+            }
+        }
+
+        parent = retval;
+    }
+
+    return retval;
+}
+
 void qemu_fdt_dumpdtb(void *fdt, int size)
 {
     const char *dumpdtb = current_machine->dumpdtb;
-- 
2.23.0

[RFC PATCH v4 5/7] hw/arm/virt: Add cpu-map to device tree

[Yanan Wang]

From: Andrew Jones <drjones@redhat.com>

Support device tree CPU topology descriptions.

In accordance with the Devicetree Specification, the Linux Doc
"arm/cpus.yaml" requires that cpus and cpu nodes in the DT are
present. And we have already met the requirement by generating
/cpus/cpu@* nodes for members within ms->smp.cpus. Accordingly,
we should also create subnodes in cpu-map for the present cpus,
each of which relates to an unique cpu node.

The Linux Doc "cpu/cpu-topology.txt" states that the hierarchy
of CPUs in a SMP system is defined through four entities and
they are socket/cluster/core/thread. It is also required that
a socket node's child nodes must be one or more cluster nodes.
Given that currently we are only provided with information of
socket/core/thread, we assume there is one cluster child node
in each socket node when creating cpu-map.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c | 58 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f29d796f3f..645ce7d260 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -355,17 +355,17 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
     int smp_cpus = ms->smp.cpus;
 
     /*
-     * From Documentation/devicetree/bindings/arm/cpus.txt
-     *  On ARM v8 64-bit systems value should be set to 2,
-     *  that corresponds to the MPIDR_EL1 register size.
-     *  If MPIDR_EL1[63:32] value is equal to 0 on all CPUs
-     *  in the system, #address-cells can be set to 1, since
-     *  MPIDR_EL1[63:32] bits are not used for CPUs
-     *  identification.
+     * See Linux Documentation/devicetree/bindings/arm/cpus.yaml
+     * On ARM v8 64-bit systems value should be set to 2,
+     * that corresponds to the MPIDR_EL1 register size.
+     * If MPIDR_EL1[63:32] value is equal to 0 on all CPUs
+     * in the system, #address-cells can be set to 1, since
+     * MPIDR_EL1[63:32] bits are not used for CPUs
+     * identification.
      *
-     *  Here we actually don't know whether our system is 32- or 64-bit one.
-     *  The simplest way to go is to examine affinity IDs of all our CPUs. If
-     *  at least one of them has Aff3 populated, we set #address-cells to 2.
+     * Here we actually don't know whether our system is 32- or 64-bit one.
+     * The simplest way to go is to examine affinity IDs of all our CPUs. If
+     * at least one of them has Aff3 populated, we set #address-cells to 2.
      */
     for (cpu = 0; cpu < smp_cpus; cpu++) {
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
@@ -408,8 +408,46 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
+        if (ms->expose_cpu_topology) {
+            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
+                                  qemu_fdt_alloc_phandle(ms->fdt));
+        }
+
         g_free(nodename);
     }
+
+    if (ms->expose_cpu_topology) {
+        /*
+         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
+         * In a SMP system, the hierarchy of CPUs is defined through four
+         * entities that are used to describe the layout of physical CPUs
+         * in the system: socket/cluster/core/thread.
+         */
+        qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map");
+
+        for (cpu = smp_cpus - 1; cpu >= 0; cpu--) {
+            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
+            char *map_path;
+
+            if (ms->smp.threads > 1) {
+                map_path = g_strdup_printf(
+                    "/cpus/cpu-map/%s%d/cluster0/%s%d/%s%d",
+                    "socket", cpu / (ms->smp.cores * ms->smp.threads),
+                    "core", (cpu / ms->smp.threads) % ms->smp.cores,
+                    "thread", cpu % ms->smp.threads);
+            } else {
+                map_path = g_strdup_printf(
+                    "/cpus/cpu-map/%s%d/cluster0/%s%d",
+                    "socket", cpu / ms->smp.cores,
+                    "core", cpu % ms->smp.cores);
+            }
+            qemu_fdt_add_path(ms->fdt, map_path);
+            qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path);
+
+            g_free(map_path);
+            g_free(cpu_path);
+        }
+    }
 }
 
 static void fdt_add_its_gic_node(VirtMachineState *vms)
-- 
2.23.0

[RFC PATCH v4 6/7] hw/acpi/aml-build: Add Processor hierarchy node structure

[Yanan Wang]

Add a generic API to build Processor hierarchy node structure (Type 0),
which is strictly consistent with descriptions in ACPI 6.2: 5.2.29.1.

This function will be used to build ACPI PPTT table for cpu topology.

Co-developed-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
Co-developed-by: Henglong Fan <fanhenglong@huawei.com>
Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |  4 ++++
 2 files changed, 30 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index d5103e6d7b..9fa5024414 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1920,6 +1920,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                  table_data->len - slit_start, 1, oem_id, oem_table_id);
 }
 
+/* ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0) */
+void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
+                                    uint32_t parent, uint32_t id,
+                                    uint32_t *priv_rsrc, uint32_t priv_num)
+{
+    int i;
+
+    build_append_byte(tbl, 0);                 /* Type 0 - processor */
+    build_append_byte(tbl, 20 + priv_num * 4); /* Length */
+    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
+    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
+    build_append_int_noprefix(tbl, parent, 4); /* Parent */
+    build_append_int_noprefix(tbl, id, 4);     /* ACPI Processor ID */
+
+    /* Number of private resources */
+    build_append_int_noprefix(tbl, priv_num, 4);
+
+    /* Private resources[N] */
+    if (priv_num > 0) {
+        assert(priv_rsrc);
+        for (i = 0; i < priv_num; i++) {
+            build_append_int_noprefix(tbl, priv_rsrc[i], 4);
+        }
+    }
+}
+
 /* build rev1/rev3/rev5.1 FADT */
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id)
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 471266d739..ea74b8f6ed 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -462,6 +462,10 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                 const char *oem_id, const char *oem_table_id);
 
+void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
+                                    uint32_t parent, uint32_t id,
+                                    uint32_t *priv_rsrc, uint32_t priv_num);
+
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id);
 
-- 
2.23.0

[RFC PATCH v4 7/7] hw/acpi/aml-build: Generate PPTT table

[Yanan Wang]

From: Andrew Jones <drjones@redhat.com>

Add the Processor Properties Topology Table (PPTT) to expose
CPU topology information defined by users to ACPI guests.

Note, a DT-boot Linux guest with a non-flat CPU topology will
see socket and core IDs being sequential integers starting
from zero, which is different from ACPI-boot Linux guest,
e.g. with -smp 4,sockets=2,cores=2,threads=1

a DT boot produces:

 cpu:  0 package_id:  0 core_id:  0
 cpu:  1 package_id:  0 core_id:  1
 cpu:  2 package_id:  1 core_id:  0
 cpu:  3 package_id:  1 core_id:  1

an ACPI boot produces:

 cpu:  0 package_id: 36 core_id:  0
 cpu:  1 package_id: 36 core_id:  1
 cpu:  2 package_id: 96 core_id:  2
 cpu:  3 package_id: 96 core_id:  3

This is due to several reasons:

 1) DT cpu nodes do not have an equivalent field to what the PPTT
    ACPI Processor ID must be, i.e. something equal to the MADT CPU
    UID or equal to the UID of an ACPI processor container. In both
    ACPI cases those are platform dependant IDs assigned by the
    vendor.

 2) While QEMU is the vendor for a guest, if the topology specifies
    SMT (> 1 thread), then, with ACPI, it is impossible to assign a
    core-id the same value as a package-id, thus it is not possible
    to have package-id=0 and core-id=0. This is because package and
    core containers must be in the same ACPI namespace and therefore
    must have unique UIDs.

 3) ACPI processor containers are not mandatorily required for PPTT
    tables to be used and, due to the limitations of which IDs are
    selected described above in (2), they are not helpful for QEMU,
    so we don't build them with this patch. In the absence of them,
    Linux assigns its own unique IDs. The maintainers have chosen not
    to use counters from zero, but rather ACPI table offsets, which
    explains why the numbers are so much larger than with DT.

 4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
    match the logical CPU IDs, because these IDs must be equal to the
    MADT CPU UID (as no processor containers are present), and QEMU
    uses the logical CPU ID for these MADT IDs.

So in summary, with QEMU as vender for the guest, we use sequential
integers starting from zero for non-leaf nodes without valid ID flag,
so that guest will ignore them and use table offsets as unique IDs.
And we use logical CPU IDs for leaf nodes to be consistent with MADT.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/acpi/aml-build.c         | 49 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    |  8 +++++-
 include/hw/acpi/aml-build.h |  3 +++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 9fa5024414..a92c7d8123 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1946,6 +1946,55 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
     }
 }
 
+/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */
+void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
+                const char *oem_id, const char *oem_table_id)
+{
+    int pptt_start = table_data->len;
+    int uid = 0, socket;
+
+    acpi_data_push(table_data, sizeof(AcpiTableHeader));
+
+    for (socket = 0; socket < ms->smp.sockets; socket++) {
+        uint32_t socket_offset = table_data->len - pptt_start;
+        int core;
+
+        build_processor_hierarchy_node(
+            table_data,
+            (1 << 0), /* ACPI 6.2 - Physical package */
+            0, socket, NULL, 0);
+
+        for (core = 0; core < ms->smp.cores; core++) {
+            uint32_t core_offset = table_data->len - pptt_start;
+            int thread;
+
+            if (ms->smp.threads == 1) {
+                build_processor_hierarchy_node(
+                    table_data,
+                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
+                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
+                    socket_offset, uid++, NULL, 0);
+            } else {
+                build_processor_hierarchy_node(table_data, 0, socket_offset,
+                                               core, NULL, 0);
+
+                for (thread = 0; thread < ms->smp.threads; thread++) {
+                    build_processor_hierarchy_node(
+                        table_data,
+                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
+                        (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
+                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
+                        core_offset, uid++, NULL, 0);
+                }
+            }
+        }
+    }
+
+    build_header(linker, table_data,
+                 (void *)(table_data->data + pptt_start), "PPTT",
+                 table_data->len - pptt_start, 2, oem_id, oem_table_id);
+}
+
 /* build rev1/rev3/rev5.1 FADT */
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f1024843dd..90d2176b35 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -698,13 +698,19 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     dsdt = tables_blob->len;
     build_dsdt(tables_blob, tables->linker, vms);
 
-    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
+    /* FADT MADT PPTT GTDT MCFG SPCR pointed to by RSDT */
     acpi_add_table(table_offsets, tables_blob);
     build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
 
     acpi_add_table(table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, vms);
 
+    if (ms->expose_cpu_topology) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_pptt(tables_blob, tables->linker, ms, vms->oem_id,
+                   vms->oem_table_id);
+    }
+
     acpi_add_table(table_offsets, tables_blob);
     build_gtdt(tables_blob, tables->linker, vms);
 
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index ea74b8f6ed..6c29f853cd 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -466,6 +466,9 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
                                     uint32_t parent, uint32_t id,
                                     uint32_t *priv_rsrc, uint32_t priv_num);
 
+void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
+                const char *oem_id, const char *oem_table_id);
+
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id);
 
-- 
2.23.0

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Daniel P. Berrangé]

On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> Hi,
> 
> This is v4 of the series [1] that I posted to introduce support for
> generating cpu topology descriptions to guest. Comments are welcome!
> 
> Description:
> Once the view of an accurate virtual cpu topology is provided to guest,
> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> e.g., the scheduling performance improvement. See Dario Faggioli's
> research and the related performance tests in [2] for reference. So here
> we go, this patch series introduces cpu topology support for ARM platform.
> 
> In this series, instead of quietly enforcing the support for the latest
> machine type, a new parameter "expose=on|off" in -smp command line is
> introduced to leave QEMU users a choice to decide whether to enable the
> feature or not. This will allow the feature to work on different machine
> types and also ideally compat with already in-use -smp command lines.
> Also we make much stricter requirement for the topology configuration
> with "expose=on".

Seeing this 'expose=on' parameter feels to me like we're adding a
"make-it-work=yes" parameter. IMHO this is just something that should
be done by default for the current machine type version and beyond.
I don't see the need for a parameter to turnthis on, especially since
it is being made architecture specific.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> > Hi,
> > 
> > This is v4 of the series [1] that I posted to introduce support for
> > generating cpu topology descriptions to guest. Comments are welcome!
> > 
> > Description:
> > Once the view of an accurate virtual cpu topology is provided to guest,
> > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > e.g., the scheduling performance improvement. See Dario Faggioli's
> > research and the related performance tests in [2] for reference. So here
> > we go, this patch series introduces cpu topology support for ARM platform.
> > 
> > In this series, instead of quietly enforcing the support for the latest
> > machine type, a new parameter "expose=on|off" in -smp command line is
> > introduced to leave QEMU users a choice to decide whether to enable the
> > feature or not. This will allow the feature to work on different machine
> > types and also ideally compat with already in-use -smp command lines.
> > Also we make much stricter requirement for the topology configuration
> > with "expose=on".
> 
> Seeing this 'expose=on' parameter feels to me like we're adding a
> "make-it-work=yes" parameter. IMHO this is just something that should
> be done by default for the current machine type version and beyond.
> I don't see the need for a parameter to turnthis on, especially since
> it is being made architecture specific.
>

I agree.

Yanan, we never discussed an "expose" parameter in the previous versions
of this series. We discussed a "strict" parameter though, which would
allow existing command lines to "work" using assumptions of what the user
meant and strict=on users to get what they mean or an error saying that
they asked for something that won't work or would require unreasonable
assumptions. Why was this changed to an "expose" parameter?

Thanks,
drew

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

On 2021/6/22 19:46, Andrew Jones wrote:
> On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
>> On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
>>> Hi,
>>>
>>> This is v4 of the series [1] that I posted to introduce support for
>>> generating cpu topology descriptions to guest. Comments are welcome!
>>>
>>> Description:
>>> Once the view of an accurate virtual cpu topology is provided to guest,
>>> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
>>> e.g., the scheduling performance improvement. See Dario Faggioli's
>>> research and the related performance tests in [2] for reference. So here
>>> we go, this patch series introduces cpu topology support for ARM platform.
>>>
>>> In this series, instead of quietly enforcing the support for the latest
>>> machine type, a new parameter "expose=on|off" in -smp command line is
>>> introduced to leave QEMU users a choice to decide whether to enable the
>>> feature or not. This will allow the feature to work on different machine
>>> types and also ideally compat with already in-use -smp command lines.
>>> Also we make much stricter requirement for the topology configuration
>>> with "expose=on".
>> Seeing this 'expose=on' parameter feels to me like we're adding a
>> "make-it-work=yes" parameter. IMHO this is just something that should
>> be done by default for the current machine type version and beyond.
>> I don't see the need for a parameter to turnthis on, especially since
>> it is being made architecture specific.
>>
> I agree.
>
> Yanan, we never discussed an "expose" parameter in the previous versions
> of this series. We discussed a "strict" parameter though, which would
> allow existing command lines to "work" using assumptions of what the user
> meant and strict=on users to get what they mean or an error saying that
> they asked for something that won't work or would require unreasonable
> assumptions. Why was this changed to an "expose" parameter?
Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 
[1] of this series.
[1] 
https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/

And in the discussion, we hoped things would work like below with 
"strict" parameter:
Users who want to describe cpu topology should provide cmdline like

-smp strict=on,cpus=4,sockets=2,cores=2,threads=1

and in this case we require an more accurate -smp configuration and
then generate the cpu topology description through ACPI/DT.

While without a strict description, no cpu topology description would
be generated, so they get nothing through ACPI/DT.

It seems to me that the "strict" parameter actually serves as a knob to
turn on/off the exposure of topology, and this is the reason I changed
the name.

Thanks,
Yanan
.
> Thanks,
> drew
>
> .

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Daniel P. Berrangé]

On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
> 
> 
> On 2021/6/22 19:46, Andrew Jones wrote:
> > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> > > > Hi,
> > > > 
> > > > This is v4 of the series [1] that I posted to introduce support for
> > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > 
> > > > Description:
> > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > research and the related performance tests in [2] for reference. So here
> > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > 
> > > > In this series, instead of quietly enforcing the support for the latest
> > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > feature or not. This will allow the feature to work on different machine
> > > > types and also ideally compat with already in-use -smp command lines.
> > > > Also we make much stricter requirement for the topology configuration
> > > > with "expose=on".
> > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > be done by default for the current machine type version and beyond.
> > > I don't see the need for a parameter to turnthis on, especially since
> > > it is being made architecture specific.
> > > 
> > I agree.
> > 
> > Yanan, we never discussed an "expose" parameter in the previous versions
> > of this series. We discussed a "strict" parameter though, which would
> > allow existing command lines to "work" using assumptions of what the user
> > meant and strict=on users to get what they mean or an error saying that
> > they asked for something that won't work or would require unreasonable
> > assumptions. Why was this changed to an "expose" parameter?
> Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> of this series.
> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> 
> And in the discussion, we hoped things would work like below with "strict"
> parameter:
> Users who want to describe cpu topology should provide cmdline like
> 
> -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> 
> and in this case we require an more accurate -smp configuration and
> then generate the cpu topology description through ACPI/DT.
> 
> While without a strict description, no cpu topology description would
> be generated, so they get nothing through ACPI/DT.
> 
> It seems to me that the "strict" parameter actually serves as a knob to
> turn on/off the exposure of topology, and this is the reason I changed
> the name.

Yes, the use of 'strict=on' is no better than expose=on IMHO.

If I give QEMU a cli

  -smp cpus=4,sockets=2,cores=2,threads=1

then I expect that topology to be exposed to the guest. I shouldn't
have to add extra flags to make that happen.

Looking at the thread, it seems the concern was around the fact that
the settings were not honoured historically and thus the CLI values
could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9

A similar problem existed on x86 platforms. When we made that stricter
we had cde that issued a warning for a few releases, essentially
deprecating the config. EVentually it was turned into a fatal error.
This gave applications time to fix their broken configs, while having
correct configs "just work".

I'd suggest doing the same for arm. If the -smp args are semantically
valid then expose the topology automatically (for new machine type).
If the -smp args are semantically broken, then issue a warning. In
a few releases time, turn this warning into an error.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

Hi Daniel,

On 2021/6/22 20:41, Daniel P. Berrangé wrote:
> On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
>>
>> On 2021/6/22 19:46, Andrew Jones wrote:
>>> On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
>>>> On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
>>>>> Hi,
>>>>>
>>>>> This is v4 of the series [1] that I posted to introduce support for
>>>>> generating cpu topology descriptions to guest. Comments are welcome!
>>>>>
>>>>> Description:
>>>>> Once the view of an accurate virtual cpu topology is provided to guest,
>>>>> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
>>>>> e.g., the scheduling performance improvement. See Dario Faggioli's
>>>>> research and the related performance tests in [2] for reference. So here
>>>>> we go, this patch series introduces cpu topology support for ARM platform.
>>>>>
>>>>> In this series, instead of quietly enforcing the support for the latest
>>>>> machine type, a new parameter "expose=on|off" in -smp command line is
>>>>> introduced to leave QEMU users a choice to decide whether to enable the
>>>>> feature or not. This will allow the feature to work on different machine
>>>>> types and also ideally compat with already in-use -smp command lines.
>>>>> Also we make much stricter requirement for the topology configuration
>>>>> with "expose=on".
>>>> Seeing this 'expose=on' parameter feels to me like we're adding a
>>>> "make-it-work=yes" parameter. IMHO this is just something that should
>>>> be done by default for the current machine type version and beyond.
>>>> I don't see the need for a parameter to turnthis on, especially since
>>>> it is being made architecture specific.
>>>>
>>> I agree.
>>>
>>> Yanan, we never discussed an "expose" parameter in the previous versions
>>> of this series. We discussed a "strict" parameter though, which would
>>> allow existing command lines to "work" using assumptions of what the user
>>> meant and strict=on users to get what they mean or an error saying that
>>> they asked for something that won't work or would require unreasonable
>>> assumptions. Why was this changed to an "expose" parameter?
>> Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
>> of this series.
>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
>>
>> And in the discussion, we hoped things would work like below with "strict"
>> parameter:
>> Users who want to describe cpu topology should provide cmdline like
>>
>> -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
>>
>> and in this case we require an more accurate -smp configuration and
>> then generate the cpu topology description through ACPI/DT.
>>
>> While without a strict description, no cpu topology description would
>> be generated, so they get nothing through ACPI/DT.
>>
>> It seems to me that the "strict" parameter actually serves as a knob to
>> turn on/off the exposure of topology, and this is the reason I changed
>> the name.
> Yes, the use of 'strict=on' is no better than expose=on IMHO.
>
> If I give QEMU a cli
>
>    -smp cpus=4,sockets=2,cores=2,threads=1
>
> then I expect that topology to be exposed to the guest. I shouldn't
> have to add extra flags to make that happen.
>
> Looking at the thread, it seems the concern was around the fact that
> the settings were not honoured historically and thus the CLI values
> could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
configuration, and the parsing function already report error for this case.

We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
or "-smp 4, cores=1" are not acceptable any more because we are starting
to expose the topology.
> A similar problem existed on x86 platforms. When we made that stricter
> we had cde that issued a warning for a few releases, essentially
> deprecating the config. EVentually it was turned into a fatal error.
> This gave applications time to fix their broken configs, while having
> correct configs "just work".
I understand this solution. Stop exposing topology for unqualified -smp
config and report a warning message at the transitional phase, and finally
incur an error for them.

BTW, just want to be sure, it this a common method in QEMU development
to solve this kind of compatibility issues?
> I'd suggest doing the same for arm. If the -smp args are semantically
> valid then expose the topology automatically (for new machine type).
> If the -smp args are semantically broken, then issue a warning. In
> a few releases time, turn this warning into an error.
So this topology feature will only work for the current machine type and
the following versions, right?

Thanks,
Yanan
.
> Regards,
> Daniel

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Daniel P. Berrangé]

On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
> Hi Daniel,
> 
> On 2021/6/22 20:41, Daniel P. Berrangé wrote:
> > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
> > > 
> > > On 2021/6/22 19:46, Andrew Jones wrote:
> > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
> > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > 
> > > > > > Description:
> > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > 
> > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > with "expose=on".
> > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > be done by default for the current machine type version and beyond.
> > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > it is being made architecture specific.
> > > > > 
> > > > I agree.
> > > > 
> > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > of this series. We discussed a "strict" parameter though, which would
> > > > allow existing command lines to "work" using assumptions of what the user
> > > > meant and strict=on users to get what they mean or an error saying that
> > > > they asked for something that won't work or would require unreasonable
> > > > assumptions. Why was this changed to an "expose" parameter?
> > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > of this series.
> > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > 
> > > And in the discussion, we hoped things would work like below with "strict"
> > > parameter:
> > > Users who want to describe cpu topology should provide cmdline like
> > > 
> > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > 
> > > and in this case we require an more accurate -smp configuration and
> > > then generate the cpu topology description through ACPI/DT.
> > > 
> > > While without a strict description, no cpu topology description would
> > > be generated, so they get nothing through ACPI/DT.
> > > 
> > > It seems to me that the "strict" parameter actually serves as a knob to
> > > turn on/off the exposure of topology, and this is the reason I changed
> > > the name.
> > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > 
> > If I give QEMU a cli
> > 
> >    -smp cpus=4,sockets=2,cores=2,threads=1
> > 
> > then I expect that topology to be exposed to the guest. I shouldn't
> > have to add extra flags to make that happen.
> > 
> > Looking at the thread, it seems the concern was around the fact that
> > the settings were not honoured historically and thus the CLI values
> > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
> This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> configuration, and the parsing function already report error for this case.
> 
> We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> or "-smp 4, cores=1" are not acceptable any more because we are starting
> to expose the topology.

Incomplete specified topologies *are* acceptable.

The smp_parse method will automatically fill in any missing values.

ie,

  -smp 4,cores=1
  -smp cores=1
  -smp threads=1
  -smp sockets=4

are all functionally identical to

  -smp 4,sockets=4,cores=1,dies=1,threads=1


The QEMU man page says this explicitly

                 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.

note this qemu-options.hx doc will require updating since it will apply
to more than just the PC target.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Peter Maydell]

On Tue, 22 Jun 2021 at 15:11, Daniel P. Berrangé <berrange@redhat.com> wrote:
> The QEMU man page says this explicitly
>
>                  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.

Anybody feel like submitting a patch to fix the typo? Should read
"If any of"...

-- PMM

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Daniel P. Berrangé]

On Tue, Jun 22, 2021 at 03:15:18PM +0100, Peter Maydell wrote:
> On Tue, 22 Jun 2021 at 15:11, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > The QEMU man page says this explicitly
> >
> >                  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.
> 
> Anybody feel like submitting a patch to fix the typo? Should read
> "If any of"...

Actually looking at the broader text for -smp, the whole thing just needs
to be rewritten from scratch IMHO. I'll send a patch....

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
> > Hi Daniel,
> > 
> > On 2021/6/22 20:41, Daniel P. Berrangé wrote:
> > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
> > > > 
> > > > On 2021/6/22 19:46, Andrew Jones wrote:
> > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
> > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > 
> > > > > > > Description:
> > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > 
> > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > with "expose=on".
> > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > be done by default for the current machine type version and beyond.
> > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > it is being made architecture specific.
> > > > > > 
> > > > > I agree.
> > > > > 
> > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > they asked for something that won't work or would require unreasonable
> > > > > assumptions. Why was this changed to an "expose" parameter?
> > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > of this series.
> > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > 
> > > > And in the discussion, we hoped things would work like below with "strict"
> > > > parameter:
> > > > Users who want to describe cpu topology should provide cmdline like
> > > > 
> > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > 
> > > > and in this case we require an more accurate -smp configuration and
> > > > then generate the cpu topology description through ACPI/DT.
> > > > 
> > > > While without a strict description, no cpu topology description would
> > > > be generated, so they get nothing through ACPI/DT.
> > > > 
> > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > the name.
> > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > 
> > > If I give QEMU a cli
> > > 
> > >    -smp cpus=4,sockets=2,cores=2,threads=1
> > > 
> > > then I expect that topology to be exposed to the guest. I shouldn't
> > > have to add extra flags to make that happen.
> > > 
> > > Looking at the thread, it seems the concern was around the fact that
> > > the settings were not honoured historically and thus the CLI values
> > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
> > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > configuration, and the parsing function already report error for this case.
> > 
> > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > to expose the topology.
> 
> Incomplete specified topologies *are* acceptable.
> 
> The smp_parse method will automatically fill in any missing values.
> 
> ie,
> 
>   -smp 4,cores=1
>   -smp cores=1
>   -smp threads=1
>   -smp sockets=4
> 
> are all functionally identical to
> 
>   -smp 4,sockets=4,cores=1,dies=1,threads=1
> 
> 
> The QEMU man page says this explicitly
> 
>                  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.

It doesn't say how it will compute them though, which for the default
smp_parse and for x86 is to prefer sockets over cores over threads.
That's not necessarily what the user expects. IMO, we need a 'strict=on'
parameter that doesn't allow any collection of smp parameters which
require unreasonable assumptions. Reasonable assumptions are threads=1,
when threads is not specified and the rest of the math adds up. Also,
maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
as reasonable to decide how to divide cores among sockets or to assume
threads=1 when only sockets and cores are given. How do we know the user
didn't forget to specify threads if we can't check the math?

Thanks,
drew

> 
> note this qemu-options.hx doc will require updating since it will apply
> to more than just the PC target.
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Daniel P. Berrangé]

On Tue, Jun 22, 2021 at 04:29:15PM +0200, Andrew Jones wrote:
> On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
> > > Hi Daniel,
> > > 
> > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:
> > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
> > > > > 
> > > > > On 2021/6/22 19:46, Andrew Jones wrote:
> > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
> > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > 
> > > > > > > > Description:
> > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > 
> > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > with "expose=on".
> > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > it is being made architecture specific.
> > > > > > > 
> > > > > > I agree.
> > > > > > 
> > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > they asked for something that won't work or would require unreasonable
> > > > > > assumptions. Why was this changed to an "expose" parameter?
> > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > of this series.
> > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > 
> > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > parameter:
> > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > 
> > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > 
> > > > > and in this case we require an more accurate -smp configuration and
> > > > > then generate the cpu topology description through ACPI/DT.
> > > > > 
> > > > > While without a strict description, no cpu topology description would
> > > > > be generated, so they get nothing through ACPI/DT.
> > > > > 
> > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > the name.
> > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > 
> > > > If I give QEMU a cli
> > > > 
> > > >    -smp cpus=4,sockets=2,cores=2,threads=1
> > > > 
> > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > have to add extra flags to make that happen.
> > > > 
> > > > Looking at the thread, it seems the concern was around the fact that
> > > > the settings were not honoured historically and thus the CLI values
> > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
> > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > configuration, and the parsing function already report error for this case.
> > > 
> > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > to expose the topology.
> > 
> > Incomplete specified topologies *are* acceptable.
> > 
> > The smp_parse method will automatically fill in any missing values.
> > 
> > ie,
> > 
> >   -smp 4,cores=1
> >   -smp cores=1
> >   -smp threads=1
> >   -smp sockets=4
> > 
> > are all functionally identical to
> > 
> >   -smp 4,sockets=4,cores=1,dies=1,threads=1
> > 
> > 
> > The QEMU man page says this explicitly
> > 
> >                  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.
> 
> It doesn't say how it will compute them though, which for the default
> smp_parse and for x86 is to prefer sockets over cores over threads.
> That's not necessarily what the user expects. IMO, we need a 'strict=on'
> parameter that doesn't allow any collection of smp parameters which
> require unreasonable assumptions. Reasonable assumptions are threads=1,
> when threads is not specified and the rest of the math adds up. Also,
> maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> as reasonable to decide how to divide cores among sockets or to assume
> threads=1 when only sockets and cores are given. How do we know the user
> didn't forget to specify threads if we can't check the math?

It is a tradeoff between convenience and error reporting. The
current approach is obviousy implemented from the POV that the
convenience of defaulting parameters is more broadly useful
than error reporting in fact of mistakes.

If strongly caring about the specific break down of sockets,
dies, cores, threads then all must be provided explicitly
so the built-in computing of omitted parameters is skipped.

If the user can forget to give a threads=NN param, then they
can also forget to give a strict=on param, so that's not
especially compelling way to improve error reporting IMHO.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Igor Mammedov]

On Tue, 22 Jun 2021 16:29:15 +0200
Andrew Jones <drjones@redhat.com> wrote:

> On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:  
> > > Hi Daniel,
> > > 
> > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:  
> > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:  
> > > > > 
> > > > > On 2021/6/22 19:46, Andrew Jones wrote:  
> > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:  
> > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:  
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > 
> > > > > > > > Description:
> > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > 
> > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > with "expose=on".  
> > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > it is being made architecture specific.
> > > > > > >   
> > > > > > I agree.
> > > > > > 
> > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > they asked for something that won't work or would require unreasonable
> > > > > > assumptions. Why was this changed to an "expose" parameter?  
> > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > of this series.
> > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > 
> > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > parameter:
> > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > 
> > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > 
> > > > > and in this case we require an more accurate -smp configuration and
> > > > > then generate the cpu topology description through ACPI/DT.
> > > > > 
> > > > > While without a strict description, no cpu topology description would
> > > > > be generated, so they get nothing through ACPI/DT.
> > > > > 
> > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > the name.  
> > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > 
> > > > If I give QEMU a cli
> > > > 
> > > >    -smp cpus=4,sockets=2,cores=2,threads=1
> > > > 
> > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > have to add extra flags to make that happen.
> > > > 
> > > > Looking at the thread, it seems the concern was around the fact that
> > > > the settings were not honoured historically and thus the CLI values
> > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9  
> > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > configuration, and the parsing function already report error for this case.
> > > 
> > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > to expose the topology.  
> > 
> > Incomplete specified topologies *are* acceptable.
> > 
> > The smp_parse method will automatically fill in any missing values.
> > 
> > ie,
> > 
> >   -smp 4,cores=1
> >   -smp cores=1
> >   -smp threads=1
> >   -smp sockets=4
> > 
> > are all functionally identical to
> > 
> >   -smp 4,sockets=4,cores=1,dies=1,threads=1
> > 
> > 
> > The QEMU man page says this explicitly
> > 
> >                  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.  
> 
> It doesn't say how it will compute them though, which for the default
> smp_parse and for x86 is to prefer sockets over cores over threads.
> That's not necessarily what the user expects. IMO, we need a 'strict=on'
> parameter that doesn't allow any collection of smp parameters which
> require unreasonable assumptions. Reasonable assumptions are threads=1,
> when threads is not specified and the rest of the math adds up. Also,
> maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> as reasonable to decide how to divide cores among sockets or to assume
> threads=1 when only sockets and cores are given. How do we know the user
> didn't forget to specify threads if we can't check the math?

or just outlaw all invalid topologies incl. incomplete by default
(without requiring extra option), and permit them only for old machine
types ()using compat machinery) without topo info provided to guest.
And maybe later deprecate invalid topologies altogether. 


> 
> Thanks,
> drew
> 
> > 
> > note this qemu-options.hx doc will require updating since it will apply
> > to more than just the PC target.
> > 
> > Regards,
> > Daniel
> > -- 
> > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> >   
> 

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
> On Tue, 22 Jun 2021 16:29:15 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:  
> > > > Hi Daniel,
> > > > 
> > > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:  
> > > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:  
> > > > > > 
> > > > > > On 2021/6/22 19:46, Andrew Jones wrote:  
> > > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:  
> > > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:  
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > > 
> > > > > > > > > Description:
> > > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > > 
> > > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > > with "expose=on".  
> > > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > > it is being made architecture specific.
> > > > > > > >   
> > > > > > > I agree.
> > > > > > > 
> > > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > > they asked for something that won't work or would require unreasonable
> > > > > > > assumptions. Why was this changed to an "expose" parameter?  
> > > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > > of this series.
> > > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > > 
> > > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > > parameter:
> > > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > > 
> > > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > > 
> > > > > > and in this case we require an more accurate -smp configuration and
> > > > > > then generate the cpu topology description through ACPI/DT.
> > > > > > 
> > > > > > While without a strict description, no cpu topology description would
> > > > > > be generated, so they get nothing through ACPI/DT.
> > > > > > 
> > > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > > the name.  
> > > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > > 
> > > > > If I give QEMU a cli
> > > > > 
> > > > >    -smp cpus=4,sockets=2,cores=2,threads=1
> > > > > 
> > > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > > have to add extra flags to make that happen.
> > > > > 
> > > > > Looking at the thread, it seems the concern was around the fact that
> > > > > the settings were not honoured historically and thus the CLI values
> > > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9  
> > > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > > configuration, and the parsing function already report error for this case.
> > > > 
> > > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > > to expose the topology.  
> > > 
> > > Incomplete specified topologies *are* acceptable.
> > > 
> > > The smp_parse method will automatically fill in any missing values.
> > > 
> > > ie,
> > > 
> > >   -smp 4,cores=1
> > >   -smp cores=1
> > >   -smp threads=1
> > >   -smp sockets=4
> > > 
> > > are all functionally identical to
> > > 
> > >   -smp 4,sockets=4,cores=1,dies=1,threads=1
> > > 
> > > 
> > > The QEMU man page says this explicitly
> > > 
> > >                  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.  
> > 
> > It doesn't say how it will compute them though, which for the default
> > smp_parse and for x86 is to prefer sockets over cores over threads.
> > That's not necessarily what the user expects. IMO, we need a 'strict=on'
> > parameter that doesn't allow any collection of smp parameters which
> > require unreasonable assumptions. Reasonable assumptions are threads=1,
> > when threads is not specified and the rest of the math adds up. Also,
> > maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> > as reasonable to decide how to divide cores among sockets or to assume
> > threads=1 when only sockets and cores are given. How do we know the user
> > didn't forget to specify threads if we can't check the math?
> 
> or just outlaw all invalid topologies incl. incomplete by default
> (without requiring extra option), and permit them only for old machine
> types ()using compat machinery) without topo info provided to guest.
> And maybe later deprecate invalid topologies altogether. 

I like this proposal, but Peter generally does not want currently working
command lines to stop working. The 'virt' machine type always points to
the latest machine type, so '-M virt -smp sockets=2,cores=4' will
currently work (even though no topology is generated), but, if we were to
merge patches that outlawed that using the compat machinery for 6.2, then
it'll stop working, possibly breaking scripts. The 'strict' parameter
allows one to opt into the strict parsing and actually get the topology
described. Everyone else will still have working command lines without
topology no matter what they use. If Peter agrees to making the smp parser
strict from 6.2 on (possibly by issuing a warning for a release or two
instead of an error, like Daniel suggested), then that's indeed better.
If not, then I don't know how to get a useful -smp command line parser
without the additional parameter.

Thanks,
drew

> 
> 
> > 
> > Thanks,
> > drew
> > 
> > > 
> > > note this qemu-options.hx doc will require updating since it will apply
> > > to more than just the PC target.
> > > 
> > > Regards,
> > > Daniel
> > > -- 
> > > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> > >   
> > 
> 

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Daniel P. Berrangé]

On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
> On Tue, 22 Jun 2021 16:29:15 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:  
> > > > Hi Daniel,
> > > > 
> > > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:  
> > > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:  
> > > > > > 
> > > > > > On 2021/6/22 19:46, Andrew Jones wrote:  
> > > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:  
> > > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:  
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > > 
> > > > > > > > > Description:
> > > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > > 
> > > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > > with "expose=on".  
> > > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > > it is being made architecture specific.
> > > > > > > >   
> > > > > > > I agree.
> > > > > > > 
> > > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > > they asked for something that won't work or would require unreasonable
> > > > > > > assumptions. Why was this changed to an "expose" parameter?  
> > > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > > of this series.
> > > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > > 
> > > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > > parameter:
> > > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > > 
> > > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > > 
> > > > > > and in this case we require an more accurate -smp configuration and
> > > > > > then generate the cpu topology description through ACPI/DT.
> > > > > > 
> > > > > > While without a strict description, no cpu topology description would
> > > > > > be generated, so they get nothing through ACPI/DT.
> > > > > > 
> > > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > > the name.  
> > > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > > 
> > > > > If I give QEMU a cli
> > > > > 
> > > > >    -smp cpus=4,sockets=2,cores=2,threads=1
> > > > > 
> > > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > > have to add extra flags to make that happen.
> > > > > 
> > > > > Looking at the thread, it seems the concern was around the fact that
> > > > > the settings were not honoured historically and thus the CLI values
> > > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9  
> > > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > > configuration, and the parsing function already report error for this case.
> > > > 
> > > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > > to expose the topology.  
> > > 
> > > Incomplete specified topologies *are* acceptable.
> > > 
> > > The smp_parse method will automatically fill in any missing values.
> > > 
> > > ie,
> > > 
> > >   -smp 4,cores=1
> > >   -smp cores=1
> > >   -smp threads=1
> > >   -smp sockets=4
> > > 
> > > are all functionally identical to
> > > 
> > >   -smp 4,sockets=4,cores=1,dies=1,threads=1
> > > 
> > > 
> > > The QEMU man page says this explicitly
> > > 
> > >                  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.  
> > 
> > It doesn't say how it will compute them though, which for the default
> > smp_parse and for x86 is to prefer sockets over cores over threads.
> > That's not necessarily what the user expects. IMO, we need a 'strict=on'
> > parameter that doesn't allow any collection of smp parameters which
> > require unreasonable assumptions. Reasonable assumptions are threads=1,
> > when threads is not specified and the rest of the math adds up. Also,
> > maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> > as reasonable to decide how to divide cores among sockets or to assume
> > threads=1 when only sockets and cores are given. How do we know the user
> > didn't forget to specify threads if we can't check the math?
> 
> or just outlaw all invalid topologies incl. incomplete by default
> (without requiring extra option), and permit them only for old machine
> types ()using compat machinery) without topo info provided to guest.
> And maybe later deprecate invalid topologies altogether.

This feels like it is creating pain for users to fix a problem that
isn't shown to actually be causing any common issues.

We've supposed that users are having problems when forgetting to
specify "threads" and not having the compute value be desirable,
but where are the bug reports to back this up ?

The partial topologies are valid and have well defined semantics.
Those semantics may not match everyone's preference, but that
doesn't make them invalid.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
> > On Tue, 22 Jun 2021 16:29:15 +0200
> > Andrew Jones <drjones@redhat.com> wrote:
> > 
> > > On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > > > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:  
> > > > > Hi Daniel,
> > > > > 
> > > > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:  
> > > > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:  
> > > > > > > 
> > > > > > > On 2021/6/22 19:46, Andrew Jones wrote:  
> > > > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:  
> > > > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:  
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > > > 
> > > > > > > > > > Description:
> > > > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > > > 
> > > > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > > > with "expose=on".  
> > > > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > > > it is being made architecture specific.
> > > > > > > > >   
> > > > > > > > I agree.
> > > > > > > > 
> > > > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > > > they asked for something that won't work or would require unreasonable
> > > > > > > > assumptions. Why was this changed to an "expose" parameter?  
> > > > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > > > of this series.
> > > > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > > > 
> > > > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > > > parameter:
> > > > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > > > 
> > > > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > > > 
> > > > > > > and in this case we require an more accurate -smp configuration and
> > > > > > > then generate the cpu topology description through ACPI/DT.
> > > > > > > 
> > > > > > > While without a strict description, no cpu topology description would
> > > > > > > be generated, so they get nothing through ACPI/DT.
> > > > > > > 
> > > > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > > > the name.  
> > > > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > > > 
> > > > > > If I give QEMU a cli
> > > > > > 
> > > > > >    -smp cpus=4,sockets=2,cores=2,threads=1
> > > > > > 
> > > > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > > > have to add extra flags to make that happen.
> > > > > > 
> > > > > > Looking at the thread, it seems the concern was around the fact that
> > > > > > the settings were not honoured historically and thus the CLI values
> > > > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9  
> > > > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > > > configuration, and the parsing function already report error for this case.
> > > > > 
> > > > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > > > to expose the topology.  
> > > > 
> > > > Incomplete specified topologies *are* acceptable.
> > > > 
> > > > The smp_parse method will automatically fill in any missing values.
> > > > 
> > > > ie,
> > > > 
> > > >   -smp 4,cores=1
> > > >   -smp cores=1
> > > >   -smp threads=1
> > > >   -smp sockets=4
> > > > 
> > > > are all functionally identical to
> > > > 
> > > >   -smp 4,sockets=4,cores=1,dies=1,threads=1
> > > > 
> > > > 
> > > > The QEMU man page says this explicitly
> > > > 
> > > >                  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.  
> > > 
> > > It doesn't say how it will compute them though, which for the default
> > > smp_parse and for x86 is to prefer sockets over cores over threads.
> > > That's not necessarily what the user expects. IMO, we need a 'strict=on'
> > > parameter that doesn't allow any collection of smp parameters which
> > > require unreasonable assumptions. Reasonable assumptions are threads=1,
> > > when threads is not specified and the rest of the math adds up. Also,
> > > maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> > > as reasonable to decide how to divide cores among sockets or to assume
> > > threads=1 when only sockets and cores are given. How do we know the user
> > > didn't forget to specify threads if we can't check the math?
> > 
> > or just outlaw all invalid topologies incl. incomplete by default
> > (without requiring extra option), and permit them only for old machine
> > types ()using compat machinery) without topo info provided to guest.
> > And maybe later deprecate invalid topologies altogether.
> 
> This feels like it is creating pain for users to fix a problem that
> isn't shown to actually be causing any common issues.
> 
> We've supposed that users are having problems when forgetting to
> specify "threads" and not having the compute value be desirable,
> but where are the bug reports to back this up ?
> 
> The partial topologies are valid and have well defined semantics.
> Those semantics may not match everyone's preference, but that
> doesn't make them invalid.
>

If we adopt the [undocumented] semantics of x86 for arm, then we may
surprise some users that expect e.g. '-smp 16' to give them a single
socket with 16 cores, because they'll start getting 16 sockets with 1
core each. That's because if we don't describe a topology to an arm linux
guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
prefer we require explicit inputs from users and, if necessary, for them
to explicitly opt-in to requiring those explicit inputs.

Thanks,
drew

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Daniel P. Berrangé]

On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
> On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
> > > On Tue, 22 Jun 2021 16:29:15 +0200
> > > Andrew Jones <drjones@redhat.com> wrote:
> > > 
> > > > On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > > > > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:  
> > > > > > Hi Daniel,
> > > > > > 
> > > > > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:  
> > > > > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:  
> > > > > > > > 
> > > > > > > > On 2021/6/22 19:46, Andrew Jones wrote:  
> > > > > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:  
> > > > > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:  
> > > > > > > > > > > Hi,
> > > > > > > > > > > 
> > > > > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > > > > 
> > > > > > > > > > > Description:
> > > > > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > > > > 
> > > > > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > > > > with "expose=on".  
> > > > > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > > > > it is being made architecture specific.
> > > > > > > > > >   
> > > > > > > > > I agree.
> > > > > > > > > 
> > > > > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > > > > they asked for something that won't work or would require unreasonable
> > > > > > > > > assumptions. Why was this changed to an "expose" parameter?  
> > > > > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > > > > of this series.
> > > > > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > > > > 
> > > > > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > > > > parameter:
> > > > > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > > > > 
> > > > > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > > > > 
> > > > > > > > and in this case we require an more accurate -smp configuration and
> > > > > > > > then generate the cpu topology description through ACPI/DT.
> > > > > > > > 
> > > > > > > > While without a strict description, no cpu topology description would
> > > > > > > > be generated, so they get nothing through ACPI/DT.
> > > > > > > > 
> > > > > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > > > > the name.  
> > > > > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > > > > 
> > > > > > > If I give QEMU a cli
> > > > > > > 
> > > > > > >    -smp cpus=4,sockets=2,cores=2,threads=1
> > > > > > > 
> > > > > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > > > > have to add extra flags to make that happen.
> > > > > > > 
> > > > > > > Looking at the thread, it seems the concern was around the fact that
> > > > > > > the settings were not honoured historically and thus the CLI values
> > > > > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9  
> > > > > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > > > > configuration, and the parsing function already report error for this case.
> > > > > > 
> > > > > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > > > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > > > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > > > > to expose the topology.  
> > > > > 
> > > > > Incomplete specified topologies *are* acceptable.
> > > > > 
> > > > > The smp_parse method will automatically fill in any missing values.
> > > > > 
> > > > > ie,
> > > > > 
> > > > >   -smp 4,cores=1
> > > > >   -smp cores=1
> > > > >   -smp threads=1
> > > > >   -smp sockets=4
> > > > > 
> > > > > are all functionally identical to
> > > > > 
> > > > >   -smp 4,sockets=4,cores=1,dies=1,threads=1
> > > > > 
> > > > > 
> > > > > The QEMU man page says this explicitly
> > > > > 
> > > > >                  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.  
> > > > 
> > > > It doesn't say how it will compute them though, which for the default
> > > > smp_parse and for x86 is to prefer sockets over cores over threads.
> > > > That's not necessarily what the user expects. IMO, we need a 'strict=on'
> > > > parameter that doesn't allow any collection of smp parameters which
> > > > require unreasonable assumptions. Reasonable assumptions are threads=1,
> > > > when threads is not specified and the rest of the math adds up. Also,
> > > > maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> > > > as reasonable to decide how to divide cores among sockets or to assume
> > > > threads=1 when only sockets and cores are given. How do we know the user
> > > > didn't forget to specify threads if we can't check the math?
> > > 
> > > or just outlaw all invalid topologies incl. incomplete by default
> > > (without requiring extra option), and permit them only for old machine
> > > types ()using compat machinery) without topo info provided to guest.
> > > And maybe later deprecate invalid topologies altogether.
> > 
> > This feels like it is creating pain for users to fix a problem that
> > isn't shown to actually be causing any common issues.
> > 
> > We've supposed that users are having problems when forgetting to
> > specify "threads" and not having the compute value be desirable,
> > but where are the bug reports to back this up ?
> > 
> > The partial topologies are valid and have well defined semantics.
> > Those semantics may not match everyone's preference, but that
> > doesn't make them invalid.
> >
> 
> If we adopt the [undocumented] semantics of x86 for arm, then we may
> surprise some users that expect e.g. '-smp 16' to give them a single
> socket with 16 cores, because they'll start getting 16 sockets with 1
> core each. That's because if we don't describe a topology to an arm linux
> guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
> prefer we require explicit inputs from users and, if necessary, for them
> to explicitly opt-in to requiring those explicit inputs.

Even for x86, defaulting to maximising sockets over cores is sub-optimal.
In real world x86 hardware it is very rare to have sockets > 2 or 4. For
large CPU counts, you generally have large cores-per-socket counts on x86.

The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
is a fairly arbitrary historical decision.

It can cause problems with guest OS licensing because both Windows
and RHEL have been known to charge differently for sockets vs cores,
with high core counts being cheaper.

We are not tied into the precise behaviour of the computed topology
values, as we have no made any promises. All that's required is that
we keep ABI compat for existing machine types.

So we could decide to change the computed topology so that it prefers
high core counts, over sockets, whem using new machine types only.
That would seem to benefit all arches, by making QEMU more reflective
of real world CPUs topology.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

Hi,
On 2021/6/23 1:39, Daniel P. Berrangé wrote:
> On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
>> On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
>>> On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
>>>> On Tue, 22 Jun 2021 16:29:15 +0200
>>>> Andrew Jones <drjones@redhat.com> wrote:
>>>>
>>>>> On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
>>>>>> On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
>>>>>>> Hi Daniel,
>>>>>>>
>>>>>>> On 2021/6/22 20:41, Daniel P. Berrangé wrote:
>>>>>>>> On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
>>>>>>>>> On 2021/6/22 19:46, Andrew Jones wrote:
>>>>>>>>>> On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
>>>>>>>>>>> On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> This is v4 of the series [1] that I posted to introduce support for
>>>>>>>>>>>> generating cpu topology descriptions to guest. Comments are welcome!
>>>>>>>>>>>>
>>>>>>>>>>>> Description:
>>>>>>>>>>>> Once the view of an accurate virtual cpu topology is provided to guest,
>>>>>>>>>>>> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
>>>>>>>>>>>> e.g., the scheduling performance improvement. See Dario Faggioli's
>>>>>>>>>>>> research and the related performance tests in [2] for reference. So here
>>>>>>>>>>>> we go, this patch series introduces cpu topology support for ARM platform.
>>>>>>>>>>>>
>>>>>>>>>>>> In this series, instead of quietly enforcing the support for the latest
>>>>>>>>>>>> machine type, a new parameter "expose=on|off" in -smp command line is
>>>>>>>>>>>> introduced to leave QEMU users a choice to decide whether to enable the
>>>>>>>>>>>> feature or not. This will allow the feature to work on different machine
>>>>>>>>>>>> types and also ideally compat with already in-use -smp command lines.
>>>>>>>>>>>> Also we make much stricter requirement for the topology configuration
>>>>>>>>>>>> with "expose=on".
>>>>>>>>>>> Seeing this 'expose=on' parameter feels to me like we're adding a
>>>>>>>>>>> "make-it-work=yes" parameter. IMHO this is just something that should
>>>>>>>>>>> be done by default for the current machine type version and beyond.
>>>>>>>>>>> I don't see the need for a parameter to turnthis on, especially since
>>>>>>>>>>> it is being made architecture specific.
>>>>>>>>>>>    
>>>>>>>>>> I agree.
>>>>>>>>>>
>>>>>>>>>> Yanan, we never discussed an "expose" parameter in the previous versions
>>>>>>>>>> of this series. We discussed a "strict" parameter though, which would
>>>>>>>>>> allow existing command lines to "work" using assumptions of what the user
>>>>>>>>>> meant and strict=on users to get what they mean or an error saying that
>>>>>>>>>> they asked for something that won't work or would require unreasonable
>>>>>>>>>> assumptions. Why was this changed to an "expose" parameter?
>>>>>>>>> Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
>>>>>>>>> of this series.
>>>>>>>>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
>>>>>>>>>
>>>>>>>>> And in the discussion, we hoped things would work like below with "strict"
>>>>>>>>> parameter:
>>>>>>>>> Users who want to describe cpu topology should provide cmdline like
>>>>>>>>>
>>>>>>>>> -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>
>>>>>>>>> and in this case we require an more accurate -smp configuration and
>>>>>>>>> then generate the cpu topology description through ACPI/DT.
>>>>>>>>>
>>>>>>>>> While without a strict description, no cpu topology description would
>>>>>>>>> be generated, so they get nothing through ACPI/DT.
>>>>>>>>>
>>>>>>>>> It seems to me that the "strict" parameter actually serves as a knob to
>>>>>>>>> turn on/off the exposure of topology, and this is the reason I changed
>>>>>>>>> the name.
>>>>>>>> Yes, the use of 'strict=on' is no better than expose=on IMHO.
>>>>>>>>
>>>>>>>> If I give QEMU a cli
>>>>>>>>
>>>>>>>>     -smp cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>
>>>>>>>> then I expect that topology to be exposed to the guest. I shouldn't
>>>>>>>> have to add extra flags to make that happen.
>>>>>>>>
>>>>>>>> Looking at the thread, it seems the concern was around the fact that
>>>>>>>> the settings were not honoured historically and thus the CLI values
>>>>>>>> could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
>>>>>>> This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
>>>>>>> configuration, and the parsing function already report error for this case.
>>>>>>>
>>>>>>> We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
>>>>>>> for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
>>>>>>> or "-smp 4, cores=1" are not acceptable any more because we are starting
>>>>>>> to expose the topology.
>>>>>> Incomplete specified topologies *are* acceptable.
>>>>>>
>>>>>> The smp_parse method will automatically fill in any missing values.
>>>>>>
>>>>>> ie,
>>>>>>
>>>>>>    -smp 4,cores=1
>>>>>>    -smp cores=1
>>>>>>    -smp threads=1
>>>>>>    -smp sockets=4
>>>>>>
>>>>>> are all functionally identical to
>>>>>>
>>>>>>    -smp 4,sockets=4,cores=1,dies=1,threads=1
>>>>>>
>>>>>>
>>>>>> The QEMU man page says this explicitly
>>>>>>
>>>>>>                   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.
>>>>> It doesn't say how it will compute them though, which for the default
>>>>> smp_parse and for x86 is to prefer sockets over cores over threads.
>>>>> That's not necessarily what the user expects. IMO, we need a 'strict=on'
>>>>> parameter that doesn't allow any collection of smp parameters which
>>>>> require unreasonable assumptions. Reasonable assumptions are threads=1,
>>>>> when threads is not specified and the rest of the math adds up. Also,
>>>>> maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
>>>>> as reasonable to decide how to divide cores among sockets or to assume
>>>>> threads=1 when only sockets and cores are given. How do we know the user
>>>>> didn't forget to specify threads if we can't check the math?
>>>> or just outlaw all invalid topologies incl. incomplete by default
>>>> (without requiring extra option), and permit them only for old machine
>>>> types ()using compat machinery) without topo info provided to guest.
>>>> And maybe later deprecate invalid topologies altogether.
>>> This feels like it is creating pain for users to fix a problem that
>>> isn't shown to actually be causing any common issues.
>>>
>>> We've supposed that users are having problems when forgetting to
>>> specify "threads" and not having the compute value be desirable,
>>> but where are the bug reports to back this up ?
>>>
>>> The partial topologies are valid and have well defined semantics.
>>> Those semantics may not match everyone's preference, but that
>>> doesn't make them invalid.
>>>
>> If we adopt the [undocumented] semantics of x86 for arm, then we may
>> surprise some users that expect e.g. '-smp 16' to give them a single
>> socket with 16 cores, because they'll start getting 16 sockets with 1
>> core each. That's because if we don't describe a topology to an arm linux
>> guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
>> prefer we require explicit inputs from users and, if necessary, for them
>> to explicitly opt-in to requiring those explicit inputs.
> Even for x86, defaulting to maximising sockets over cores is sub-optimal.
> In real world x86 hardware it is very rare to have sockets > 2 or 4. For
> large CPU counts, you generally have large cores-per-socket counts on x86.
>
> The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
> is a fairly arbitrary historical decision.
>
> It can cause problems with guest OS licensing because both Windows
> and RHEL have been known to charge differently for sockets vs cores,
> with high core counts being cheaper.
>
> We are not tied into the precise behaviour of the computed topology
> values, as we have no made any promises. All that's required is that
> we keep ABI compat for existing machine types.
If based on this point of view that we haven't made any promises for the
precise behavior of the computed topology, things may get much easier.
I have the following understanding (also a proposal):

We will introduce the support for exposing cpu topology since machine
type 6.2 and we will also describe the computed topology for the guest.
We will not make any stricter parsing logic, however the -smp content in
qemu-options.hx should be rearranged to clearly explain how the missing
values will exactly be computed. And this is what QEMU is responsible for.

We know that a well designed cpu topology configuration can gain much
benefit for the guest, while a badly designed one will also probably cause
negative impact. But the users should be responsible for the design of the
-smp cmdlines. If they are using an incomplete cmdline for a 6.2 machine,
then they should have known what the computed values will be and that
the computed topology will be exposed to the guest.
>
> So we could decide to change the computed topology so that it prefers
> high core counts, over sockets, whem using new machine types only.
> That would seem to benefit all arches, by making QEMU more reflective
> of real world CPUs topology.
If we really decide to prefer cores over sockets over threads for new 
machine
types, then I think we should also record this change in qemu-option.hx.

Thanks,
Yanan
.
> Regards,
> Daniel

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Mon, Jun 28, 2021 at 04:43:05PM +0800, wangyanan (Y) wrote:
> Hi,
> On 2021/6/23 1:39, Daniel P. Berrangé wrote:
> > On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
> > > On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
> > > > On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
> > > > > On Tue, 22 Jun 2021 16:29:15 +0200
> > > > > Andrew Jones <drjones@redhat.com> wrote:
> > > > > 
> > > > > > On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > > > > > > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
> > > > > > > > Hi Daniel,
> > > > > > > > 
> > > > > > > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:
> > > > > > > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
> > > > > > > > > > On 2021/6/22 19:46, Andrew Jones wrote:
> > > > > > > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
> > > > > > > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Description:
> > > > > > > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > > > > > > with "expose=on".
> > > > > > > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > > > > > > it is being made architecture specific.
> > > > > > > > > > > I agree.
> > > > > > > > > > > 
> > > > > > > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > > > > > > they asked for something that won't work or would require unreasonable
> > > > > > > > > > > assumptions. Why was this changed to an "expose" parameter?
> > > > > > > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > > > > > > of this series.
> > > > > > > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > > > > > > 
> > > > > > > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > > > > > > parameter:
> > > > > > > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > > > > > > 
> > > > > > > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > > > > > > 
> > > > > > > > > > and in this case we require an more accurate -smp configuration and
> > > > > > > > > > then generate the cpu topology description through ACPI/DT.
> > > > > > > > > > 
> > > > > > > > > > While without a strict description, no cpu topology description would
> > > > > > > > > > be generated, so they get nothing through ACPI/DT.
> > > > > > > > > > 
> > > > > > > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > > > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > > > > > > the name.
> > > > > > > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > > > > > > 
> > > > > > > > > If I give QEMU a cli
> > > > > > > > > 
> > > > > > > > >     -smp cpus=4,sockets=2,cores=2,threads=1
> > > > > > > > > 
> > > > > > > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > > > > > > have to add extra flags to make that happen.
> > > > > > > > > 
> > > > > > > > > Looking at the thread, it seems the concern was around the fact that
> > > > > > > > > the settings were not honoured historically and thus the CLI values
> > > > > > > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
> > > > > > > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > > > > > > configuration, and the parsing function already report error for this case.
> > > > > > > > 
> > > > > > > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > > > > > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > > > > > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > > > > > > to expose the topology.
> > > > > > > Incomplete specified topologies *are* acceptable.
> > > > > > > 
> > > > > > > The smp_parse method will automatically fill in any missing values.
> > > > > > > 
> > > > > > > ie,
> > > > > > > 
> > > > > > >    -smp 4,cores=1
> > > > > > >    -smp cores=1
> > > > > > >    -smp threads=1
> > > > > > >    -smp sockets=4
> > > > > > > 
> > > > > > > are all functionally identical to
> > > > > > > 
> > > > > > >    -smp 4,sockets=4,cores=1,dies=1,threads=1
> > > > > > > 
> > > > > > > 
> > > > > > > The QEMU man page says this explicitly
> > > > > > > 
> > > > > > >                   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.
> > > > > > It doesn't say how it will compute them though, which for the default
> > > > > > smp_parse and for x86 is to prefer sockets over cores over threads.
> > > > > > That's not necessarily what the user expects. IMO, we need a 'strict=on'
> > > > > > parameter that doesn't allow any collection of smp parameters which
> > > > > > require unreasonable assumptions. Reasonable assumptions are threads=1,
> > > > > > when threads is not specified and the rest of the math adds up. Also,
> > > > > > maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> > > > > > as reasonable to decide how to divide cores among sockets or to assume
> > > > > > threads=1 when only sockets and cores are given. How do we know the user
> > > > > > didn't forget to specify threads if we can't check the math?
> > > > > or just outlaw all invalid topologies incl. incomplete by default
> > > > > (without requiring extra option), and permit them only for old machine
> > > > > types ()using compat machinery) without topo info provided to guest.
> > > > > And maybe later deprecate invalid topologies altogether.
> > > > This feels like it is creating pain for users to fix a problem that
> > > > isn't shown to actually be causing any common issues.
> > > > 
> > > > We've supposed that users are having problems when forgetting to
> > > > specify "threads" and not having the compute value be desirable,
> > > > but where are the bug reports to back this up ?
> > > > 
> > > > The partial topologies are valid and have well defined semantics.
> > > > Those semantics may not match everyone's preference, but that
> > > > doesn't make them invalid.
> > > > 
> > > If we adopt the [undocumented] semantics of x86 for arm, then we may
> > > surprise some users that expect e.g. '-smp 16' to give them a single
> > > socket with 16 cores, because they'll start getting 16 sockets with 1
> > > core each. That's because if we don't describe a topology to an arm linux
> > > guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
> > > prefer we require explicit inputs from users and, if necessary, for them
> > > to explicitly opt-in to requiring those explicit inputs.
> > Even for x86, defaulting to maximising sockets over cores is sub-optimal.
> > In real world x86 hardware it is very rare to have sockets > 2 or 4. For
> > large CPU counts, you generally have large cores-per-socket counts on x86.
> > 
> > The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
> > is a fairly arbitrary historical decision.
> > 
> > It can cause problems with guest OS licensing because both Windows
> > and RHEL have been known to charge differently for sockets vs cores,
> > with high core counts being cheaper.
> > 
> > We are not tied into the precise behaviour of the computed topology
> > values, as we have no made any promises. All that's required is that
> > we keep ABI compat for existing machine types.
> If based on this point of view that we haven't made any promises for the
> precise behavior of the computed topology, things may get much easier.
> I have the following understanding (also a proposal):
> 
> We will introduce the support for exposing cpu topology since machine
> type 6.2 and we will also describe the computed topology for the guest.
> We will not make any stricter parsing logic, however the -smp content in
> qemu-options.hx should be rearranged to clearly explain how the missing
> values will exactly be computed. And this is what QEMU is responsible for.
> 
> We know that a well designed cpu topology configuration can gain much
> benefit for the guest, while a badly designed one will also probably cause
> negative impact. But the users should be responsible for the design of the
> -smp cmdlines. If they are using an incomplete cmdline for a 6.2 machine,
> then they should have known what the computed values will be and that
> the computed topology will be exposed to the guest.
> > 
> > So we could decide to change the computed topology so that it prefers
> > high core counts, over sockets, whem using new machine types only.
> > That would seem to benefit all arches, by making QEMU more reflective
> > of real world CPUs topology.
> If we really decide to prefer cores over sockets over threads for new
> machine
> types, then I think we should also record this change in qemu-option.hx.
>

I agree. The proposal sounds good to me. I'd like to hear Eduardo's
opinion too (CC'ed).

Thanks,
drew 

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

On 2021/6/28 16:58, Andrew Jones wrote:
> On Mon, Jun 28, 2021 at 04:43:05PM +0800, wangyanan (Y) wrote:
>> Hi,
>> On 2021/6/23 1:39, Daniel P. Berrangé wrote:
>>> On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
>>>> On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
>>>>> On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
>>>>>> On Tue, 22 Jun 2021 16:29:15 +0200
>>>>>> Andrew Jones <drjones@redhat.com> wrote:
>>>>>>
>>>>>>> On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
>>>>>>>> On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
>>>>>>>>> Hi Daniel,
>>>>>>>>>
>>>>>>>>> On 2021/6/22 20:41, Daniel P. Berrangé wrote:
>>>>>>>>>> On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
>>>>>>>>>>> On 2021/6/22 19:46, Andrew Jones wrote:
>>>>>>>>>>>> On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
>>>>>>>>>>>>> On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is v4 of the series [1] that I posted to introduce support for
>>>>>>>>>>>>>> generating cpu topology descriptions to guest. Comments are welcome!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Description:
>>>>>>>>>>>>>> Once the view of an accurate virtual cpu topology is provided to guest,
>>>>>>>>>>>>>> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
>>>>>>>>>>>>>> e.g., the scheduling performance improvement. See Dario Faggioli's
>>>>>>>>>>>>>> research and the related performance tests in [2] for reference. So here
>>>>>>>>>>>>>> we go, this patch series introduces cpu topology support for ARM platform.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In this series, instead of quietly enforcing the support for the latest
>>>>>>>>>>>>>> machine type, a new parameter "expose=on|off" in -smp command line is
>>>>>>>>>>>>>> introduced to leave QEMU users a choice to decide whether to enable the
>>>>>>>>>>>>>> feature or not. This will allow the feature to work on different machine
>>>>>>>>>>>>>> types and also ideally compat with already in-use -smp command lines.
>>>>>>>>>>>>>> Also we make much stricter requirement for the topology configuration
>>>>>>>>>>>>>> with "expose=on".
>>>>>>>>>>>>> Seeing this 'expose=on' parameter feels to me like we're adding a
>>>>>>>>>>>>> "make-it-work=yes" parameter. IMHO this is just something that should
>>>>>>>>>>>>> be done by default for the current machine type version and beyond.
>>>>>>>>>>>>> I don't see the need for a parameter to turnthis on, especially since
>>>>>>>>>>>>> it is being made architecture specific.
>>>>>>>>>>>> I agree.
>>>>>>>>>>>>
>>>>>>>>>>>> Yanan, we never discussed an "expose" parameter in the previous versions
>>>>>>>>>>>> of this series. We discussed a "strict" parameter though, which would
>>>>>>>>>>>> allow existing command lines to "work" using assumptions of what the user
>>>>>>>>>>>> meant and strict=on users to get what they mean or an error saying that
>>>>>>>>>>>> they asked for something that won't work or would require unreasonable
>>>>>>>>>>>> assumptions. Why was this changed to an "expose" parameter?
>>>>>>>>>>> Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
>>>>>>>>>>> of this series.
>>>>>>>>>>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
>>>>>>>>>>>
>>>>>>>>>>> And in the discussion, we hoped things would work like below with "strict"
>>>>>>>>>>> parameter:
>>>>>>>>>>> Users who want to describe cpu topology should provide cmdline like
>>>>>>>>>>>
>>>>>>>>>>> -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>>>
>>>>>>>>>>> and in this case we require an more accurate -smp configuration and
>>>>>>>>>>> then generate the cpu topology description through ACPI/DT.
>>>>>>>>>>>
>>>>>>>>>>> While without a strict description, no cpu topology description would
>>>>>>>>>>> be generated, so they get nothing through ACPI/DT.
>>>>>>>>>>>
>>>>>>>>>>> It seems to me that the "strict" parameter actually serves as a knob to
>>>>>>>>>>> turn on/off the exposure of topology, and this is the reason I changed
>>>>>>>>>>> the name.
>>>>>>>>>> Yes, the use of 'strict=on' is no better than expose=on IMHO.
>>>>>>>>>>
>>>>>>>>>> If I give QEMU a cli
>>>>>>>>>>
>>>>>>>>>>      -smp cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>>
>>>>>>>>>> then I expect that topology to be exposed to the guest. I shouldn't
>>>>>>>>>> have to add extra flags to make that happen.
>>>>>>>>>>
>>>>>>>>>> Looking at the thread, it seems the concern was around the fact that
>>>>>>>>>> the settings were not honoured historically and thus the CLI values
>>>>>>>>>> could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
>>>>>>>>> This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
>>>>>>>>> configuration, and the parsing function already report error for this case.
>>>>>>>>>
>>>>>>>>> We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
>>>>>>>>> for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
>>>>>>>>> or "-smp 4, cores=1" are not acceptable any more because we are starting
>>>>>>>>> to expose the topology.
>>>>>>>> Incomplete specified topologies *are* acceptable.
>>>>>>>>
>>>>>>>> The smp_parse method will automatically fill in any missing values.
>>>>>>>>
>>>>>>>> ie,
>>>>>>>>
>>>>>>>>     -smp 4,cores=1
>>>>>>>>     -smp cores=1
>>>>>>>>     -smp threads=1
>>>>>>>>     -smp sockets=4
>>>>>>>>
>>>>>>>> are all functionally identical to
>>>>>>>>
>>>>>>>>     -smp 4,sockets=4,cores=1,dies=1,threads=1
>>>>>>>>
>>>>>>>>
>>>>>>>> The QEMU man page says this explicitly
>>>>>>>>
>>>>>>>>                    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.
>>>>>>> It doesn't say how it will compute them though, which for the default
>>>>>>> smp_parse and for x86 is to prefer sockets over cores over threads.
>>>>>>> That's not necessarily what the user expects. IMO, we need a 'strict=on'
>>>>>>> parameter that doesn't allow any collection of smp parameters which
>>>>>>> require unreasonable assumptions. Reasonable assumptions are threads=1,
>>>>>>> when threads is not specified and the rest of the math adds up. Also,
>>>>>>> maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
>>>>>>> as reasonable to decide how to divide cores among sockets or to assume
>>>>>>> threads=1 when only sockets and cores are given. How do we know the user
>>>>>>> didn't forget to specify threads if we can't check the math?
>>>>>> or just outlaw all invalid topologies incl. incomplete by default
>>>>>> (without requiring extra option), and permit them only for old machine
>>>>>> types ()using compat machinery) without topo info provided to guest.
>>>>>> And maybe later deprecate invalid topologies altogether.
>>>>> This feels like it is creating pain for users to fix a problem that
>>>>> isn't shown to actually be causing any common issues.
>>>>>
>>>>> We've supposed that users are having problems when forgetting to
>>>>> specify "threads" and not having the compute value be desirable,
>>>>> but where are the bug reports to back this up ?
>>>>>
>>>>> The partial topologies are valid and have well defined semantics.
>>>>> Those semantics may not match everyone's preference, but that
>>>>> doesn't make them invalid.
>>>>>
>>>> If we adopt the [undocumented] semantics of x86 for arm, then we may
>>>> surprise some users that expect e.g. '-smp 16' to give them a single
>>>> socket with 16 cores, because they'll start getting 16 sockets with 1
>>>> core each. That's because if we don't describe a topology to an arm linux
>>>> guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
>>>> prefer we require explicit inputs from users and, if necessary, for them
>>>> to explicitly opt-in to requiring those explicit inputs.
>>> Even for x86, defaulting to maximising sockets over cores is sub-optimal.
>>> In real world x86 hardware it is very rare to have sockets > 2 or 4. For
>>> large CPU counts, you generally have large cores-per-socket counts on x86.
>>>
>>> The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
>>> is a fairly arbitrary historical decision.
>>>
>>> It can cause problems with guest OS licensing because both Windows
>>> and RHEL have been known to charge differently for sockets vs cores,
>>> with high core counts being cheaper.
>>>
>>> We are not tied into the precise behaviour of the computed topology
>>> values, as we have no made any promises. All that's required is that
>>> we keep ABI compat for existing machine types.
>> If based on this point of view that we haven't made any promises for the
>> precise behavior of the computed topology, things may get much easier.
>> I have the following understanding (also a proposal):
>>
>> We will introduce the support for exposing cpu topology since machine
>> type 6.2 and we will also describe the computed topology for the guest.
>> We will not make any stricter parsing logic, however the -smp content in
>> qemu-options.hx should be rearranged to clearly explain how the missing
>> values will exactly be computed. And this is what QEMU is responsible for.
>>
>> We know that a well designed cpu topology configuration can gain much
>> benefit for the guest, while a badly designed one will also probably cause
>> negative impact. But the users should be responsible for the design of the
>> -smp cmdlines. If they are using an incomplete cmdline for a 6.2 machine,
>> then they should have known what the computed values will be and that
>> the computed topology will be exposed to the guest.
>>> So we could decide to change the computed topology so that it prefers
>>> high core counts, over sockets, whem using new machine types only.
>>> That would seem to benefit all arches, by making QEMU more reflective
>>> of real world CPUs topology.
>> If we really decide to prefer cores over sockets over threads for new
>> machine
>> types, then I think we should also record this change in qemu-option.hx.
>>
> I agree. The proposal sounds good to me. I'd like to hear Eduardo's
> opinion too (CC'ed).
Thanks, Drew. I also hope that more people will reach an agreement on 
this issue,
so that I can prepare v5 with confidence, although it will probably be 
late for 6.1.

Thanks,
Yanan
.
> Thanks,
> drew
>
>
> .

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

Hi Daniel,

On 2021/6/22 22:28, Daniel P. Berrangé wrote:
> On Tue, Jun 22, 2021 at 03:15:18PM +0100, Peter Maydell wrote:
>> On Tue, 22 Jun 2021 at 15:11, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>> The QEMU man page says this explicitly
>>>
>>>                   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.
>> Anybody feel like submitting a patch to fix the typo? Should read
>> "If any of"...
> Actually looking at the broader text for -smp, the whole thing just needs
> to be rewritten from scratch IMHO. I'll send a patch....
Do you have any plan to rewrite the -smp text in qemu-options.hx recently?

I suggest that we explain in more detail how the missing values will
be computed, so that qemu users can have a clear image of what a
-smp parameter collection would be parsed out if they are using an
incomplete cmdline and have read the man page.

Also if we all agree to prefer cores over sockets for all arches since
machine type 6.2, I'll be glad to add this change for the man page
and update the parsing helpers in next version of this patch series.

Thanks,
Yanan
.

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Daniel P. Berrangé]

On Mon, Jun 28, 2021 at 07:14:02PM +0800, wangyanan (Y) wrote:
> Hi Daniel,
> 
> On 2021/6/22 22:28, Daniel P. Berrangé wrote:
> > On Tue, Jun 22, 2021 at 03:15:18PM +0100, Peter Maydell wrote:
> > > On Tue, 22 Jun 2021 at 15:11, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > The QEMU man page says this explicitly
> > > > 
> > > >                   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.
> > > Anybody feel like submitting a patch to fix the typo? Should read
> > > "If any of"...
> > Actually looking at the broader text for -smp, the whole thing just needs
> > to be rewritten from scratch IMHO. I'll send a patch....
> Do you have any plan to rewrite the -smp text in qemu-options.hx recently?
> 
> I suggest that we explain in more detail how the missing values will
> be computed, so that qemu users can have a clear image of what a
> -smp parameter collection would be parsed out if they are using an
> incomplete cmdline and have read the man page.
> 
> Also if we all agree to prefer cores over sockets for all arches since
> machine type 6.2, I'll be glad to add this change for the man page
> and update the parsing helpers in next version of this patch series.

I wrote a docs update on friday, which I've just sent out as a short
patch series with yourself CC'd.  I wrote it such that we can easily
update it again, if we want to prefer cores over sockets.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

On 2021/6/28 19:31, Daniel P. Berrangé wrote:
> On Mon, Jun 28, 2021 at 07:14:02PM +0800, wangyanan (Y) wrote:
>> Hi Daniel,
>>
>> On 2021/6/22 22:28, Daniel P. Berrangé wrote:
>>> On Tue, Jun 22, 2021 at 03:15:18PM +0100, Peter Maydell wrote:
>>>> On Tue, 22 Jun 2021 at 15:11, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>> The QEMU man page says this explicitly
>>>>>
>>>>>                    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.
>>>> Anybody feel like submitting a patch to fix the typo? Should read
>>>> "If any of"...
>>> Actually looking at the broader text for -smp, the whole thing just needs
>>> to be rewritten from scratch IMHO. I'll send a patch....
>> Do you have any plan to rewrite the -smp text in qemu-options.hx recently?
>>
>> I suggest that we explain in more detail how the missing values will
>> be computed, so that qemu users can have a clear image of what a
>> -smp parameter collection would be parsed out if they are using an
>> incomplete cmdline and have read the man page.
>>
>> Also if we all agree to prefer cores over sockets for all arches since
>> machine type 6.2, I'll be glad to add this change for the man page
>> and update the parsing helpers in next version of this patch series.
> I wrote a docs update on friday, which I've just sent out as a short
> patch series with yourself CC'd.  I wrote it such that we can easily
> update it again, if we want to prefer cores over sockets.
>
Great, thanks. I'm going to have a look.

Thanks,
Yanan
.

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

Hi Drew, Igor,

I have a question below, hope for some explanation... :)

I'm trying to rearrange the smp_parse() helper to make it more scalable.
But I wonder why we are currently using maxcpus to calculate the missing
sockets while using *cpus* to calculate the missing cores and threads?

This makes the following cmdlines work fine,
-smp cpus=8,maxcpus=12  <==>  -smp 
cpus=8,maxcpus=12,sockets=12,cores=1,threads=1
-smp cpus=8,maxcpus=12,cores=6  <==>  -smp 
cpus=8,maxcpus=12,sockets=2,cores=6,threads=1
-smp cpus=8,maxcpus=12,threads=2  <==>  -smp 
cpus=8,maxcpus=12,sockets=6,cores=1,threads=2

but the following ones break the invalid CPU topology check:
-smp cpus=8,maxcpus=12,sockets=2  <==>  -smp 
cpus=8,maxcpus=12,sockets=2,cores=4,threads=1
-smp cpus=8,maxcpus=12,sockets=4,threads=1  <==>  -smp 
cpus=8,maxcpus=12,sockets=4,cores=2,threads=1
-smp maxcpus=12  <==>  -smp cpus=1,maxcpus=12,sockets=1,cores=1,threads=1
-smp maxcpus=12,sockets=2  <==>  -smp 
cpus=2,maxcpus=12,sockets=2,cores=1,threads=1

IMO we should uniformly use maxcpus to calculate the missing sockets
also cores and threads, which will allow all the above cmdlines work.
Or maybe I missed something? I read the related discussion in [1] but
didn't get an unambiguous conclusion.

[1] 
https://patchwork.kernel.org/project/qemu-devel/patch/1535553121-80352-1-git-send-email-imammedo@redhat.com/

Regards,
Yanan
.

On 2021/6/28 16:58, Andrew Jones wrote:
> On Mon, Jun 28, 2021 at 04:43:05PM +0800, wangyanan (Y) wrote:
>> Hi,
>> On 2021/6/23 1:39, Daniel P. Berrangé wrote:
>>> On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
>>>> On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
>>>>> On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
>>>>>> On Tue, 22 Jun 2021 16:29:15 +0200
>>>>>> Andrew Jones <drjones@redhat.com> wrote:
>>>>>>
>>>>>>> On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
>>>>>>>> On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
>>>>>>>>> Hi Daniel,
>>>>>>>>>
>>>>>>>>> On 2021/6/22 20:41, Daniel P. Berrangé wrote:
>>>>>>>>>> On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
>>>>>>>>>>> On 2021/6/22 19:46, Andrew Jones wrote:
>>>>>>>>>>>> On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
>>>>>>>>>>>>> On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is v4 of the series [1] that I posted to introduce support for
>>>>>>>>>>>>>> generating cpu topology descriptions to guest. Comments are welcome!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Description:
>>>>>>>>>>>>>> Once the view of an accurate virtual cpu topology is provided to guest,
>>>>>>>>>>>>>> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
>>>>>>>>>>>>>> e.g., the scheduling performance improvement. See Dario Faggioli's
>>>>>>>>>>>>>> research and the related performance tests in [2] for reference. So here
>>>>>>>>>>>>>> we go, this patch series introduces cpu topology support for ARM platform.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In this series, instead of quietly enforcing the support for the latest
>>>>>>>>>>>>>> machine type, a new parameter "expose=on|off" in -smp command line is
>>>>>>>>>>>>>> introduced to leave QEMU users a choice to decide whether to enable the
>>>>>>>>>>>>>> feature or not. This will allow the feature to work on different machine
>>>>>>>>>>>>>> types and also ideally compat with already in-use -smp command lines.
>>>>>>>>>>>>>> Also we make much stricter requirement for the topology configuration
>>>>>>>>>>>>>> with "expose=on".
>>>>>>>>>>>>> Seeing this 'expose=on' parameter feels to me like we're adding a
>>>>>>>>>>>>> "make-it-work=yes" parameter. IMHO this is just something that should
>>>>>>>>>>>>> be done by default for the current machine type version and beyond.
>>>>>>>>>>>>> I don't see the need for a parameter to turnthis on, especially since
>>>>>>>>>>>>> it is being made architecture specific.
>>>>>>>>>>>> I agree.
>>>>>>>>>>>>
>>>>>>>>>>>> Yanan, we never discussed an "expose" parameter in the previous versions
>>>>>>>>>>>> of this series. We discussed a "strict" parameter though, which would
>>>>>>>>>>>> allow existing command lines to "work" using assumptions of what the user
>>>>>>>>>>>> meant and strict=on users to get what they mean or an error saying that
>>>>>>>>>>>> they asked for something that won't work or would require unreasonable
>>>>>>>>>>>> assumptions. Why was this changed to an "expose" parameter?
>>>>>>>>>>> Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
>>>>>>>>>>> of this series.
>>>>>>>>>>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
>>>>>>>>>>>
>>>>>>>>>>> And in the discussion, we hoped things would work like below with "strict"
>>>>>>>>>>> parameter:
>>>>>>>>>>> Users who want to describe cpu topology should provide cmdline like
>>>>>>>>>>>
>>>>>>>>>>> -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>>>
>>>>>>>>>>> and in this case we require an more accurate -smp configuration and
>>>>>>>>>>> then generate the cpu topology description through ACPI/DT.
>>>>>>>>>>>
>>>>>>>>>>> While without a strict description, no cpu topology description would
>>>>>>>>>>> be generated, so they get nothing through ACPI/DT.
>>>>>>>>>>>
>>>>>>>>>>> It seems to me that the "strict" parameter actually serves as a knob to
>>>>>>>>>>> turn on/off the exposure of topology, and this is the reason I changed
>>>>>>>>>>> the name.
>>>>>>>>>> Yes, the use of 'strict=on' is no better than expose=on IMHO.
>>>>>>>>>>
>>>>>>>>>> If I give QEMU a cli
>>>>>>>>>>
>>>>>>>>>>      -smp cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>>
>>>>>>>>>> then I expect that topology to be exposed to the guest. I shouldn't
>>>>>>>>>> have to add extra flags to make that happen.
>>>>>>>>>>
>>>>>>>>>> Looking at the thread, it seems the concern was around the fact that
>>>>>>>>>> the settings were not honoured historically and thus the CLI values
>>>>>>>>>> could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
>>>>>>>>> This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
>>>>>>>>> configuration, and the parsing function already report error for this case.
>>>>>>>>>
>>>>>>>>> We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
>>>>>>>>> for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
>>>>>>>>> or "-smp 4, cores=1" are not acceptable any more because we are starting
>>>>>>>>> to expose the topology.
>>>>>>>> Incomplete specified topologies *are* acceptable.
>>>>>>>>
>>>>>>>> The smp_parse method will automatically fill in any missing values.
>>>>>>>>
>>>>>>>> ie,
>>>>>>>>
>>>>>>>>     -smp 4,cores=1
>>>>>>>>     -smp cores=1
>>>>>>>>     -smp threads=1
>>>>>>>>     -smp sockets=4
>>>>>>>>
>>>>>>>> are all functionally identical to
>>>>>>>>
>>>>>>>>     -smp 4,sockets=4,cores=1,dies=1,threads=1
>>>>>>>>
>>>>>>>>
>>>>>>>> The QEMU man page says this explicitly
>>>>>>>>
>>>>>>>>                    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.
>>>>>>> It doesn't say how it will compute them though, which for the default
>>>>>>> smp_parse and for x86 is to prefer sockets over cores over threads.
>>>>>>> That's not necessarily what the user expects. IMO, we need a 'strict=on'
>>>>>>> parameter that doesn't allow any collection of smp parameters which
>>>>>>> require unreasonable assumptions. Reasonable assumptions are threads=1,
>>>>>>> when threads is not specified and the rest of the math adds up. Also,
>>>>>>> maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
>>>>>>> as reasonable to decide how to divide cores among sockets or to assume
>>>>>>> threads=1 when only sockets and cores are given. How do we know the user
>>>>>>> didn't forget to specify threads if we can't check the math?
>>>>>> or just outlaw all invalid topologies incl. incomplete by default
>>>>>> (without requiring extra option), and permit them only for old machine
>>>>>> types ()using compat machinery) without topo info provided to guest.
>>>>>> And maybe later deprecate invalid topologies altogether.
>>>>> This feels like it is creating pain for users to fix a problem that
>>>>> isn't shown to actually be causing any common issues.
>>>>>
>>>>> We've supposed that users are having problems when forgetting to
>>>>> specify "threads" and not having the compute value be desirable,
>>>>> but where are the bug reports to back this up ?
>>>>>
>>>>> The partial topologies are valid and have well defined semantics.
>>>>> Those semantics may not match everyone's preference, but that
>>>>> doesn't make them invalid.
>>>>>
>>>> If we adopt the [undocumented] semantics of x86 for arm, then we may
>>>> surprise some users that expect e.g. '-smp 16' to give them a single
>>>> socket with 16 cores, because they'll start getting 16 sockets with 1
>>>> core each. That's because if we don't describe a topology to an arm linux
>>>> guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
>>>> prefer we require explicit inputs from users and, if necessary, for them
>>>> to explicitly opt-in to requiring those explicit inputs.
>>> Even for x86, defaulting to maximising sockets over cores is sub-optimal.
>>> In real world x86 hardware it is very rare to have sockets > 2 or 4. For
>>> large CPU counts, you generally have large cores-per-socket counts on x86.
>>>
>>> The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
>>> is a fairly arbitrary historical decision.
>>>
>>> It can cause problems with guest OS licensing because both Windows
>>> and RHEL have been known to charge differently for sockets vs cores,
>>> with high core counts being cheaper.
>>>
>>> We are not tied into the precise behaviour of the computed topology
>>> values, as we have no made any promises. All that's required is that
>>> we keep ABI compat for existing machine types.
>> If based on this point of view that we haven't made any promises for the
>> precise behavior of the computed topology, things may get much easier.
>> I have the following understanding (also a proposal):
>>
>> We will introduce the support for exposing cpu topology since machine
>> type 6.2 and we will also describe the computed topology for the guest.
>> We will not make any stricter parsing logic, however the -smp content in
>> qemu-options.hx should be rearranged to clearly explain how the missing
>> values will exactly be computed. And this is what QEMU is responsible for.
>>
>> We know that a well designed cpu topology configuration can gain much
>> benefit for the guest, while a badly designed one will also probably cause
>> negative impact. But the users should be responsible for the design of the
>> -smp cmdlines. If they are using an incomplete cmdline for a 6.2 machine,
>> then they should have known what the computed values will be and that
>> the computed topology will be exposed to the guest.
>>> So we could decide to change the computed topology so that it prefers
>>> high core counts, over sockets, whem using new machine types only.
>>> That would seem to benefit all arches, by making QEMU more reflective
>>> of real world CPUs topology.
>> If we really decide to prefer cores over sockets over threads for new
>> machine
>> types, then I think we should also record this change in qemu-option.hx.
>>
> I agree. The proposal sounds good to me. I'd like to hear Eduardo's
> opinion too (CC'ed).
>
> Thanks,
> drew
>
>
> .

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Wed, Jun 30, 2021 at 02:36:31PM +0800, wangyanan (Y) wrote:
> Hi Drew, Igor,
> 
> I have a question below, hope for some explanation... :)
> 
> I'm trying to rearrange the smp_parse() helper to make it more scalable.
> But I wonder why we are currently using maxcpus to calculate the missing
> sockets while using *cpus* to calculate the missing cores and threads?
> 
> This makes the following cmdlines work fine,
> -smp cpus=8,maxcpus=12  <==>  -smp
> cpus=8,maxcpus=12,sockets=12,cores=1,threads=1
> -smp cpus=8,maxcpus=12,cores=6  <==>  -smp
> cpus=8,maxcpus=12,sockets=2,cores=6,threads=1
> -smp cpus=8,maxcpus=12,threads=2  <==>  -smp
> cpus=8,maxcpus=12,sockets=6,cores=1,threads=2
> 
> but the following ones break the invalid CPU topology check:
> -smp cpus=8,maxcpus=12,sockets=2  <==>  -smp
> cpus=8,maxcpus=12,sockets=2,cores=4,threads=1
> -smp cpus=8,maxcpus=12,sockets=4,threads=1  <==>  -smp
> cpus=8,maxcpus=12,sockets=4,cores=2,threads=1
> -smp maxcpus=12  <==>  -smp cpus=1,maxcpus=12,sockets=1,cores=1,threads=1
> -smp maxcpus=12,sockets=2  <==>  -smp
> cpus=2,maxcpus=12,sockets=2,cores=1,threads=1
> 
> IMO we should uniformly use maxcpus to calculate the missing sockets
> also cores and threads, which will allow all the above cmdlines work.
> Or maybe I missed something? I read the related discussion in [1] but
> didn't get an unambiguous conclusion.
> 
> [1] https://patchwork.kernel.org/project/qemu-devel/patch/1535553121-80352-1-git-send-email-imammedo@redhat.com/

I agree that maxcpus should be used for all calculations. I think we need
to write -smp parsing from scratch using a set of clean requirements and
then use the machine compat stuff to switch to it. And also properly
document it with something like "Since 6.2..."

Thanks,
drew

> 
> Regards,
> Yanan
> .
> 
> On 2021/6/28 16:58, Andrew Jones wrote:
> > On Mon, Jun 28, 2021 at 04:43:05PM +0800, wangyanan (Y) wrote:
> > > Hi,
> > > On 2021/6/23 1:39, Daniel P. Berrangé wrote:
> > > > On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
> > > > > On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
> > > > > > On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
> > > > > > > On Tue, 22 Jun 2021 16:29:15 +0200
> > > > > > > Andrew Jones <drjones@redhat.com> wrote:
> > > > > > > 
> > > > > > > > On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > > > > > > > > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
> > > > > > > > > > Hi Daniel,
> > > > > > > > > > 
> > > > > > > > > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:
> > > > > > > > > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
> > > > > > > > > > > > On 2021/6/22 19:46, Andrew Jones wrote:
> > > > > > > > > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
> > > > > > > > > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Description:
> > > > > > > > > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > > > > > > > > with "expose=on".
> > > > > > > > > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > > > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > > > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > > > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > > > > > > > > it is being made architecture specific.
> > > > > > > > > > > > > I agree.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > > > > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > > > > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > > > > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > > > > > > > > they asked for something that won't work or would require unreasonable
> > > > > > > > > > > > > assumptions. Why was this changed to an "expose" parameter?
> > > > > > > > > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > > > > > > > > of this series.
> > > > > > > > > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > > > > > > > > 
> > > > > > > > > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > > > > > > > > parameter:
> > > > > > > > > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > > > > > > > > 
> > > > > > > > > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > > > > > > > > 
> > > > > > > > > > > > and in this case we require an more accurate -smp configuration and
> > > > > > > > > > > > then generate the cpu topology description through ACPI/DT.
> > > > > > > > > > > > 
> > > > > > > > > > > > While without a strict description, no cpu topology description would
> > > > > > > > > > > > be generated, so they get nothing through ACPI/DT.
> > > > > > > > > > > > 
> > > > > > > > > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > > > > > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > > > > > > > > the name.
> > > > > > > > > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > > > > > > > > 
> > > > > > > > > > > If I give QEMU a cli
> > > > > > > > > > > 
> > > > > > > > > > >      -smp cpus=4,sockets=2,cores=2,threads=1
> > > > > > > > > > > 
> > > > > > > > > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > > > > > > > > have to add extra flags to make that happen.
> > > > > > > > > > > 
> > > > > > > > > > > Looking at the thread, it seems the concern was around the fact that
> > > > > > > > > > > the settings were not honoured historically and thus the CLI values
> > > > > > > > > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
> > > > > > > > > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > > > > > > > > configuration, and the parsing function already report error for this case.
> > > > > > > > > > 
> > > > > > > > > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > > > > > > > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > > > > > > > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > > > > > > > > to expose the topology.
> > > > > > > > > Incomplete specified topologies *are* acceptable.
> > > > > > > > > 
> > > > > > > > > The smp_parse method will automatically fill in any missing values.
> > > > > > > > > 
> > > > > > > > > ie,
> > > > > > > > > 
> > > > > > > > >     -smp 4,cores=1
> > > > > > > > >     -smp cores=1
> > > > > > > > >     -smp threads=1
> > > > > > > > >     -smp sockets=4
> > > > > > > > > 
> > > > > > > > > are all functionally identical to
> > > > > > > > > 
> > > > > > > > >     -smp 4,sockets=4,cores=1,dies=1,threads=1
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > The QEMU man page says this explicitly
> > > > > > > > > 
> > > > > > > > >                    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.
> > > > > > > > It doesn't say how it will compute them though, which for the default
> > > > > > > > smp_parse and for x86 is to prefer sockets over cores over threads.
> > > > > > > > That's not necessarily what the user expects. IMO, we need a 'strict=on'
> > > > > > > > parameter that doesn't allow any collection of smp parameters which
> > > > > > > > require unreasonable assumptions. Reasonable assumptions are threads=1,
> > > > > > > > when threads is not specified and the rest of the math adds up. Also,
> > > > > > > > maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> > > > > > > > as reasonable to decide how to divide cores among sockets or to assume
> > > > > > > > threads=1 when only sockets and cores are given. How do we know the user
> > > > > > > > didn't forget to specify threads if we can't check the math?
> > > > > > > or just outlaw all invalid topologies incl. incomplete by default
> > > > > > > (without requiring extra option), and permit them only for old machine
> > > > > > > types ()using compat machinery) without topo info provided to guest.
> > > > > > > And maybe later deprecate invalid topologies altogether.
> > > > > > This feels like it is creating pain for users to fix a problem that
> > > > > > isn't shown to actually be causing any common issues.
> > > > > > 
> > > > > > We've supposed that users are having problems when forgetting to
> > > > > > specify "threads" and not having the compute value be desirable,
> > > > > > but where are the bug reports to back this up ?
> > > > > > 
> > > > > > The partial topologies are valid and have well defined semantics.
> > > > > > Those semantics may not match everyone's preference, but that
> > > > > > doesn't make them invalid.
> > > > > > 
> > > > > If we adopt the [undocumented] semantics of x86 for arm, then we may
> > > > > surprise some users that expect e.g. '-smp 16' to give them a single
> > > > > socket with 16 cores, because they'll start getting 16 sockets with 1
> > > > > core each. That's because if we don't describe a topology to an arm linux
> > > > > guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
> > > > > prefer we require explicit inputs from users and, if necessary, for them
> > > > > to explicitly opt-in to requiring those explicit inputs.
> > > > Even for x86, defaulting to maximising sockets over cores is sub-optimal.
> > > > In real world x86 hardware it is very rare to have sockets > 2 or 4. For
> > > > large CPU counts, you generally have large cores-per-socket counts on x86.
> > > > 
> > > > The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
> > > > is a fairly arbitrary historical decision.
> > > > 
> > > > It can cause problems with guest OS licensing because both Windows
> > > > and RHEL have been known to charge differently for sockets vs cores,
> > > > with high core counts being cheaper.
> > > > 
> > > > We are not tied into the precise behaviour of the computed topology
> > > > values, as we have no made any promises. All that's required is that
> > > > we keep ABI compat for existing machine types.
> > > If based on this point of view that we haven't made any promises for the
> > > precise behavior of the computed topology, things may get much easier.
> > > I have the following understanding (also a proposal):
> > > 
> > > We will introduce the support for exposing cpu topology since machine
> > > type 6.2 and we will also describe the computed topology for the guest.
> > > We will not make any stricter parsing logic, however the -smp content in
> > > qemu-options.hx should be rearranged to clearly explain how the missing
> > > values will exactly be computed. And this is what QEMU is responsible for.
> > > 
> > > We know that a well designed cpu topology configuration can gain much
> > > benefit for the guest, while a badly designed one will also probably cause
> > > negative impact. But the users should be responsible for the design of the
> > > -smp cmdlines. If they are using an incomplete cmdline for a 6.2 machine,
> > > then they should have known what the computed values will be and that
> > > the computed topology will be exposed to the guest.
> > > > So we could decide to change the computed topology so that it prefers
> > > > high core counts, over sockets, whem using new machine types only.
> > > > That would seem to benefit all arches, by making QEMU more reflective
> > > > of real world CPUs topology.
> > > If we really decide to prefer cores over sockets over threads for new
> > > machine
> > > types, then I think we should also record this change in qemu-option.hx.
> > > 
> > I agree. The proposal sounds good to me. I'd like to hear Eduardo's
> > opinion too (CC'ed).
> > 
> > Thanks,
> > drew
> > 
> > 
> > .
> 

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

On 2021/6/30 16:30, Andrew Jones wrote:
> On Wed, Jun 30, 2021 at 02:36:31PM +0800, wangyanan (Y) wrote:
>> Hi Drew, Igor,
>>
>> I have a question below, hope for some explanation... :)
>>
>> I'm trying to rearrange the smp_parse() helper to make it more scalable.
>> But I wonder why we are currently using maxcpus to calculate the missing
>> sockets while using *cpus* to calculate the missing cores and threads?
>>
>> This makes the following cmdlines work fine,
>> -smp cpus=8,maxcpus=12  <==>  -smp
>> cpus=8,maxcpus=12,sockets=12,cores=1,threads=1
>> -smp cpus=8,maxcpus=12,cores=6  <==>  -smp
>> cpus=8,maxcpus=12,sockets=2,cores=6,threads=1
>> -smp cpus=8,maxcpus=12,threads=2  <==>  -smp
>> cpus=8,maxcpus=12,sockets=6,cores=1,threads=2
>>
>> but the following ones break the invalid CPU topology check:
>> -smp cpus=8,maxcpus=12,sockets=2  <==>  -smp
>> cpus=8,maxcpus=12,sockets=2,cores=4,threads=1
>> -smp cpus=8,maxcpus=12,sockets=4,threads=1  <==>  -smp
>> cpus=8,maxcpus=12,sockets=4,cores=2,threads=1
>> -smp maxcpus=12  <==>  -smp cpus=1,maxcpus=12,sockets=1,cores=1,threads=1
>> -smp maxcpus=12,sockets=2  <==>  -smp
>> cpus=2,maxcpus=12,sockets=2,cores=1,threads=1
>>
>> IMO we should uniformly use maxcpus to calculate the missing sockets
>> also cores and threads, which will allow all the above cmdlines work.
>> Or maybe I missed something? I read the related discussion in [1] but
>> didn't get an unambiguous conclusion.
>>
>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/1535553121-80352-1-git-send-email-imammedo@redhat.com/
> I agree that maxcpus should be used for all calculations.
Thanks. From my view uniformly using maxcpus to calculate the missing
values won't break any existing working cmdlines, but will allow some now
being invalid and incomplete cmdlines to be valid. I will use maxcpus and
test the parser for all possible parameter collections.
> I think we need
> to write -smp parsing from scratch using a set of clean requirements and
> then use the machine compat stuff to switch to it. And also properly
> document it with something like "Since 6.2..."
I agree to rewrite the -smp parsing. But what's the meaning of clean 
requirements?
Sorry I didn't get it.

Thanks,
Yanan
.
>
>> Regards,
>> Yanan
>> .
>>
>> On 2021/6/28 16:58, Andrew Jones wrote:
>>> On Mon, Jun 28, 2021 at 04:43:05PM +0800, wangyanan (Y) wrote:
>>>> Hi,
>>>> On 2021/6/23 1:39, Daniel P. Berrangé wrote:
>>>>> On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
>>>>>> On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
>>>>>>> On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
>>>>>>>> On Tue, 22 Jun 2021 16:29:15 +0200
>>>>>>>> Andrew Jones <drjones@redhat.com> wrote:
>>>>>>>>
>>>>>>>>> On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
>>>>>>>>>> On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
>>>>>>>>>>> Hi Daniel,
>>>>>>>>>>>
>>>>>>>>>>> On 2021/6/22 20:41, Daniel P. Berrangé wrote:
>>>>>>>>>>>> On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
>>>>>>>>>>>>> On 2021/6/22 19:46, Andrew Jones wrote:
>>>>>>>>>>>>>> On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
>>>>>>>>>>>>>>> On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This is v4 of the series [1] that I posted to introduce support for
>>>>>>>>>>>>>>>> generating cpu topology descriptions to guest. Comments are welcome!
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Description:
>>>>>>>>>>>>>>>> Once the view of an accurate virtual cpu topology is provided to guest,
>>>>>>>>>>>>>>>> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
>>>>>>>>>>>>>>>> e.g., the scheduling performance improvement. See Dario Faggioli's
>>>>>>>>>>>>>>>> research and the related performance tests in [2] for reference. So here
>>>>>>>>>>>>>>>> we go, this patch series introduces cpu topology support for ARM platform.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In this series, instead of quietly enforcing the support for the latest
>>>>>>>>>>>>>>>> machine type, a new parameter "expose=on|off" in -smp command line is
>>>>>>>>>>>>>>>> introduced to leave QEMU users a choice to decide whether to enable the
>>>>>>>>>>>>>>>> feature or not. This will allow the feature to work on different machine
>>>>>>>>>>>>>>>> types and also ideally compat with already in-use -smp command lines.
>>>>>>>>>>>>>>>> Also we make much stricter requirement for the topology configuration
>>>>>>>>>>>>>>>> with "expose=on".
>>>>>>>>>>>>>>> Seeing this 'expose=on' parameter feels to me like we're adding a
>>>>>>>>>>>>>>> "make-it-work=yes" parameter. IMHO this is just something that should
>>>>>>>>>>>>>>> be done by default for the current machine type version and beyond.
>>>>>>>>>>>>>>> I don't see the need for a parameter to turnthis on, especially since
>>>>>>>>>>>>>>> it is being made architecture specific.
>>>>>>>>>>>>>> I agree.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yanan, we never discussed an "expose" parameter in the previous versions
>>>>>>>>>>>>>> of this series. We discussed a "strict" parameter though, which would
>>>>>>>>>>>>>> allow existing command lines to "work" using assumptions of what the user
>>>>>>>>>>>>>> meant and strict=on users to get what they mean or an error saying that
>>>>>>>>>>>>>> they asked for something that won't work or would require unreasonable
>>>>>>>>>>>>>> assumptions. Why was this changed to an "expose" parameter?
>>>>>>>>>>>>> Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
>>>>>>>>>>>>> of this series.
>>>>>>>>>>>>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
>>>>>>>>>>>>>
>>>>>>>>>>>>> And in the discussion, we hoped things would work like below with "strict"
>>>>>>>>>>>>> parameter:
>>>>>>>>>>>>> Users who want to describe cpu topology should provide cmdline like
>>>>>>>>>>>>>
>>>>>>>>>>>>> -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>>>>>
>>>>>>>>>>>>> and in this case we require an more accurate -smp configuration and
>>>>>>>>>>>>> then generate the cpu topology description through ACPI/DT.
>>>>>>>>>>>>>
>>>>>>>>>>>>> While without a strict description, no cpu topology description would
>>>>>>>>>>>>> be generated, so they get nothing through ACPI/DT.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It seems to me that the "strict" parameter actually serves as a knob to
>>>>>>>>>>>>> turn on/off the exposure of topology, and this is the reason I changed
>>>>>>>>>>>>> the name.
>>>>>>>>>>>> Yes, the use of 'strict=on' is no better than expose=on IMHO.
>>>>>>>>>>>>
>>>>>>>>>>>> If I give QEMU a cli
>>>>>>>>>>>>
>>>>>>>>>>>>       -smp cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>>>>
>>>>>>>>>>>> then I expect that topology to be exposed to the guest. I shouldn't
>>>>>>>>>>>> have to add extra flags to make that happen.
>>>>>>>>>>>>
>>>>>>>>>>>> Looking at the thread, it seems the concern was around the fact that
>>>>>>>>>>>> the settings were not honoured historically and thus the CLI values
>>>>>>>>>>>> could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
>>>>>>>>>>> This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
>>>>>>>>>>> configuration, and the parsing function already report error for this case.
>>>>>>>>>>>
>>>>>>>>>>> We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
>>>>>>>>>>> for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
>>>>>>>>>>> or "-smp 4, cores=1" are not acceptable any more because we are starting
>>>>>>>>>>> to expose the topology.
>>>>>>>>>> Incomplete specified topologies *are* acceptable.
>>>>>>>>>>
>>>>>>>>>> The smp_parse method will automatically fill in any missing values.
>>>>>>>>>>
>>>>>>>>>> ie,
>>>>>>>>>>
>>>>>>>>>>      -smp 4,cores=1
>>>>>>>>>>      -smp cores=1
>>>>>>>>>>      -smp threads=1
>>>>>>>>>>      -smp sockets=4
>>>>>>>>>>
>>>>>>>>>> are all functionally identical to
>>>>>>>>>>
>>>>>>>>>>      -smp 4,sockets=4,cores=1,dies=1,threads=1
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The QEMU man page says this explicitly
>>>>>>>>>>
>>>>>>>>>>                     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.
>>>>>>>>> It doesn't say how it will compute them though, which for the default
>>>>>>>>> smp_parse and for x86 is to prefer sockets over cores over threads.
>>>>>>>>> That's not necessarily what the user expects. IMO, we need a 'strict=on'
>>>>>>>>> parameter that doesn't allow any collection of smp parameters which
>>>>>>>>> require unreasonable assumptions. Reasonable assumptions are threads=1,
>>>>>>>>> when threads is not specified and the rest of the math adds up. Also,
>>>>>>>>> maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
>>>>>>>>> as reasonable to decide how to divide cores among sockets or to assume
>>>>>>>>> threads=1 when only sockets and cores are given. How do we know the user
>>>>>>>>> didn't forget to specify threads if we can't check the math?
>>>>>>>> or just outlaw all invalid topologies incl. incomplete by default
>>>>>>>> (without requiring extra option), and permit them only for old machine
>>>>>>>> types ()using compat machinery) without topo info provided to guest.
>>>>>>>> And maybe later deprecate invalid topologies altogether.
>>>>>>> This feels like it is creating pain for users to fix a problem that
>>>>>>> isn't shown to actually be causing any common issues.
>>>>>>>
>>>>>>> We've supposed that users are having problems when forgetting to
>>>>>>> specify "threads" and not having the compute value be desirable,
>>>>>>> but where are the bug reports to back this up ?
>>>>>>>
>>>>>>> The partial topologies are valid and have well defined semantics.
>>>>>>> Those semantics may not match everyone's preference, but that
>>>>>>> doesn't make them invalid.
>>>>>>>
>>>>>> If we adopt the [undocumented] semantics of x86 for arm, then we may
>>>>>> surprise some users that expect e.g. '-smp 16' to give them a single
>>>>>> socket with 16 cores, because they'll start getting 16 sockets with 1
>>>>>> core each. That's because if we don't describe a topology to an arm linux
>>>>>> guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
>>>>>> prefer we require explicit inputs from users and, if necessary, for them
>>>>>> to explicitly opt-in to requiring those explicit inputs.
>>>>> Even for x86, defaulting to maximising sockets over cores is sub-optimal.
>>>>> In real world x86 hardware it is very rare to have sockets > 2 or 4. For
>>>>> large CPU counts, you generally have large cores-per-socket counts on x86.
>>>>>
>>>>> The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
>>>>> is a fairly arbitrary historical decision.
>>>>>
>>>>> It can cause problems with guest OS licensing because both Windows
>>>>> and RHEL have been known to charge differently for sockets vs cores,
>>>>> with high core counts being cheaper.
>>>>>
>>>>> We are not tied into the precise behaviour of the computed topology
>>>>> values, as we have no made any promises. All that's required is that
>>>>> we keep ABI compat for existing machine types.
>>>> If based on this point of view that we haven't made any promises for the
>>>> precise behavior of the computed topology, things may get much easier.
>>>> I have the following understanding (also a proposal):
>>>>
>>>> We will introduce the support for exposing cpu topology since machine
>>>> type 6.2 and we will also describe the computed topology for the guest.
>>>> We will not make any stricter parsing logic, however the -smp content in
>>>> qemu-options.hx should be rearranged to clearly explain how the missing
>>>> values will exactly be computed. And this is what QEMU is responsible for.
>>>>
>>>> We know that a well designed cpu topology configuration can gain much
>>>> benefit for the guest, while a badly designed one will also probably cause
>>>> negative impact. But the users should be responsible for the design of the
>>>> -smp cmdlines. If they are using an incomplete cmdline for a 6.2 machine,
>>>> then they should have known what the computed values will be and that
>>>> the computed topology will be exposed to the guest.
>>>>> So we could decide to change the computed topology so that it prefers
>>>>> high core counts, over sockets, whem using new machine types only.
>>>>> That would seem to benefit all arches, by making QEMU more reflective
>>>>> of real world CPUs topology.
>>>> If we really decide to prefer cores over sockets over threads for new
>>>> machine
>>>> types, then I think we should also record this change in qemu-option.hx.
>>>>
>>> I agree. The proposal sounds good to me. I'd like to hear Eduardo's
>>> opinion too (CC'ed).
>>>
>>> Thanks,
>>> drew
>>>
>>>
>>> .
>
> .

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Wed, Jun 30, 2021 at 05:37:42PM +0800, wangyanan (Y) wrote:
> On 2021/6/30 16:30, Andrew Jones wrote:
> > On Wed, Jun 30, 2021 at 02:36:31PM +0800, wangyanan (Y) wrote:
> > > Hi Drew, Igor,
> > > 
> > > I have a question below, hope for some explanation... :)
> > > 
> > > I'm trying to rearrange the smp_parse() helper to make it more scalable.
> > > But I wonder why we are currently using maxcpus to calculate the missing
> > > sockets while using *cpus* to calculate the missing cores and threads?
> > > 
> > > This makes the following cmdlines work fine,
> > > -smp cpus=8,maxcpus=12  <==>  -smp
> > > cpus=8,maxcpus=12,sockets=12,cores=1,threads=1
> > > -smp cpus=8,maxcpus=12,cores=6  <==>  -smp
> > > cpus=8,maxcpus=12,sockets=2,cores=6,threads=1
> > > -smp cpus=8,maxcpus=12,threads=2  <==>  -smp
> > > cpus=8,maxcpus=12,sockets=6,cores=1,threads=2
> > > 
> > > but the following ones break the invalid CPU topology check:
> > > -smp cpus=8,maxcpus=12,sockets=2  <==>  -smp
> > > cpus=8,maxcpus=12,sockets=2,cores=4,threads=1
> > > -smp cpus=8,maxcpus=12,sockets=4,threads=1  <==>  -smp
> > > cpus=8,maxcpus=12,sockets=4,cores=2,threads=1
> > > -smp maxcpus=12  <==>  -smp cpus=1,maxcpus=12,sockets=1,cores=1,threads=1
> > > -smp maxcpus=12,sockets=2  <==>  -smp
> > > cpus=2,maxcpus=12,sockets=2,cores=1,threads=1
> > > 
> > > IMO we should uniformly use maxcpus to calculate the missing sockets
> > > also cores and threads, which will allow all the above cmdlines work.
> > > Or maybe I missed something? I read the related discussion in [1] but
> > > didn't get an unambiguous conclusion.
> > > 
> > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/1535553121-80352-1-git-send-email-imammedo@redhat.com/
> > I agree that maxcpus should be used for all calculations.
> Thanks. From my view uniformly using maxcpus to calculate the missing
> values won't break any existing working cmdlines, but will allow some now
> being invalid and incomplete cmdlines to be valid. I will use maxcpus and
> test the parser for all possible parameter collections.
> > I think we need
> > to write -smp parsing from scratch using a set of clean requirements and
> > then use the machine compat stuff to switch to it. And also properly
> > document it with something like "Since 6.2..."
> I agree to rewrite the -smp parsing. But what's the meaning of clean
> requirements?
> Sorry I didn't get it.

I think -smp evolved without all the details considered up front. Now that
we've considered the details/requirements more completely, then let's
apply our knowledge of them to an implementation that gets them all
covered. Here's a quick list from the top of my head, there might be
some missing 

 - maxcpus should be used for computation of missing values
 - we should assume cores over sockets over threads
 - we should allow extending the topology with arch-specific
   members, such as dies, which will always default to 1 when
   not provided, rather than be computed
 - we should store the results in the smp machine properties

Thanks,
drew

> 
> Thanks,
> Yanan
> .
> > 
> > > Regards,
> > > Yanan
> > > .
> > > 
> > > On 2021/6/28 16:58, Andrew Jones wrote:
> > > > On Mon, Jun 28, 2021 at 04:43:05PM +0800, wangyanan (Y) wrote:
> > > > > Hi,
> > > > > On 2021/6/23 1:39, Daniel P. Berrangé wrote:
> > > > > > On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
> > > > > > > On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
> > > > > > > > On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
> > > > > > > > > On Tue, 22 Jun 2021 16:29:15 +0200
> > > > > > > > > Andrew Jones <drjones@redhat.com> wrote:
> > > > > > > > > 
> > > > > > > > > > On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
> > > > > > > > > > > On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
> > > > > > > > > > > > Hi Daniel,
> > > > > > > > > > > > 
> > > > > > > > > > > > On 2021/6/22 20:41, Daniel P. Berrangé wrote:
> > > > > > > > > > > > > On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
> > > > > > > > > > > > > > On 2021/6/22 19:46, Andrew Jones wrote:
> > > > > > > > > > > > > > > On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
> > > > > > > > > > > > > > > > On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
> > > > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > This is v4 of the series [1] that I posted to introduce support for
> > > > > > > > > > > > > > > > > generating cpu topology descriptions to guest. Comments are welcome!
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > Description:
> > > > > > > > > > > > > > > > > Once the view of an accurate virtual cpu topology is provided to guest,
> > > > > > > > > > > > > > > > > with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> > > > > > > > > > > > > > > > > e.g., the scheduling performance improvement. See Dario Faggioli's
> > > > > > > > > > > > > > > > > research and the related performance tests in [2] for reference. So here
> > > > > > > > > > > > > > > > > we go, this patch series introduces cpu topology support for ARM platform.
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > In this series, instead of quietly enforcing the support for the latest
> > > > > > > > > > > > > > > > > machine type, a new parameter "expose=on|off" in -smp command line is
> > > > > > > > > > > > > > > > > introduced to leave QEMU users a choice to decide whether to enable the
> > > > > > > > > > > > > > > > > feature or not. This will allow the feature to work on different machine
> > > > > > > > > > > > > > > > > types and also ideally compat with already in-use -smp command lines.
> > > > > > > > > > > > > > > > > Also we make much stricter requirement for the topology configuration
> > > > > > > > > > > > > > > > > with "expose=on".
> > > > > > > > > > > > > > > > Seeing this 'expose=on' parameter feels to me like we're adding a
> > > > > > > > > > > > > > > > "make-it-work=yes" parameter. IMHO this is just something that should
> > > > > > > > > > > > > > > > be done by default for the current machine type version and beyond.
> > > > > > > > > > > > > > > > I don't see the need for a parameter to turnthis on, especially since
> > > > > > > > > > > > > > > > it is being made architecture specific.
> > > > > > > > > > > > > > > I agree.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Yanan, we never discussed an "expose" parameter in the previous versions
> > > > > > > > > > > > > > > of this series. We discussed a "strict" parameter though, which would
> > > > > > > > > > > > > > > allow existing command lines to "work" using assumptions of what the user
> > > > > > > > > > > > > > > meant and strict=on users to get what they mean or an error saying that
> > > > > > > > > > > > > > > they asked for something that won't work or would require unreasonable
> > > > > > > > > > > > > > > assumptions. Why was this changed to an "expose" parameter?
> > > > > > > > > > > > > > Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
> > > > > > > > > > > > > > of this series.
> > > > > > > > > > > > > > [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > And in the discussion, we hoped things would work like below with "strict"
> > > > > > > > > > > > > > parameter:
> > > > > > > > > > > > > > Users who want to describe cpu topology should provide cmdline like
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > and in this case we require an more accurate -smp configuration and
> > > > > > > > > > > > > > then generate the cpu topology description through ACPI/DT.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > While without a strict description, no cpu topology description would
> > > > > > > > > > > > > > be generated, so they get nothing through ACPI/DT.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > It seems to me that the "strict" parameter actually serves as a knob to
> > > > > > > > > > > > > > turn on/off the exposure of topology, and this is the reason I changed
> > > > > > > > > > > > > > the name.
> > > > > > > > > > > > > Yes, the use of 'strict=on' is no better than expose=on IMHO.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > If I give QEMU a cli
> > > > > > > > > > > > > 
> > > > > > > > > > > > >       -smp cpus=4,sockets=2,cores=2,threads=1
> > > > > > > > > > > > > 
> > > > > > > > > > > > > then I expect that topology to be exposed to the guest. I shouldn't
> > > > > > > > > > > > > have to add extra flags to make that happen.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Looking at the thread, it seems the concern was around the fact that
> > > > > > > > > > > > > the settings were not honoured historically and thus the CLI values
> > > > > > > > > > > > > could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
> > > > > > > > > > > > This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
> > > > > > > > > > > > configuration, and the parsing function already report error for this case.
> > > > > > > > > > > > 
> > > > > > > > > > > > We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
> > > > > > > > > > > > for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
> > > > > > > > > > > > or "-smp 4, cores=1" are not acceptable any more because we are starting
> > > > > > > > > > > > to expose the topology.
> > > > > > > > > > > Incomplete specified topologies *are* acceptable.
> > > > > > > > > > > 
> > > > > > > > > > > The smp_parse method will automatically fill in any missing values.
> > > > > > > > > > > 
> > > > > > > > > > > ie,
> > > > > > > > > > > 
> > > > > > > > > > >      -smp 4,cores=1
> > > > > > > > > > >      -smp cores=1
> > > > > > > > > > >      -smp threads=1
> > > > > > > > > > >      -smp sockets=4
> > > > > > > > > > > 
> > > > > > > > > > > are all functionally identical to
> > > > > > > > > > > 
> > > > > > > > > > >      -smp 4,sockets=4,cores=1,dies=1,threads=1
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > The QEMU man page says this explicitly
> > > > > > > > > > > 
> > > > > > > > > > >                     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.
> > > > > > > > > > It doesn't say how it will compute them though, which for the default
> > > > > > > > > > smp_parse and for x86 is to prefer sockets over cores over threads.
> > > > > > > > > > That's not necessarily what the user expects. IMO, we need a 'strict=on'
> > > > > > > > > > parameter that doesn't allow any collection of smp parameters which
> > > > > > > > > > require unreasonable assumptions. Reasonable assumptions are threads=1,
> > > > > > > > > > when threads is not specified and the rest of the math adds up. Also,
> > > > > > > > > > maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
> > > > > > > > > > as reasonable to decide how to divide cores among sockets or to assume
> > > > > > > > > > threads=1 when only sockets and cores are given. How do we know the user
> > > > > > > > > > didn't forget to specify threads if we can't check the math?
> > > > > > > > > or just outlaw all invalid topologies incl. incomplete by default
> > > > > > > > > (without requiring extra option), and permit them only for old machine
> > > > > > > > > types ()using compat machinery) without topo info provided to guest.
> > > > > > > > > And maybe later deprecate invalid topologies altogether.
> > > > > > > > This feels like it is creating pain for users to fix a problem that
> > > > > > > > isn't shown to actually be causing any common issues.
> > > > > > > > 
> > > > > > > > We've supposed that users are having problems when forgetting to
> > > > > > > > specify "threads" and not having the compute value be desirable,
> > > > > > > > but where are the bug reports to back this up ?
> > > > > > > > 
> > > > > > > > The partial topologies are valid and have well defined semantics.
> > > > > > > > Those semantics may not match everyone's preference, but that
> > > > > > > > doesn't make them invalid.
> > > > > > > > 
> > > > > > > If we adopt the [undocumented] semantics of x86 for arm, then we may
> > > > > > > surprise some users that expect e.g. '-smp 16' to give them a single
> > > > > > > socket with 16 cores, because they'll start getting 16 sockets with 1
> > > > > > > core each. That's because if we don't describe a topology to an arm linux
> > > > > > > guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
> > > > > > > prefer we require explicit inputs from users and, if necessary, for them
> > > > > > > to explicitly opt-in to requiring those explicit inputs.
> > > > > > Even for x86, defaulting to maximising sockets over cores is sub-optimal.
> > > > > > In real world x86 hardware it is very rare to have sockets > 2 or 4. For
> > > > > > large CPU counts, you generally have large cores-per-socket counts on x86.
> > > > > > 
> > > > > > The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
> > > > > > is a fairly arbitrary historical decision.
> > > > > > 
> > > > > > It can cause problems with guest OS licensing because both Windows
> > > > > > and RHEL have been known to charge differently for sockets vs cores,
> > > > > > with high core counts being cheaper.
> > > > > > 
> > > > > > We are not tied into the precise behaviour of the computed topology
> > > > > > values, as we have no made any promises. All that's required is that
> > > > > > we keep ABI compat for existing machine types.
> > > > > If based on this point of view that we haven't made any promises for the
> > > > > precise behavior of the computed topology, things may get much easier.
> > > > > I have the following understanding (also a proposal):
> > > > > 
> > > > > We will introduce the support for exposing cpu topology since machine
> > > > > type 6.2 and we will also describe the computed topology for the guest.
> > > > > We will not make any stricter parsing logic, however the -smp content in
> > > > > qemu-options.hx should be rearranged to clearly explain how the missing
> > > > > values will exactly be computed. And this is what QEMU is responsible for.
> > > > > 
> > > > > We know that a well designed cpu topology configuration can gain much
> > > > > benefit for the guest, while a badly designed one will also probably cause
> > > > > negative impact. But the users should be responsible for the design of the
> > > > > -smp cmdlines. If they are using an incomplete cmdline for a 6.2 machine,
> > > > > then they should have known what the computed values will be and that
> > > > > the computed topology will be exposed to the guest.
> > > > > > So we could decide to change the computed topology so that it prefers
> > > > > > high core counts, over sockets, whem using new machine types only.
> > > > > > That would seem to benefit all arches, by making QEMU more reflective
> > > > > > of real world CPUs topology.
> > > > > If we really decide to prefer cores over sockets over threads for new
> > > > > machine
> > > > > types, then I think we should also record this change in qemu-option.hx.
> > > > > 
> > > > I agree. The proposal sounds good to me. I'd like to hear Eduardo's
> > > > opinion too (CC'ed).
> > > > 
> > > > Thanks,
> > > > drew
> > > > 
> > > > 
> > > > .
> > 
> > .
> 

Re: [RFC PATCH v4 0/7] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

On 2021/6/30 19:56, Andrew Jones wrote:
> On Wed, Jun 30, 2021 at 05:37:42PM +0800, wangyanan (Y) wrote:
>> On 2021/6/30 16:30, Andrew Jones wrote:
>>> On Wed, Jun 30, 2021 at 02:36:31PM +0800, wangyanan (Y) wrote:
>>>> Hi Drew, Igor,
>>>>
>>>> I have a question below, hope for some explanation... :)
>>>>
>>>> I'm trying to rearrange the smp_parse() helper to make it more scalable.
>>>> But I wonder why we are currently using maxcpus to calculate the missing
>>>> sockets while using *cpus* to calculate the missing cores and threads?
>>>>
>>>> This makes the following cmdlines work fine,
>>>> -smp cpus=8,maxcpus=12  <==>  -smp
>>>> cpus=8,maxcpus=12,sockets=12,cores=1,threads=1
>>>> -smp cpus=8,maxcpus=12,cores=6  <==>  -smp
>>>> cpus=8,maxcpus=12,sockets=2,cores=6,threads=1
>>>> -smp cpus=8,maxcpus=12,threads=2  <==>  -smp
>>>> cpus=8,maxcpus=12,sockets=6,cores=1,threads=2
>>>>
>>>> but the following ones break the invalid CPU topology check:
>>>> -smp cpus=8,maxcpus=12,sockets=2  <==>  -smp
>>>> cpus=8,maxcpus=12,sockets=2,cores=4,threads=1
>>>> -smp cpus=8,maxcpus=12,sockets=4,threads=1  <==>  -smp
>>>> cpus=8,maxcpus=12,sockets=4,cores=2,threads=1
>>>> -smp maxcpus=12  <==>  -smp cpus=1,maxcpus=12,sockets=1,cores=1,threads=1
>>>> -smp maxcpus=12,sockets=2  <==>  -smp
>>>> cpus=2,maxcpus=12,sockets=2,cores=1,threads=1
>>>>
>>>> IMO we should uniformly use maxcpus to calculate the missing sockets
>>>> also cores and threads, which will allow all the above cmdlines work.
>>>> Or maybe I missed something? I read the related discussion in [1] but
>>>> didn't get an unambiguous conclusion.
>>>>
>>>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/1535553121-80352-1-git-send-email-imammedo@redhat.com/
>>> I agree that maxcpus should be used for all calculations.
>> Thanks. From my view uniformly using maxcpus to calculate the missing
>> values won't break any existing working cmdlines, but will allow some now
>> being invalid and incomplete cmdlines to be valid. I will use maxcpus and
>> test the parser for all possible parameter collections.
>>> I think we need
>>> to write -smp parsing from scratch using a set of clean requirements and
>>> then use the machine compat stuff to switch to it. And also properly
>>> document it with something like "Since 6.2..."
>> I agree to rewrite the -smp parsing. But what's the meaning of clean
>> requirements?
>> Sorry I didn't get it.
> I think -smp evolved without all the details considered up front. Now that
> we've considered the details/requirements more completely, then let's
> apply our knowledge of them to an implementation that gets them all
> covered.
Got it now.
> Here's a quick list from the top of my head, there might be
> some missing
>
>   - maxcpus should be used for computation of missing values
>   - we should assume cores over sockets over threads
>   - we should allow extending the topology with arch-specific
>     members, such as dies, which will always default to 1 when
>     not provided, rather than be computed
>   - we should store the results in the smp machine properties
Right! This is a good summary of what we have discussed recently.

Thanks,
Yanan
.
>
>> Thanks,
>> Yanan
>> .
>>>> Regards,
>>>> Yanan
>>>> .
>>>>
>>>> On 2021/6/28 16:58, Andrew Jones wrote:
>>>>> On Mon, Jun 28, 2021 at 04:43:05PM +0800, wangyanan (Y) wrote:
>>>>>> Hi,
>>>>>> On 2021/6/23 1:39, Daniel P. Berrangé wrote:
>>>>>>> On Tue, Jun 22, 2021 at 07:29:34PM +0200, Andrew Jones wrote:
>>>>>>>> On Tue, Jun 22, 2021 at 06:14:25PM +0100, Daniel P. Berrangé wrote:
>>>>>>>>> On Tue, Jun 22, 2021 at 05:40:13PM +0200, Igor Mammedov wrote:
>>>>>>>>>> On Tue, 22 Jun 2021 16:29:15 +0200
>>>>>>>>>> Andrew Jones <drjones@redhat.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> On Tue, Jun 22, 2021 at 03:10:57PM +0100, Daniel P. Berrangé wrote:
>>>>>>>>>>>> On Tue, Jun 22, 2021 at 10:04:52PM +0800, wangyanan (Y) wrote:
>>>>>>>>>>>>> Hi Daniel,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2021/6/22 20:41, Daniel P. Berrangé wrote:
>>>>>>>>>>>>>> On Tue, Jun 22, 2021 at 08:31:22PM +0800, wangyanan (Y) wrote:
>>>>>>>>>>>>>>> On 2021/6/22 19:46, Andrew Jones wrote:
>>>>>>>>>>>>>>>> On Tue, Jun 22, 2021 at 11:18:09AM +0100, Daniel P. Berrangé wrote:
>>>>>>>>>>>>>>>>> On Tue, Jun 22, 2021 at 05:34:06PM +0800, Yanan Wang wrote:
>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> This is v4 of the series [1] that I posted to introduce support for
>>>>>>>>>>>>>>>>>> generating cpu topology descriptions to guest. Comments are welcome!
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Description:
>>>>>>>>>>>>>>>>>> Once the view of an accurate virtual cpu topology is provided to guest,
>>>>>>>>>>>>>>>>>> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
>>>>>>>>>>>>>>>>>> e.g., the scheduling performance improvement. See Dario Faggioli's
>>>>>>>>>>>>>>>>>> research and the related performance tests in [2] for reference. So here
>>>>>>>>>>>>>>>>>> we go, this patch series introduces cpu topology support for ARM platform.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> In this series, instead of quietly enforcing the support for the latest
>>>>>>>>>>>>>>>>>> machine type, a new parameter "expose=on|off" in -smp command line is
>>>>>>>>>>>>>>>>>> introduced to leave QEMU users a choice to decide whether to enable the
>>>>>>>>>>>>>>>>>> feature or not. This will allow the feature to work on different machine
>>>>>>>>>>>>>>>>>> types and also ideally compat with already in-use -smp command lines.
>>>>>>>>>>>>>>>>>> Also we make much stricter requirement for the topology configuration
>>>>>>>>>>>>>>>>>> with "expose=on".
>>>>>>>>>>>>>>>>> Seeing this 'expose=on' parameter feels to me like we're adding a
>>>>>>>>>>>>>>>>> "make-it-work=yes" parameter. IMHO this is just something that should
>>>>>>>>>>>>>>>>> be done by default for the current machine type version and beyond.
>>>>>>>>>>>>>>>>> I don't see the need for a parameter to turnthis on, especially since
>>>>>>>>>>>>>>>>> it is being made architecture specific.
>>>>>>>>>>>>>>>> I agree.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yanan, we never discussed an "expose" parameter in the previous versions
>>>>>>>>>>>>>>>> of this series. We discussed a "strict" parameter though, which would
>>>>>>>>>>>>>>>> allow existing command lines to "work" using assumptions of what the user
>>>>>>>>>>>>>>>> meant and strict=on users to get what they mean or an error saying that
>>>>>>>>>>>>>>>> they asked for something that won't work or would require unreasonable
>>>>>>>>>>>>>>>> assumptions. Why was this changed to an "expose" parameter?
>>>>>>>>>>>>>>> Yes, we indeed discuss a new "strict" parameter but not a "expose" in v2 [1]
>>>>>>>>>>>>>>> of this series.
>>>>>>>>>>>>>>> [1] https://patchwork.kernel.org/project/qemu-devel/patch/20210413080745.33004-6-wangyanan55@huawei.com/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> And in the discussion, we hoped things would work like below with "strict"
>>>>>>>>>>>>>>> parameter:
>>>>>>>>>>>>>>> Users who want to describe cpu topology should provide cmdline like
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -smp strict=on,cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> and in this case we require an more accurate -smp configuration and
>>>>>>>>>>>>>>> then generate the cpu topology description through ACPI/DT.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> While without a strict description, no cpu topology description would
>>>>>>>>>>>>>>> be generated, so they get nothing through ACPI/DT.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It seems to me that the "strict" parameter actually serves as a knob to
>>>>>>>>>>>>>>> turn on/off the exposure of topology, and this is the reason I changed
>>>>>>>>>>>>>>> the name.
>>>>>>>>>>>>>> Yes, the use of 'strict=on' is no better than expose=on IMHO.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If I give QEMU a cli
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>        -smp cpus=4,sockets=2,cores=2,threads=1
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> then I expect that topology to be exposed to the guest. I shouldn't
>>>>>>>>>>>>>> have to add extra flags to make that happen.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Looking at the thread, it seems the concern was around the fact that
>>>>>>>>>>>>>> the settings were not honoured historically and thus the CLI values
>>>>>>>>>>>>>> could be garbage. ie  -smp cpus=4,sockets=8,cores=3,thread=9
>>>>>>>>>>>>> This "-smp cpus=4,sockets=8,cores=3,threads=9" behaviors as a wrong
>>>>>>>>>>>>> configuration, and the parsing function already report error for this case.
>>>>>>>>>>>>>
>>>>>>>>>>>>> We hope more complete config like "-smp 4,sockets=2,cores=2,threads=1"
>>>>>>>>>>>>> for exposure of topology, and the incomplete ones like "-smp 4,sockets=1"
>>>>>>>>>>>>> or "-smp 4, cores=1" are not acceptable any more because we are starting
>>>>>>>>>>>>> to expose the topology.
>>>>>>>>>>>> Incomplete specified topologies *are* acceptable.
>>>>>>>>>>>>
>>>>>>>>>>>> The smp_parse method will automatically fill in any missing values.
>>>>>>>>>>>>
>>>>>>>>>>>> ie,
>>>>>>>>>>>>
>>>>>>>>>>>>       -smp 4,cores=1
>>>>>>>>>>>>       -smp cores=1
>>>>>>>>>>>>       -smp threads=1
>>>>>>>>>>>>       -smp sockets=4
>>>>>>>>>>>>
>>>>>>>>>>>> are all functionally identical to
>>>>>>>>>>>>
>>>>>>>>>>>>       -smp 4,sockets=4,cores=1,dies=1,threads=1
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> The QEMU man page says this explicitly
>>>>>>>>>>>>
>>>>>>>>>>>>                      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.
>>>>>>>>>>> It doesn't say how it will compute them though, which for the default
>>>>>>>>>>> smp_parse and for x86 is to prefer sockets over cores over threads.
>>>>>>>>>>> That's not necessarily what the user expects. IMO, we need a 'strict=on'
>>>>>>>>>>> parameter that doesn't allow any collection of smp parameters which
>>>>>>>>>>> require unreasonable assumptions. Reasonable assumptions are threads=1,
>>>>>>>>>>> when threads is not specified and the rest of the math adds up. Also,
>>>>>>>>>>> maxcpus == cpus when maxcpus isn't specified is reasonable. But, it's not
>>>>>>>>>>> as reasonable to decide how to divide cores among sockets or to assume
>>>>>>>>>>> threads=1 when only sockets and cores are given. How do we know the user
>>>>>>>>>>> didn't forget to specify threads if we can't check the math?
>>>>>>>>>> or just outlaw all invalid topologies incl. incomplete by default
>>>>>>>>>> (without requiring extra option), and permit them only for old machine
>>>>>>>>>> types ()using compat machinery) without topo info provided to guest.
>>>>>>>>>> And maybe later deprecate invalid topologies altogether.
>>>>>>>>> This feels like it is creating pain for users to fix a problem that
>>>>>>>>> isn't shown to actually be causing any common issues.
>>>>>>>>>
>>>>>>>>> We've supposed that users are having problems when forgetting to
>>>>>>>>> specify "threads" and not having the compute value be desirable,
>>>>>>>>> but where are the bug reports to back this up ?
>>>>>>>>>
>>>>>>>>> The partial topologies are valid and have well defined semantics.
>>>>>>>>> Those semantics may not match everyone's preference, but that
>>>>>>>>> doesn't make them invalid.
>>>>>>>>>
>>>>>>>> If we adopt the [undocumented] semantics of x86 for arm, then we may
>>>>>>>> surprise some users that expect e.g. '-smp 16' to give them a single
>>>>>>>> socket with 16 cores, because they'll start getting 16 sockets with 1
>>>>>>>> core each. That's because if we don't describe a topology to an arm linux
>>>>>>>> guest then it assumes cores. Maybe we shouldn't worry about this, but I'd
>>>>>>>> prefer we require explicit inputs from users and, if necessary, for them
>>>>>>>> to explicitly opt-in to requiring those explicit inputs.
>>>>>>> Even for x86, defaulting to maximising sockets over cores is sub-optimal.
>>>>>>> In real world x86 hardware it is very rare to have sockets > 2 or 4. For
>>>>>>> large CPU counts, you generally have large cores-per-socket counts on x86.
>>>>>>>
>>>>>>> The QEMU preference for sockets over cores on x86 (and PPC too IIUC)
>>>>>>> is a fairly arbitrary historical decision.
>>>>>>>
>>>>>>> It can cause problems with guest OS licensing because both Windows
>>>>>>> and RHEL have been known to charge differently for sockets vs cores,
>>>>>>> with high core counts being cheaper.
>>>>>>>
>>>>>>> We are not tied into the precise behaviour of the computed topology
>>>>>>> values, as we have no made any promises. All that's required is that
>>>>>>> we keep ABI compat for existing machine types.
>>>>>> If based on this point of view that we haven't made any promises for the
>>>>>> precise behavior of the computed topology, things may get much easier.
>>>>>> I have the following understanding (also a proposal):
>>>>>>
>>>>>> We will introduce the support for exposing cpu topology since machine
>>>>>> type 6.2 and we will also describe the computed topology for the guest.
>>>>>> We will not make any stricter parsing logic, however the -smp content in
>>>>>> qemu-options.hx should be rearranged to clearly explain how the missing
>>>>>> values will exactly be computed. And this is what QEMU is responsible for.
>>>>>>
>>>>>> We know that a well designed cpu topology configuration can gain much
>>>>>> benefit for the guest, while a badly designed one will also probably cause
>>>>>> negative impact. But the users should be responsible for the design of the
>>>>>> -smp cmdlines. If they are using an incomplete cmdline for a 6.2 machine,
>>>>>> then they should have known what the computed values will be and that
>>>>>> the computed topology will be exposed to the guest.
>>>>>>> So we could decide to change the computed topology so that it prefers
>>>>>>> high core counts, over sockets, whem using new machine types only.
>>>>>>> That would seem to benefit all arches, by making QEMU more reflective
>>>>>>> of real world CPUs topology.
>>>>>> If we really decide to prefer cores over sockets over threads for new
>>>>>> machine
>>>>>> types, then I think we should also record this change in qemu-option.hx.
>>>>>>
>>>>> I agree. The proposal sounds good to me. I'd like to hear Eduardo's
>>>>> opinion too (CC'ed).
>>>>>
>>>>> Thanks,
>>>>> drew
>>>>>
>>>>>
>>>>> .
>>> .
>
> .