Re: [PATCH] hw/arm/virt: Prevent CPUs in one socket to span mutiple NUMA nodes

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 22/2/23 00:12, Gavin Shan wrote:
> On 2/21/23 9:21 PM, Philippe Mathieu-Daudé wrote:
>> On 21/2/23 10:21, Gavin Shan wrote:
>>> On 2/21/23 8:15 PM, Philippe Mathieu-Daudé wrote:
>>>> On 21/2/23 09:53, Gavin Shan wrote:
>>>>> Linux kernel guest reports warning when two CPUs in one socket have
>>>>> been associated with different NUMA nodes, using the following command
>>>>> lines.
>>>>>
>>>>>    -smp 6,maxcpus=6,sockets=2,clusters=1,cores=3,threads=1 \
>>>>>    -numa node,nodeid=0,cpus=0-1,memdev=ram0                \
>>>>>    -numa node,nodeid=1,cpus=2-3,memdev=ram1                \
>>>>>    -numa node,nodeid=2,cpus=4-5,memdev=ram2                \
>>>>>
>>>>>    ------------[ cut here ]------------
>>>>>    WARNING: CPU: 0 PID: 1 at kernel/sched/topology.c:2271 
>>>>> build_sched_domains+0x284/0x910
>>>>>    Modules linked in:
>>>>>    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0-268.el9.aarch64 #1
>>>>>    pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>>    pc : build_sched_domains+0x284/0x910
>>>>>    lr : build_sched_domains+0x184/0x910
>>>>>    sp : ffff80000804bd50
>>>>>    x29: ffff80000804bd50 x28: 0000000000000002 x27: 0000000000000000
>>>>>    x26: ffff800009cf9a80 x25: 0000000000000000 x24: ffff800009cbf840
>>>>>    x23: ffff000080325000 x22: ffff0000005df800 x21: ffff80000a4ce508
>>>>>    x20: 0000000000000000 x19: ffff000080324440 x18: 0000000000000014
>>>>>    x17: 00000000388925c0 x16: 000000005386a066 x15: 000000009c10cc2e
>>>>>    x14: 00000000000001c0 x13: 0000000000000001 x12: ffff00007fffb1a0
>>>>>    x11: ffff00007fffb180 x10: ffff80000a4ce508 x9 : 0000000000000041
>>>>>    x8 : ffff80000a4ce500 x7 : ffff80000a4cf920 x6 : 0000000000000001
>>>>>    x5 : 0000000000000001 x4 : 0000000000000007 x3 : 0000000000000002
>>>>>    x2 : 0000000000001000 x1 : ffff80000a4cf928 x0 : 0000000000000001
>>>>>    Call trace:
>>>>>     build_sched_domains+0x284/0x910
>>>>>     sched_init_domains+0xac/0xe0
>>>>>     sched_init_smp+0x48/0xc8
>>>>>     kernel_init_freeable+0x140/0x1ac
>>>>>     kernel_init+0x28/0x140
>>>>>     ret_from_fork+0x10/0x20
>>>>>
>>>>> Fix it by preventing mutiple CPUs in one socket to be associated with
>>>>> different NUMA nodes.
>>>>>
>>>>> Reported-by: Yihuang Yu <yihyu@redhat.com>
>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>> ---
>>>>>   hw/arm/virt.c | 37 +++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 37 insertions(+)
>>>>>
>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>> index ac626b3bef..e0af267c77 100644
>>>>> --- a/hw/arm/virt.c
>>>>> +++ b/hw/arm/virt.c
>>>>> @@ -230,6 +230,39 @@ static bool cpu_type_valid(const char *cpu)
>>>>>       return false;
>>>>>   }
>>>>> +static bool numa_state_valid(MachineState *ms)
>>>>> +{
>>>>> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>>>>> +    NumaState *state = ms->numa_state;
>>>>> +    const CPUArchIdList *possible_cpus = 
>>>>> mc->possible_cpu_arch_ids(ms);
>>>>> +    const CPUArchId *cpus = possible_cpus->cpus;
>>>>> +    int len = possible_cpus->len, i, j;
>>>>> +
>>>>> +    if (!state || state->num_nodes <= 1 || len <= 1) {
>>>>> +        return true;
>>>>> +    }
>>>>> +
>>>>> +    for (i = 0; i < len; i++) {
>>>>> +        for (j = i + 1; j < len; j++) {
>>>>> +            if (cpus[i].props.has_socket_id &&
>>>>> +                cpus[i].props.has_node_id &&
>>>>> +                cpus[j].props.has_socket_id &&
>>>>> +                cpus[j].props.has_node_id &&
>>>>> +                cpus[i].props.socket_id == cpus[j].props.socket_id &&
>>>>> +                cpus[i].props.node_id != cpus[j].props.node_id) {
>>>>> +                error_report("CPU-%d and CPU-%d in socket-%ld have 
>>>>> been "
>>>>> +                             "associated with node-%ld and node-%ld",
>>>>> +                             i, j, cpus[i].props.socket_id,
>>>>> +                             cpus[i].props.node_id,
>>>>> +                             cpus[j].props.node_id);
>>>>> +                return false;
>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return true;
>>>>> +}
>>>>> +
>>>>>   static void create_randomness(MachineState *ms, const char *node)
>>>>>   {
>>>>>       struct {
>>>>> @@ -2040,6 +2073,10 @@ static void machvirt_init(MachineState 
>>>>> *machine)
>>>>>           exit(1);
>>>>>       }
>>>>> +    if (!numa_state_valid(machine)) {
>>>>> +        exit(1);
>>>>> +    }
>>>>
>>>> Why restrict to the virt machine?
>>>>
>>>
>>> We tried x86 machines and virt machine, but the issue isn't 
>>> reproducible on x86 machines.
>>> So I think it's machine or architecture specific issue. However, I 
>>> believe RiscV should
>>> have similar issue because linux/drivers/base/arch_topology.c is 
>>> shared by ARM64 and RiscV.
>>> x86 doesn't use the driver to populate its CPU topology.
>>
>> Oh, I haven't thought about the other archs, I meant this seem a generic
>> issue which affects all (ARM) machines, so why restrict to the (ARM)
>> virt machine?
>>
> 
> [Ccing Igor for comments]
> 
> Well, virt machine is the only concern to us for now. You're right that 
> all ARM64 and ARM machines
> need this check and limitation. So the check needs to be done in the 
> generic path. The best way
> I can figure out is like something below. The idea is to introduce a 
> switch to 'struct NumaState'
> and do the check in the generic path. The switch is turned on by 
> individual machines. Please let me
> know if you have better ideas

Can't this be done generically in machine_numa_finish_cpu_init()
-> numa_validate_initiator()?

> - Add 'bool struct NumaState::has_strict_socket_mapping', which is 
> 'false' by default until
>    machine specific initialization function calls helper 
> set_numa_strict_socket_mapping(), for
>    example in hw/arm/virt.c::virt_instance_init().
> 
> - In numa_complete_configuration(), do the check to make sure the socket 
> doesn't cross over
>    the NUMA node boundary if 'bool struct 
> NumaState::has_strict_socket_mapping' is true.
> 
> Thanks,
> Gavin
> 
>