| Message ID | 20220420104909.233058-3-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Fix CPU's default NUMA node ID | expand |
Hi Gavin, Sorry I missed the v6. On 2022/4/20 18:49, Gavin Shan wrote: > Currently, the SMP configuration isn't considered when the CPU > topology is populated. In this case, it's impossible to provide > the default CPU-to-NUMA mapping or association based on the socket > ID of the given CPU. > > This takes account of SMP configuration when the CPU topology > is populated. The die ID for the given CPU isn't assigned since > it's not supported on arm/virt machine. Besides, the used SMP > configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted > to avoid testing failure > > Signed-off-by: Gavin Shan <gshan@redhat.com> > Reviewed-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/arm/virt.c | 15 ++++++++++++++- > tests/qtest/numa-test.c | 3 ++- > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index d2e5ecd234..5443ecae92 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) > int n; > unsigned int max_cpus = ms->smp.max_cpus; > VirtMachineState *vms = VIRT_MACHINE(ms); > + MachineClass *mc = MACHINE_GET_CLASS(vms); > > if (ms->possible_cpus) { > assert(ms->possible_cpus->len == max_cpus); > @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) > ms->possible_cpus->cpus[n].type = ms->cpu_type; > ms->possible_cpus->cpus[n].arch_id = > virt_cpu_mp_affinity(vms, n); > + > + assert(!mc->smp_props.dies_supported); > + ms->possible_cpus->cpus[n].props.has_socket_id = true; > + ms->possible_cpus->cpus[n].props.socket_id = > + (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads)); nit: so the outermost "()" is unnecessary too. > + ms->possible_cpus->cpus[n].props.has_cluster_id = true; > + ms->possible_cpus->cpus[n].props.cluster_id = > + (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters; > + ms->possible_cpus->cpus[n].props.has_core_id = true; > + ms->possible_cpus->cpus[n].props.core_id = > + (n / ms->smp.threads) % ms->smp.cores; > ms->possible_cpus->cpus[n].props.has_thread_id = true; > - ms->possible_cpus->cpus[n].props.thread_id = n; > + ms->possible_cpus->cpus[n].props.thread_id = > + n % ms->smp.threads; > } > return ms->possible_cpus; > } > diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c > index 90bf68a5b3..aeda8c774c 100644 > --- a/tests/qtest/numa-test.c > +++ b/tests/qtest/numa-test.c > @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data) > QTestState *qts; > g_autofree char *cli = NULL; > > - cli = make_cli(data, "-machine smp.cpus=2 " > + cli = make_cli(data, "-machine " > + "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 " > "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 " > "-numa cpu,node-id=1,thread-id=0 " > "-numa cpu,node-id=0,thread-id=1"); Thanks, Yanan
Hi Yanan, On 4/21/22 7:50 PM, wangyanan (Y) wrote: > Hi Gavin, > Sorry I missed the v6. No problem at all. thanks for your review again :) > On 2022/4/20 18:49, Gavin Shan wrote: >> Currently, the SMP configuration isn't considered when the CPU >> topology is populated. In this case, it's impossible to provide >> the default CPU-to-NUMA mapping or association based on the socket >> ID of the given CPU. >> >> This takes account of SMP configuration when the CPU topology >> is populated. The die ID for the given CPU isn't assigned since >> it's not supported on arm/virt machine. Besides, the used SMP >> configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted >> to avoid testing failure >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> Reviewed-by: Yanan Wang <wangyanan55@huawei.com> >> --- >> hw/arm/virt.c | 15 ++++++++++++++- >> tests/qtest/numa-test.c | 3 ++- >> 2 files changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index d2e5ecd234..5443ecae92 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) >> int n; >> unsigned int max_cpus = ms->smp.max_cpus; >> VirtMachineState *vms = VIRT_MACHINE(ms); >> + MachineClass *mc = MACHINE_GET_CLASS(vms); >> if (ms->possible_cpus) { >> assert(ms->possible_cpus->len == max_cpus); >> @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) >> ms->possible_cpus->cpus[n].type = ms->cpu_type; >> ms->possible_cpus->cpus[n].arch_id = >> virt_cpu_mp_affinity(vms, n); >> + >> + assert(!mc->smp_props.dies_supported); >> + ms->possible_cpus->cpus[n].props.has_socket_id = true; >> + ms->possible_cpus->cpus[n].props.socket_id = >> + (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads)); > nit: so the outermost "()" is unnecessary too. It was kept by intention so that it has same style as to other fields like cluster_id. I will remove it in v8 and it doesn't matter actually. >> + ms->possible_cpus->cpus[n].props.has_cluster_id = true; >> + ms->possible_cpus->cpus[n].props.cluster_id = >> + (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters; >> + ms->possible_cpus->cpus[n].props.has_core_id = true; >> + ms->possible_cpus->cpus[n].props.core_id = >> + (n / ms->smp.threads) % ms->smp.cores; >> ms->possible_cpus->cpus[n].props.has_thread_id = true; >> - ms->possible_cpus->cpus[n].props.thread_id = n; >> + ms->possible_cpus->cpus[n].props.thread_id = >> + n % ms->smp.threads; >> } >> return ms->possible_cpus; >> } >> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c >> index 90bf68a5b3..aeda8c774c 100644 >> --- a/tests/qtest/numa-test.c >> +++ b/tests/qtest/numa-test.c >> @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data) >> QTestState *qts; >> g_autofree char *cli = NULL; >> - cli = make_cli(data, "-machine smp.cpus=2 " >> + cli = make_cli(data, "-machine " >> + "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 " >> "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 " >> "-numa cpu,node-id=1,thread-id=0 " >> "-numa cpu,node-id=0,thread-id=1"); As discussed with Igor, the changes to test/qtest/numa-test.c will be split into a separate patch in v8, which goes before this one. I assume your reviewed-by tag is still valid, even for the separate patch. Thanks, Gavin
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index d2e5ecd234..5443ecae92 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2505,6 +2505,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) int n; unsigned int max_cpus = ms->smp.max_cpus; VirtMachineState *vms = VIRT_MACHINE(ms); + MachineClass *mc = MACHINE_GET_CLASS(vms); if (ms->possible_cpus) { assert(ms->possible_cpus->len == max_cpus); @@ -2518,8 +2519,20 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) ms->possible_cpus->cpus[n].type = ms->cpu_type; ms->possible_cpus->cpus[n].arch_id = virt_cpu_mp_affinity(vms, n); + + assert(!mc->smp_props.dies_supported); + ms->possible_cpus->cpus[n].props.has_socket_id = true; + ms->possible_cpus->cpus[n].props.socket_id = + (n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads)); + ms->possible_cpus->cpus[n].props.has_cluster_id = true; + ms->possible_cpus->cpus[n].props.cluster_id = + (n / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters; + ms->possible_cpus->cpus[n].props.has_core_id = true; + ms->possible_cpus->cpus[n].props.core_id = + (n / ms->smp.threads) % ms->smp.cores; ms->possible_cpus->cpus[n].props.has_thread_id = true; - ms->possible_cpus->cpus[n].props.thread_id = n; + ms->possible_cpus->cpus[n].props.thread_id = + n % ms->smp.threads; } return ms->possible_cpus; } diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c index 90bf68a5b3..aeda8c774c 100644 --- a/tests/qtest/numa-test.c +++ b/tests/qtest/numa-test.c @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data) QTestState *qts; g_autofree char *cli = NULL; - cli = make_cli(data, "-machine smp.cpus=2 " + cli = make_cli(data, "-machine " + "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 " "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 " "-numa cpu,node-id=1,thread-id=0 " "-numa cpu,node-id=0,thread-id=1");