Re: [PATCH v6 2/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Igor Mammedov <imammedo@redhat.com>]

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
>