| Message ID | 20210516103228.37792-5-wangyanan55@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Introduce cluster cpu topology support | expand |
On Sun, May 16, 2021 at 06:32:28PM +0800, Yanan Wang wrote: > 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; > ms->smp.cores = cores; > ms->smp.threads = threads; > } > -- > 2.19.1 > After reworking "[RFC PATCH v3 9/9] hw/arm/virt: Add separate -smp parsing function for ARM machines", this should also be reworked and fully tested, possibly using a standalone test, as as I suggested in the other review. Thanks, drew
Hi Drew, On 2021/5/17 17:12, Andrew Jones wrote: > On Sun, May 16, 2021 at 06:32:28PM +0800, Yanan Wang wrote: >> 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; >> ms->smp.cores = cores; >> ms->smp.threads = threads; >> } >> -- >> 2.19.1 >> > After reworking "[RFC PATCH v3 9/9] hw/arm/virt: Add separate -smp parsing > function for ARM machines", this should also be reworked and fully tested, > possibly using a standalone test, as as I suggested in the other review. Ok, I will make full test. Thanks, Yanan > Thanks, > drew > > .
> 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? Salil. > ms->smp.cores = cores; > ms->smp.threads = threads; > }
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: 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; >> } > .
> From: wangyanan (Y) > > 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. yes, that’s the idea. It is always good to have right place holders so that the code comprehension/usage(in this case) becomes easy and obvious. > > 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: > https://patchwork.kernel.org/project/qemu-devel/patch/20210513162901.131023 > 9-10-pbonzini@redhat.com/ sure. > > Thanks, > Yanan > > Salil. > > > >> ms->smp.cores = cores; > >> ms->smp.threads = threads; > >> }
On Tue, May 18, 2021 at 11:48:34AM +0800, wangyanan (Y) wrote: > 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. Yes, please do that. Thanks, drew > > 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: > 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; > > > } > > . >
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; ms->smp.cores = cores; ms->smp.threads = threads; }
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(-)