On Wed, 20 Apr 2022 22:24:46 +0800 Gavin Shan <gs...@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 <gs...@redhat.com> wrote: > >> On 4/20/22 4:32 PM, Igor Mammedov wrote: > >>> On Mon, 18 Apr 2022 10:09:18 +0800 > >>> Gavin Shan <gs...@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 <gs...@redhat.com> > >>>> Reviewed-by: Yanan Wang <wangyana...@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 >