| Message ID | 20220403145953.10522-3-gshan@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Fix CPU's default NUMA node ID | expand |
On Sun, Apr 03, 2022 at 10:59:51PM +0800, 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 yet. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > hw/arm/virt.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets; > + 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; Does this need to be conditionalized d behind a machine property, so that we don't change behaviour of existing machine type versions ? With regards, Daniel
Hi Daniel, On 4/4/22 4:39 PM, Daniel P. Berrangé wrote: > On Sun, Apr 03, 2022 at 10:59:51PM +0800, 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 yet. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> hw/arm/virt.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets; >> + 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; > > Does this need to be conditionalized d behind a machine property, so that > we don't change behaviour of existing machine type versions ? > I think we probably needn't to do that because the default NUMA node for the given CPU is returned based on the socket ID in next patch. The socket ID is calculated in this patch. Otherwise, we will see CPU topology broken warnings in Linux guest. I think we need to fix this issue for all machine type versions. Thanks, Gavin
On Mon, 4 Apr 2022 18:48:00 +0800 Gavin Shan <gshan@redhat.com> wrote: > Hi Daniel, > > On 4/4/22 4:39 PM, Daniel P. Berrangé wrote: > > On Sun, Apr 03, 2022 at 10:59:51PM +0800, 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 yet. > >> > >> Signed-off-by: Gavin Shan <gshan@redhat.com> > >> --- > >> hw/arm/virt.c | 16 +++++++++++++++- > >> 1 file changed, 15 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c > >> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets; > >> + 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; > > > > Does this need to be conditionalized d behind a machine property, so that > > we don't change behaviour of existing machine type versions ? > > > > I think we probably needn't to do that because the default NUMA node > for the given CPU is returned based on the socket ID in next patch. > The socket ID is calculated in this patch. Otherwise, we will see > CPU topology broken warnings in Linux guest. I think we need to fix > this issue for all machine type versions. Agreed. Also guest-wise it's ACPI only change, which is 'firmware' part of QEMU, and by default we don't to version those changes unless we a pressed into it (i.e the same policy that goes for bundled firmware) > Thanks, > Gavin >
Hi Yanan, On 4/13/22 8:39 PM, wangyanan (Y) wrote: > On 2022/4/3 22:59, 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 yet. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> hw/arm/virt.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets; > No need for "% ms->smp.sockets". Yeah, lets remove it in v6. >> + 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; >> } > Otherwise, looks good to me: > Reviewed-by: Yanan Wang <wangyanan55@huawei.com> > Thanks for your time to review :) Thanks, Gavin
Hi Yanan, On 4/14/22 10:27 AM, wangyanan (Y) wrote: > On 2022/4/14 8:08, Gavin Shan wrote: >> On 4/13/22 8:39 PM, wangyanan (Y) wrote: >>> On 2022/4/3 22:59, 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 yet. >>>> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>>> --- >>>> hw/arm/virt.c | 16 +++++++++++++++- >>>> 1 file changed, 15 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets; >>> No need for "% ms->smp.sockets". >> >> Yeah, lets remove it in v6. >> >>>> + 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; >>>> } >>> Otherwise, looks good to me: >>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com> >>> >> >> Thanks for your time to review :) >> >> > Oh, after further testing this patch breaks numa-test for aarch64, > which should be checked and fixed. I guess it's because we have > more IDs supported for ARM. We have to fully running the QEMU > tests before sending some patches to ensure that they are not > breaking anything. :) > Thanks for catching the failure and reporting back. I'm not too much familar with QEMU's test workframe. Could you please share the detailed commands to reproduce the failure? I will fix in v6, which will be done in a separate patch :) Thanks, Gavin
Hi Yanan, On 4/14/22 10:49 AM, wangyanan (Y) wrote: > On 2022/4/14 10:37, Gavin Shan wrote: >> On 4/14/22 10:27 AM, wangyanan (Y) wrote: >>> On 2022/4/14 8:08, Gavin Shan wrote: >>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote: >>>>> On 2022/4/3 22:59, 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 yet. >>>>>> >>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>>>>> --- >>>>>> hw/arm/virt.c | 16 +++++++++++++++- >>>>>> 1 file changed, 15 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets; >>>>> No need for "% ms->smp.sockets". >>>> >>>> Yeah, lets remove it in v6. >>>> >>>>>> + 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; >>>>>> } >>>>> Otherwise, looks good to me: >>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com> >>>>> >>>> >>>> Thanks for your time to review :) >>>> >>>> >>> Oh, after further testing this patch breaks numa-test for aarch64, >>> which should be checked and fixed. I guess it's because we have >>> more IDs supported for ARM. We have to fully running the QEMU >>> tests before sending some patches to ensure that they are not >>> breaking anything. :) >>> >> >> Thanks for catching the failure and reporting back. I'm not >> too much familar with QEMU's test workframe. Could you please >> share the detailed commands to reproduce the failure? I will >> fix in v6, which will be done in a separate patch :) >> > There is a reference link: https://wiki.qemu.org/Testing > To catch the failure of this patch: "make check" will be enough. > Thanks for the pointer. The issue is caused by ms->possible_cpus->cpus[n].props.thread_id. Before this patch, it's value of [0 ... smp.cpus]. However, it's always zero after this patch is applied because '%' operation is applied for the thread ID. What we need to do is to specify SMP configuration for the test case. I will add PATCH[v6 5/5] for it. diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c index 90bf68a5b3..6178ac21a5 100644 --- a/tests/qtest/numa-test.c +++ b/tests/qtest/numa-test.c @@ -223,7 +223,7 @@ 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.cores=1,smp.threads=2 " Thanks, Gavin
Hi Yanan, On 4/14/22 5:29 PM, wangyanan (Y) wrote: > On 2022/4/14 15:35, Gavin Shan wrote: >> On 4/14/22 10:49 AM, wangyanan (Y) wrote: >>> On 2022/4/14 10:37, Gavin Shan wrote: >>>> On 4/14/22 10:27 AM, wangyanan (Y) wrote: >>>>> On 2022/4/14 8:08, Gavin Shan wrote: >>>>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote: >>>>>>> On 2022/4/3 22:59, 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 yet. >>>>>>>> >>>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>>>>>>> --- >>>>>>>> hw/arm/virt.c | 16 +++++++++++++++- >>>>>>>> 1 file changed, 15 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>>>>>> index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets; >>>>>>> No need for "% ms->smp.sockets". >>>>>> >>>>>> Yeah, lets remove it in v6. >>>>>> >>>>>>>> + 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; >>>>>>>> } >>>>>>> Otherwise, looks good to me: >>>>>>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com> >>>>>>> >>>>>> >>>>>> Thanks for your time to review :) >>>>>> >>>>>> >>>>> Oh, after further testing this patch breaks numa-test for aarch64, >>>>> which should be checked and fixed. I guess it's because we have >>>>> more IDs supported for ARM. We have to fully running the QEMU >>>>> tests before sending some patches to ensure that they are not >>>>> breaking anything. :) >>>>> >>>> >>>> Thanks for catching the failure and reporting back. I'm not >>>> too much familar with QEMU's test workframe. Could you please >>>> share the detailed commands to reproduce the failure? I will >>>> fix in v6, which will be done in a separate patch :) >>>> >>> There is a reference link: https://wiki.qemu.org/Testing >>> To catch the failure of this patch: "make check" will be enough. >>> >> >> Thanks for the pointer. The issue is caused by ms->possible_cpus->cpus[n].props.thread_id. >> Before this patch, it's value of [0 ... smp.cpus]. However, it's always zero >> after this patch is applied because '%' operation is applied for the thread >> ID. >> >> What we need to do is to specify SMP configuration for the test case. I will >> add PATCH[v6 5/5] for it. > Better to keep the fix together with this patch for good bisection. Agreed, it will be part of PATCH[v6 02/04]. >> >> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c >> index 90bf68a5b3..6178ac21a5 100644 >> --- a/tests/qtest/numa-test.c >> +++ b/tests/qtest/numa-test.c >> @@ -223,7 +223,7 @@ 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.cores=1,smp.threads=2 " >> > Maybe it's better to extend aarch64_numa_cpu() by adding > "socket_id, cluster_id, core_id" configuration to the -numa cpu, > given that we have them for aarch64 now. > Exactly, I will use the following command in v6: -machine smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 Thanks, Gavin
Hi Jonathan, On 4/14/22 5:33 PM, Jonathan Cameron wrote: > On Thu, 14 Apr 2022 15:35:41 +0800 > Gavin Shan <gshan@redhat.com> wrote: >> On 4/14/22 10:49 AM, wangyanan (Y) wrote: >>> On 2022/4/14 10:37, Gavin Shan wrote: >>>> On 4/14/22 10:27 AM, wangyanan (Y) wrote: >>>>> On 2022/4/14 8:08, Gavin Shan wrote: >>>>>> On 4/13/22 8:39 PM, wangyanan (Y) wrote: >>>>>>> On 2022/4/3 22:59, Gavin Shan wrote: [...] >>>>> Oh, after further testing this patch breaks numa-test for aarch64, >>>>> which should be checked and fixed. I guess it's because we have >>>>> more IDs supported for ARM. We have to fully running the QEMU >>>>> tests before sending some patches to ensure that they are not >>>>> breaking anything. :) >>>>> >>>> >>>> Thanks for catching the failure and reporting back. I'm not >>>> too much familar with QEMU's test workframe. Could you please >>>> share the detailed commands to reproduce the failure? I will >>>> fix in v6, which will be done in a separate patch :) >>>> >>> There is a reference link: https://wiki.qemu.org/Testing >>> To catch the failure of this patch: "make check" will be enough. >>> > > Speaking from experience, best bet is also upload to a gitlab repo > and let the CI hit things. It will catch this plus any weirdness > elsewhere without you having to figure out too much unless you see > a failure. > > The CI is pretty good though more tests always needed! > Thanks a lot for the hint. I usually use github to host my code. I will setup gitlab repositories so that the verification and tests can be automated. Not sure if there is any document on how to trigger the automatic verification and testing from gitlab? [...] Thanks, Gavin
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index d2e5ecd234..3174526730 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,21 @@ 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->smp.sockets; + 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; }
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 yet. Signed-off-by: Gavin Shan <gshan@redhat.com> --- hw/arm/virt.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)