Re: [PATCH] powerpc/numa: fix hot-added CPU on memory-less node
From: Laurent Vivier
Date: Thu Nov 15 2018 - 10:04:56 EST
On 15/11/2018 10:19, Satheesh Rajendran wrote:
> On Wed, Nov 14, 2018 at 06:03:19PM +0100, Laurent Vivier wrote:
>> Trying to hotplug a CPU on an empty NUMA node (without
>> memory or CPU) crashes the kernel when the CPU is onlined.
>>
>> During the onlining process, the kernel calls start_secondary()
>> that ends by calling
>> set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu]))
>> that relies on NODE_DATA(nid)->node_zonelists and in our case
>> NODE_DATA(nid) is NULL.
>>
>> To fix that, add the same checking as we already have in
>> find_and_online_cpu_nid(): if NODE_DATA() is NULL, use
>> the first online node.
>>
>> Bug: https://github.com/linuxppc/linux/issues/184
>> Fixes: ea05ba7c559c8e5a5946c3a94a2a266e9a6680a6
>> (powerpc/numa: Ensure nodes initialized for hotplug)
>> Signed-off-by: Laurent Vivier <lvivier@xxxxxxxxxx>
>> ---
>> arch/powerpc/mm/numa.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>
> This patch causes regression for cold plug numa case(Case 1) and
> hotplug case + reboot(Case 2) with adding all vcpus into node 0.
>
>
> Env: HW: Power8 Host.
> Kernel: 4.20-rc2 + this patch
>
> Case 1:
> 1. boot a guest with 8 vcpus(all available), spreadout in 4 numa nodes.
> <vcpu placement='static'>8</vcpu>
> ...
> <numa>
> <cell id='0' cpus='0-1' memory='4194304' unit='KiB'/>
> <cell id='1' cpus='2-3' memory='4194304' unit='KiB'/>
> <cell id='2' cpus='4-5' memory='0' unit='KiB'/>
> <cell id='3' cpus='6-7' memory='0' unit='KiB'/>
> </numa>
>
> 2. Check lscpu --- all vcpus are added to node0 --> NOK
>
> # lscpu
...
> NUMA node0 CPU(s): 0-7
> NUMA node1 CPU(s):
> NUMA node2 CPU(s):
> NUMA node3 CPU(s):
>
> without this patch it was working fine.
> # lscpu
...
> NUMA node0 CPU(s): 0,1
> NUMA node1 CPU(s): 2,3
> NUMA node2 CPU(s): 4,5
> NUMA node3 CPU(s): 6,7
>
Good point. Thank you.
I'm going to see what happens and how the cold case allows to online
CPUs on nodes with NODE_DATA() set to NULL (because it's what the patch
changes).
Thanks,
Laurent