| Message ID | 20220418020920.144263-3-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Fix CPU's default NUMA node ID | expand |
On Mon, 18 Apr 2022 10:09:18 +0800 Gavin Shan <gshan@redhat.com> 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)); > + 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 " Is cluster-less config possible? (looks like it used to work before and it doesn't after this series) > "-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");
Hi Igor, On 4/20/22 4:32 PM, Igor Mammedov wrote: > On Mon, 18 Apr 2022 10:09:18 +0800 > Gavin Shan <gshan@redhat.com> 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)); >> + 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 " > > Is cluster-less config possible? > (looks like it used to work before and it doesn't after this series) > Nope, it's impossible. This specific test case uses arm/virt machine where cluster is always supported.mc->smp_props.clusters_supported has been set to true in hw/arm/virt.c::virt_machine_class_init(). Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks the test. It's why the fix to qtest/numa-test has been squashed to this patch, to make it 'bit bisect' friendly as Yanan suggested. >> "-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, Gavin
On Wed, 20 Apr 2022 18:31:02 +0800 Gavin Shan <gshan@redhat.com> wrote: > Hi Igor, > > On 4/20/22 4:32 PM, Igor Mammedov wrote: > > On Mon, 18 Apr 2022 10:09:18 +0800 > > Gavin Shan <gshan@redhat.com> 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)); > >> + 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 " > > > > Is cluster-less config possible? > > (looks like it used to work before and it doesn't after this series) > > > > Nope, it's impossible. This specific test case uses arm/virt machine > where cluster is always supported.mc->smp_props.clusters_supported > has been set to true in hw/arm/virt.c::virt_machine_class_init(). > > Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks > the test. It's why the fix to qtest/numa-test has been squashed to this patch, to > make it 'bit bisect' friendly as Yanan suggested. so what was error that broke the test? (probably should be mentioned in commit message) (also is it possible to split out the test patch into a separate one and put it before this one) > > > >> "-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, > Gavin >
Hi Igor, On 4/20/22 7:50 PM, Igor Mammedov wrote: > On Wed, 20 Apr 2022 18:31:02 +0800 > Gavin Shan <gshan@redhat.com> wrote: >> On 4/20/22 4:32 PM, Igor Mammedov wrote: >>> On Mon, 18 Apr 2022 10:09:18 +0800 >>> Gavin Shan <gshan@redhat.com> 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)); >>>> + 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 " >>> >>> Is cluster-less config possible? >>> (looks like it used to work before and it doesn't after this series) >>> >> >> Nope, it's impossible. This specific test case uses arm/virt machine >> where cluster is always supported.mc->smp_props.clusters_supported >> has been set to true in hw/arm/virt.c::virt_machine_class_init(). >> >> Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks >> the test. It's why the fix to qtest/numa-test has been squashed to this patch, to >> make it 'bit bisect' friendly as Yanan suggested. > > so what was error that broke the test? > (probably should be mentioned in commit message) > > (also is it possible to split out the test patch into > a separate one and put it before this one) > With amend to the command lines, the following one is used and below error is raised from the test. The error is mentioned in the commit log in PATCH[v7 2/4]. -machine smp.cpus=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 qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found (reported from hw/core/machine.c::machine_set_cpu_numa_node()) After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1" isn't valid any more. The CPU topology becomes like below. Note that mc->smp_props.prefer_sockets is true on arm/virt machine. index socket cluster core thread -------------------------------------------- 0 0 0 0 0 1 1 0 0 0 With the amended command lines, the topology changes again so that "thread-id=1" is valid: index socket cluster core thread -------------------------------------------- 0 0 0 0 0 1 0 0 0 1 It should be ok to split the test/qtest/aarch64_numa_cpu() changes into a separate patch and put it before this one. In that case, the specified smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet, and 'thread-id=2' should be still valid. Lets do this if I need post v8. Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu() changes into PATCH[2/4], as we're doing. > >> >> >>>> "-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, Gavin
On Wed, 20 Apr 2022 22:24:46 +0800 Gavin Shan <gshan@redhat.com> wrote: > Hi Igor, > > On 4/20/22 7:50 PM, Igor Mammedov wrote: > > On Wed, 20 Apr 2022 18:31:02 +0800 > > Gavin Shan <gshan@redhat.com> wrote: > >> On 4/20/22 4:32 PM, Igor Mammedov wrote: > >>> On Mon, 18 Apr 2022 10:09:18 +0800 > >>> Gavin Shan <gshan@redhat.com> 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)); > >>>> + 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 " > >>> > >>> Is cluster-less config possible? > >>> (looks like it used to work before and it doesn't after this series) > >>> > >> > >> Nope, it's impossible. This specific test case uses arm/virt machine > >> where cluster is always supported.mc->smp_props.clusters_supported > >> has been set to true in hw/arm/virt.c::virt_machine_class_init(). > >> > >> Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks > >> the test. It's why the fix to qtest/numa-test has been squashed to this patch, to > >> make it 'bit bisect' friendly as Yanan suggested. > > > > so what was error that broke the test? > > (probably should be mentioned in commit message) > > > > (also is it possible to split out the test patch into > > a separate one and put it before this one) > > > > With amend to the command lines, the following one is used and below error > is raised from the test. The error is mentioned in the commit log in > PATCH[v7 2/4]. > > -machine smp.cpus=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 > > qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found > (reported from hw/core/machine.c::machine_set_cpu_numa_node()) > > After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1" > isn't valid any more. The CPU topology becomes like below. Note that > mc->smp_props.prefer_sockets is true on arm/virt machine. > > index socket cluster core thread > -------------------------------------------- > 0 0 0 0 0 > 1 1 0 0 0 > > With the amended command lines, the topology changes again so > that "thread-id=1" is valid: > > index socket cluster core thread > -------------------------------------------- > 0 0 0 0 0 > 1 0 0 0 1 > > It should be ok to split the test/qtest/aarch64_numa_cpu() changes into > a separate patch and put it before this one. In that case, the specified > smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet, > and 'thread-id=2' should be still valid. Lets do this if I need post v8. > Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu() > changes into PATCH[2/4], as we're doing. if you need to respin v7. do it as separate patch with proper commit message and maybe add an extra test that exercises fully specified topo. > > > > >> > >> > >>>> "-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, > Gavin >
On Wed, Apr 20, 2022 at 10:24:46PM +0800, Gavin Shan wrote: ... > With amend to the command lines, the following one is used and below error > is raised from the test. The error is mentioned in the commit log in > PATCH[v7 2/4]. > > -machine smp.cpus=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 > > qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found > (reported from hw/core/machine.c::machine_set_cpu_numa_node()) > > After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1" > isn't valid any more. The CPU topology becomes like below. Note that > mc->smp_props.prefer_sockets is true on arm/virt machine. prefer_sockets is only true for mach-virt 6.1 and older. It's false for 6.2 and later. Thanks, drew > > index socket cluster core thread > -------------------------------------------- > 0 0 0 0 0 > 1 1 0 0 0 > > With the amended command lines, the topology changes again so > that "thread-id=1" is valid: > > index socket cluster core thread > -------------------------------------------- > 0 0 0 0 0 > 1 0 0 0 1 > > It should be ok to split the test/qtest/aarch64_numa_cpu() changes into > a separate patch and put it before this one. In that case, the specified > smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet, > and 'thread-id=2' should be still valid. Lets do this if I need post v8. > Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu() > changes into PATCH[2/4], as we're doing. > > > > > > > > > > > > > > "-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, > Gavin >
Hi Igor, On 4/20/22 10:50 PM, Igor Mammedov wrote: > On Wed, 20 Apr 2022 22:24:46 +0800 > Gavin Shan <gshan@redhat.com> wrote: >> On 4/20/22 7:50 PM, Igor Mammedov wrote: >>> On Wed, 20 Apr 2022 18:31:02 +0800 >>> Gavin Shan <gshan@redhat.com> wrote: >>>> On 4/20/22 4:32 PM, Igor Mammedov wrote: >>>>> On Mon, 18 Apr 2022 10:09:18 +0800 >>>>> Gavin Shan <gshan@redhat.com> 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)); >>>>>> + 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 " >>>>> >>>>> Is cluster-less config possible? >>>>> (looks like it used to work before and it doesn't after this series) >>>>> >>>> >>>> Nope, it's impossible. This specific test case uses arm/virt machine >>>> where cluster is always supported.mc->smp_props.clusters_supported >>>> has been set to true in hw/arm/virt.c::virt_machine_class_init(). >>>> >>>> Exactly, the changes to virt_possible_cpu_arch_ids() included in this patch breaks >>>> the test. It's why the fix to qtest/numa-test has been squashed to this patch, to >>>> make it 'bit bisect' friendly as Yanan suggested. >>> >>> so what was error that broke the test? >>> (probably should be mentioned in commit message) >>> >>> (also is it possible to split out the test patch into >>> a separate one and put it before this one) >>> >> >> With amend to the command lines, the following one is used and below error >> is raised from the test. The error is mentioned in the commit log in >> PATCH[v7 2/4]. >> >> -machine smp.cpus=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 >> >> qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found >> (reported from hw/core/machine.c::machine_set_cpu_numa_node()) >> >> After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1" >> isn't valid any more. The CPU topology becomes like below. Note that >> mc->smp_props.prefer_sockets is true on arm/virt machine. >> >> index socket cluster core thread >> -------------------------------------------- >> 0 0 0 0 0 >> 1 1 0 0 0 >> >> With the amended command lines, the topology changes again so >> that "thread-id=1" is valid: >> >> index socket cluster core thread >> -------------------------------------------- >> 0 0 0 0 0 >> 1 0 0 0 1 >> >> It should be ok to split the test/qtest/aarch64_numa_cpu() changes into >> a separate patch and put it before this one. In that case, the specified >> smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet, >> and 'thread-id=2' should be still valid. Lets do this if I need post v8. >> Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu() >> changes into PATCH[2/4], as we're doing. > > if you need to respin v7. do it as separate patch with proper commit message > and maybe add an extra test that exercises fully specified topo. > Sure, I will split the fix for test/qtest/aarch64_numa_cpu() in v8 if it's needed. For the additional test case to exercise the fully specific topology, I rather to do it after this series is merged to v7.1 because our downstream needs the fix :) >> >>> >>>> >>>> >>>>>> "-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, Gavin
Hi Drew, On 4/21/22 5:02 PM, Andrew Jones wrote: > On Wed, Apr 20, 2022 at 10:24:46PM +0800, Gavin Shan wrote: > ... >> With amend to the command lines, the following one is used and below error >> is raised from the test. The error is mentioned in the commit log in >> PATCH[v7 2/4]. >> >> -machine smp.cpus=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 >> >> qemu-system-aarch64: -numa cpu,node-id=0,thread-id=1: no match found >> (reported from hw/core/machine.c::machine_set_cpu_numa_node()) >> >> After the changes to virt_possible_cpu_arch_ids() is applied, "thread-id=1" >> isn't valid any more. The CPU topology becomes like below. Note that >> mc->smp_props.prefer_sockets is true on arm/virt machine. > > prefer_sockets is only true for mach-virt 6.1 and older. It's false for > 6.2 and later. > Yeah, @perfer_socket is false in last mach-virt-7.0 , which is used in this test case. So we prefer CPU core over sockets, as explained in hw/core/machine.c::machine_parse_smp_config(). The CPU topology becomes like below instead, but 'thread-id=1' is still invalid. index socket cluster core thread -------------------------------------------- 0 0 0 0 0 1 0 0 1 0 >> >> index socket cluster core thread >> -------------------------------------------- >> 0 0 0 0 0 >> 1 1 0 0 0 >> >> With the amended command lines, the topology changes again so >> that "thread-id=1" is valid: >> >> index socket cluster core thread >> -------------------------------------------- >> 0 0 0 0 0 >> 1 0 0 0 1 >> >> It should be ok to split the test/qtest/aarch64_numa_cpu() changes into >> a separate patch and put it before this one. In that case, the specified >> smp.{socket, cluster, core, threads} isn't used by arm/virt machine yet, >> and 'thread-id=2' should be still valid. Lets do this if I need post v8. >> Otherwise, I guess it's also fine to squash the test/qtest/aarch64_numa_cpu() >> changes into PATCH[2/4], as we're doing. >> >>> >>>> >>>> >>>>>> "-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, 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");