On Thu, 14 Apr 2022 15:35:41 +0800
Gavin Shan <gs...@redhat.com> wrote:
> 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 <gs...@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 <wangyana...@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.
> >
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!
Jonathan
>
> 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
>
>
>
