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

[Gavin Shan <gshan@redhat.com>]

Hi Igor,

On 4/20/22 7:50 PM, Igor Mammedov wrote:
> On Wed, 20 Apr 2022 18:31:02 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>> 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)
> 

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.

> 
>>
>>
>>>>            "-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