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

[wangyanan (Y)]

On 2022/4/14 10:37, Gavin Shan wrote:
> 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 :)
There is a reference link: https://wiki.qemu.org/Testing
To catch the failure of this patch: "make check" will be enough.

Thanks,
Yanan
> Thanks,
> Gavin
>
>
> .