Re: [PATCH v2 1/3] hw/arm/virt: Fix CPU's default NUMA node ID

["wangyanan (Y)" via <qemu-devel@nongnu.org>]

On 2022/3/18 21:27, Igor Mammedov wrote:
> On Fri, 18 Mar 2022 21:00:35 +0800
> "wangyanan (Y)" <wangyanan55@huawei.com> wrote:
>
>> On 2022/3/18 17:56, Igor Mammedov wrote:
>>> On Fri, 18 Mar 2022 14:23:34 +0800
>>> "wangyanan (Y)" <wangyanan55@huawei.com> wrote:
>>>   
>>>> Hi Gavin,
>>>>
>>>> On 2022/3/3 11:11, Gavin Shan wrote:
>>>>> The default CPU-to-NUMA association is given by mc->get_default_cpu_node_id()
>>>>> when it isn't provided explicitly. However, the CPU topology isn't fully
>>>>> considered in the default association and it causes CPU topology broken
>>>>> warnings on booting Linux guest.
>>>>>
>>>>> For example, the following warning messages are observed when the Linux guest
>>>>> is booted with the following command lines.
>>>>>
>>>>>      /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
>>>>>      -accel kvm -machine virt,gic-version=host               \
>>>>>      -cpu host                                               \
>>>>>      -smp 6,sockets=2,cores=3,threads=1                      \
>>>>>      -m 1024M,slots=16,maxmem=64G                            \
>>>>>      -object memory-backend-ram,id=mem0,size=128M            \
>>>>>      -object memory-backend-ram,id=mem1,size=128M            \
>>>>>      -object memory-backend-ram,id=mem2,size=128M            \
>>>>>      -object memory-backend-ram,id=mem3,size=128M            \
>>>>>      -object memory-backend-ram,id=mem4,size=128M            \
>>>>>      -object memory-backend-ram,id=mem4,size=384M            \
>>>>>      -numa node,nodeid=0,memdev=mem0                         \
>>>>>      -numa node,nodeid=1,memdev=mem1                         \
>>>>>      -numa node,nodeid=2,memdev=mem2                         \
>>>>>      -numa node,nodeid=3,memdev=mem3                         \
>>>>>      -numa node,nodeid=4,memdev=mem4                         \
>>>>>      -numa node,nodeid=5,memdev=mem5
>>>>>             :
>>>>>      alternatives: patching kernel code
>>>>>      BUG: arch topology borken
>>>>>      the CLS domain not a subset of the MC domain
>>>>>      <the above error log repeats>
>>>>>      BUG: arch topology borken
>>>>>      the DIE domain not a subset of the NODE domain
>>>>>
>>>>> With current implementation of mc->get_default_cpu_node_id(), CPU#0 to CPU#5
>>>>> are associated with NODE#0 to NODE#5 separately. That's incorrect because
>>>>> CPU#0/1/2 should be associated with same NUMA node because they're seated
>>>>> in same socket.
>>>>>
>>>>> This fixes the issue by populating the CPU topology in virt_possible_cpu_arch_ids()
>>>>> and considering the socket index when default CPU-to-NUMA association is given
>>>>> in virt_possible_cpu_arch_ids(). With this applied, no more CPU topology broken
>>>>> warnings are seen from the Linux guest. The 6 CPUs are associated with NODE#0/1,
>>>>> but there are no CPUs associated with NODE#2/3/4/5.
>>>> It may be better to split this patch into two. One extends
>>>> virt_possible_cpu_arch_ids,
>>>> and the other fixes the numa node ID issue.
>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>> ---
>>>>>     hw/arm/virt.c | 17 ++++++++++++++++-
>>>>>     1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>> index 46bf7ceddf..dee02b60fc 100644
>>>>> --- a/hw/arm/virt.c
>>>>> +++ b/hw/arm/virt.c
>>>>> @@ -2488,7 +2488,9 @@ virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>>>>>     
>>>>>     static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx)
>>>>>     {
>>>>> -    return idx % ms->numa_state->num_nodes;
>>>>> +    int64_t socket_id = ms->possible_cpus->cpus[idx].props.socket_id;
>>>>> +
>>>>> +    return socket_id % ms->numa_state->num_nodes;
>>>>>     }
>>>>>     
>>>>>     static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>>>>> @@ -2496,6 +2498,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);
>>>>> @@ -2509,6 +2512,18 @@ 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);
>>>>> +
>>>>> +        ms->possible_cpus->cpus[n].props.has_socket_id = true;
>>>>> +        ms->possible_cpus->cpus[n].props.socket_id =
>>>>> +            n / (ms->smp.dies * ms->smp.clusters *
>>>>> +                ms->smp.cores * ms->smp.threads);
>>>>> +        if (mc->smp_props.dies_supported) {
>>>>> +            ms->possible_cpus->cpus[n].props.has_die_id = true;
>>>>> +            ms->possible_cpus->cpus[n].props.die_id =
>>>>> +                n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
>>>>> +        }
>>>> I still don't think we need to consider dies if it's certainly not
>>>> supported yet, IOW, we will never come into the if-branch.
>>>> We are populating arm-specific topo info instead of the generic,
>>>> we can probably uniformly update this part together with other
>>>> necessary places when we decide to support dies for arm virt
>>>> machine in the future. :)
>>> it seems we do support dies and they are supposed to be numa boundary too,
>>> so perhaps we should account for it when generating node-id.
>> Sorry, I actually meant that we currently don't support dies for arm, so
>> that
>> we will always have "mc->smp_props.dies_supported == False" here, which
>> makes the code a bit unnecessary.  dies are only supported for x86 for
>> now. :)
>>
> then perhaps add an assert() here, so that we would notice and fix this
> place when dies_supported becomes true.
A simple assert() works here, I think.

Thanks,
Yanan
>> Thanks,
>> Yanan
>>>>> +        ms->possible_cpus->cpus[n].props.has_core_id = true;
>>>>> +        ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
>>>>>             ms->possible_cpus->cpus[n].props.has_thread_id = true;
>>>>>             ms->possible_cpus->cpus[n].props.thread_id = n;
>>>>>         }
>>>> Maybe we should use the same algorithm in x86_topo_ids_from_idx
>>>> to populate the IDs, so that scope of socket-id will be [0, total_sockets),
>>>> scope of thread-id is [0, threads_per_core), and so on. Then with a
>>>> group of socket/cluster/core/thread-id, we determine a CPU.
>>>>
>>>> Suggestion: For the long term, is it necessary now to add similar topo
>>>> info infrastructure for ARM, such as X86CPUTopoInfo, X86CPUTopoIDs,
>>>> x86_topo_ids_from_idx?
>>>>
>>>> Thanks,
>>>> Yanan
>>>>   
>>> .
> .