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

[Gavin Shan <gshan@redhat.com>]

Hi Yanan,

On 4/14/22 5:29 PM, wangyanan (Y) wrote:
> On 2022/4/14 15:35, Gavin Shan wrote:
>> 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 <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 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.
> Better to keep the fix together with this patch for good bisection.

Agreed, it will be part of PATCH[v6 02/04].

>>
>> 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 "
>>
> Maybe it's better to extend aarch64_numa_cpu() by adding
> "socket_id, cluster_id, core_id" configuration to the -numa cpu,
> given that we have them for aarch64 now.
> 

Exactly, I will use the following command in v6:

   -machine smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2

Thanks,
Gavin