qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH v3 4/4] hw/arm/virt: Parse -smp cluster parameter in virt


From: wangyanan (Y)
Subject: Re: [RFC PATCH v3 4/4] hw/arm/virt: Parse -smp cluster parameter in virt_smp_parse
Date: Tue, 18 May 2021 11:48:34 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0

Hi Salil,

On 2021/5/17 23:17, Salil Mehta wrote:
From: Qemu-devel
[mailto:qemu-devel-bounces+salil.mehta=huawei.com@nongnu.org] On Behalf Of
Yanan Wang
Sent: Sunday, May 16, 2021 11:32 AM
To: Peter Maydell <peter.maydell@linaro.org>; Paolo Bonzini
<pbonzini@redhat.com>; Andrew Jones <drjones@redhat.com>; Michael S . Tsirkin
<mst@redhat.com>; Igor Mammedov <imammedo@redhat.com>; Shannon Zhao
<shannon.zhaosl@gmail.com>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Philippe
Mathieu-Daudé <philmd@redhat.com>; wangyanan (Y) <wangyanan55@huawei.com>;
Zengtao (B) <prime.zeng@hisilicon.com>; Wanghaibin (D)
<wanghaibin.wang@huawei.com>; yuzenghui <yuzenghui@huawei.com>; yangyicong
<yangyicong@huawei.com>; zhukeqian <zhukeqian1@huawei.com>
Subject: [RFC PATCH v3 4/4] hw/arm/virt: Parse -smp cluster parameter in
virt_smp_parse

There is a separate function virt_smp_parse() in hw/virt/arm.c used
to parse cpu topology for the ARM machines. So add parsing of -smp
cluster parameter in it, then total number of logical cpus will be
calculated like: max_cpus = sockets * clusters * cores * threads.

Note, we will assume multi-cluster in one socket is not supported
and default the value of clusters to 1, if it's not explicitly
specified in -smp cmdline.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
  hw/arm/virt.c | 32 ++++++++++++++++++--------------
  1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7de822e491..678d5ef36c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2642,8 +2642,8 @@ static int virt_kvm_type(MachineState *ms, const char
*type_str)
   * with the -smp cmdlines when parsing them.
   *
   * We require that at least one of cpus or maxcpus must be provided.
- * Threads will default to 1 if not provided. Sockets and cores must
- * be either both provided or both not.
+ * Clusters and threads will default to 1 if they are not provided.
+ * Sockets and cores must be either both provided or both not.
   *
   * Note, if neither sockets nor cores are specified, we will calculate
   * all the missing values just like smp_parse() does, but will disable
@@ -2652,15 +2652,18 @@ static int virt_kvm_type(MachineState *ms, const char
*type_str)
  static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
  {
      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms);
+    VirtMachineState *vms = VIRT_MACHINE(ms);

      if (opts) {
          unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
          unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0);
          unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
+        unsigned clusters = qemu_opt_get_number(opts, "clusters", 0);
          unsigned cores = qemu_opt_get_number(opts, "cores", 0);
          unsigned threads = qemu_opt_get_number(opts, "threads", 0);

-        /* Default threads to 1 if not provided */
+        /* Default clusters and threads to 1 if not provided */
+        clusters = clusters > 0 ? clusters : 1;
          threads = threads > 0 ? threads : 1;

          if (cpus == 0 && maxcpus == 0) {
@@ -2676,13 +2679,13 @@ static void virt_smp_parse(MachineState *ms, QemuOpts
*opts)
              cores = 1;
              if (cpus == 0) {
                  sockets = 1;
-                cpus = sockets * cores * threads;
+                cpus = sockets * clusters * cores * threads;
              } else {
                  maxcpus = maxcpus > 0 ? maxcpus : cpus;
-                sockets = maxcpus / (cores * threads);
+                sockets = maxcpus / (clusters * cores * threads);
              }
          } else if (sockets > 0 && cores > 0) {
-            cpus = cpus > 0 ? cpus : sockets * cores * threads;
+            cpus = cpus > 0 ? cpus : sockets * clusters * cores * threads;
              maxcpus = maxcpus > 0 ? maxcpus : cpus;
          } else {
              error_report("sockets and cores must be both provided "
@@ -2695,25 +2698,26 @@ static void virt_smp_parse(MachineState *ms, QemuOpts
*opts)
              exit(1);
          }

-        if (sockets * cores * threads < cpus) {
+        if (sockets * clusters * cores * threads < cpus) {
              error_report("cpu topology: "
-                         "sockets (%u) * cores (%u) * threads (%u) < "
-                         "smp_cpus (%u)",
-                         sockets, cores, threads, cpus);
+                         "sockets (%u) * clusters (%u) * cores (%u) * "
+                         "threads (%u) < smp_cpus (%u)",
+                         sockets, clusters, cores, threads, cpus);
              exit(1);
          }

-        if (sockets * cores * threads != maxcpus) {
+        if (sockets * clusters * cores * threads != maxcpus) {
              error_report("cpu topology: "
-                         "sockets (%u) * cores (%u) * threads (%u) "
-                         "!= maxcpus (%u)",
-                         sockets, cores, threads, maxcpus);
+                         "sockets (%u) * clusters (%u) * cores (%u) * "
+                         "threads (%u) != maxcpus (%u)",
+                         sockets, clusters, cores, threads, maxcpus);
              exit(1);
          }

          ms->smp.cpus = cpus;
          ms->smp.max_cpus = maxcpus;
          ms->smp.sockets = sockets;
+        vms->smp_clusters = clusters;

This variable naming *smp_clusters* looks out-of-sorts. I thought a similar
variable *smp_cpus* was destined to be removed for the reason given in below
link - a patch by Andrew Jones?

Link: https://lists.gnu.org/archive/html/qemu-arm/2020-12/msg00418.html

Am I missing anything here?
The smp_clusters is added in VirtMachineState and nowhere else because
it's currently only used for ARM. But I think maybe I should also move it to
CpuTopology structure like [1] is doing to move dies to CpuTopology.

Move clusters to CpuTopology won't affect other architectures that don't
support it yet, and will also make it easy if they want to in the future.

[1] From Paolo:
20210513162901.1310239-10-pbonzini@redhat.com/">https://patchwork.kernel.org/project/qemu-devel/patch/20210513162901.1310239-10-pbonzini@redhat.com/

Thanks,
Yanan
Salil.

          ms->smp.cores = cores;
          ms->smp.threads = threads;
      }
.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]