[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f7ab6ec1-7a49-2764-7c19-9949ad508e2e@loongson.cn>
Date: Mon, 28 Oct 2024 20:37:29 +0800
From: maobibo <maobibo@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: Jianmin Lv <lvjianmin@...ngson.cn>, loongarch@...ts.linux.dev,
linux-kernel@...r.kernel.org, lixianglai@...ngson.cn,
WANG Xuerui <kernel@...0n.name>
Subject: Re: [PATCH v2] LoongArch: Fix cpu hotplug issue
Hi Huacai,
On 2024/10/22 上午9:31, Huacai Chen wrote:
> On Tue, Oct 22, 2024 at 9:17 AM maobibo <maobibo@...ngson.cn> wrote:
>>
>>
>>
>> On 2024/10/21 下午10:32, Huacai Chen wrote:
>>> Hi, Bibo,
>>>
>>> This version still doesn't touch the round-robin method, but it
>>> doesn't matter, I think I misunderstood something since V1...
>> I do not understand why round-robin method need be modified, SRAT may be
>> disabled with general function disable_srat(). Then round-robin method
>> is required.
> I don't mean round-robin should be modified, I mean I misunderstand round-robin.
>
>>
>>>
>>> Please correct me if I'm wrong: For cpus without ACPI_MADT_ENABLED, in
>>> smp_prepare_boot_cpu() the round-robin node ids only apply to
>>> cpu_to_node(), but __cpuid_to_node[] still record the right node ids.
>>> early_cpu_to_node() returns NUMA_NO_NODE not because
>>> __cpuid_to_node[] records NUMA_NO_NODE, but because cpu_logical_map()
>>> < 0.
>>>
>>> If the above is correct, we don't need so complicated, because the
>>> correct and simplest way is:
>>> https://lore.kernel.org/loongarch/6b2b3e89-5a46-2d20-3dfb-7aae33839f49@loongson.cn/T/#m950eead5250e5992cc703bbe69622348cecfa465
>>>
>> It works also. Only that LoongArch kernel parsing about SRAT/MADT is
>> badly. If you do not mind, I do not mind neither. It is not my duty for
>> kernel side.
> Yes, I don't mind, please use that simplest way.
There is another problem with the simple way. eiointc reports error when
cpu is online. The error message is:
Loongson-64bit Processor probed (LA464 Core)
CPU2 revision is: 0014c010 (Loongson-64bit)
FPU2 revision is: 00000001
eiointc: Error: invalid nodemap!
CPU 2 UP state irqchip/loongarch/eiointc:starting (100) failed (-1)
The problem is that node_map of eiointc is problematic,
static int cpu_to_eio_node(int cpu)
{
return cpu_logical_map(cpu) / CORES_PER_EIO_NODE;
}
static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
u64 node_map)
{
int i;
node_map = node_map ? node_map : -1ULL;
for_each_possible_cpu(i) {
if (node_map & (1ULL << (cpu_to_eio_node(i)))) {
node_set(cpu_to_eio_node(i), priv->node_map);
...
The cause is that for possible not present cpu, *cpu_logical_map(cpu)*
is -1, cpu_to_eio_node(i) will be equal to -1, so node_map of eiointc is
problematic.
So cpu_logical_map(cpu) should be set during MADT parsing even if it is
not enabled at beginning, it should not be set at hotplug runtime.
Regards
Bibo Mao
>
> Huacai
>
>>
>> Bibo Mao
>>>
>>> Huacai
>>>
>>> On Mon, Oct 21, 2024 at 4:04 PM Bibo Mao <maobibo@...ngson.cn> wrote:
>>>>
>>>> On LoongArch system, there are two places to set cpu numa node. One
>>>> is in arch specified function smp_prepare_boot_cpu(), the other is
>>>> in generic function early_numa_node_init(). The latter will overwrite
>>>> the numa node information.
>>>>
>>>> With hot-added cpu without numa information, cpu_logical_map() fails
>>>> to its physical cpuid at beginning since it is not enabled in ACPI
>>>> MADT table. So function early_cpu_to_node() also fails to get its
>>>> numa node for hot-added cpu, and generic function
>>>> early_numa_node_init() will overwrite with incorrect numa node.
>>>>
>>>> APIs topo_get_cpu() and topo_add_cpu() is added here, like other
>>>> architectures logic cpu is allocated when parsing MADT table. When
>>>> parsing SRAT table or hot-add cpu, logic cpu is acquired by searching
>>>> all allocated logical cpu with matched physical id. It solves such
>>>> problems such as:
>>>> 1. Boot cpu is not the first entry in MADT table, the first entry
>>>> will be overwritten with later boot cpu.
>>>> 2. Physical cpu id not presented in MADT table is invalid, in later
>>>> SRAT/hot-add cpu parsing, invalid physical cpu detected is added
>>>> 3. For hot-add cpu, its logic cpu is allocated in MADT table parsing,
>>>> so early_cpu_to_node() can be used for hot-add cpu and cpu_to_node()
>>>> is correct for hot-add cpu.
>>>>
>>>> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
>>>> ---
>>>> v1 ... v2:
>>>> 1. Like other architectures, allocate logic cpu when parsing MADT table.
>>>> 2. Add invalid or duplicated physical cpuid parsing with SRAT table or
>>>> hot-add cpu DSDT information.
>>>> ---
>>>> arch/loongarch/include/asm/smp.h | 3 ++
>>>> arch/loongarch/kernel/acpi.c | 24 ++++++++++------
>>>> arch/loongarch/kernel/setup.c | 47 ++++++++++++++++++++++++++++++++
>>>> arch/loongarch/kernel/smp.c | 9 +++---
>>>> 4 files changed, 70 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/loongarch/include/asm/smp.h b/arch/loongarch/include/asm/smp.h
>>>> index 3383c9d24e94..c61b75937a77 100644
>>>> --- a/arch/loongarch/include/asm/smp.h
>>>> +++ b/arch/loongarch/include/asm/smp.h
>>>> @@ -119,4 +119,7 @@ static inline void __cpu_die(unsigned int cpu)
>>>> #define cpu_logical_map(cpu) 0
>>>> #endif /* CONFIG_SMP */
>>>>
>>>> +int topo_add_cpu(int physid);
>>>> +int topo_get_cpu(int physid);
>>>> +
>>>> #endif /* __ASM_SMP_H */
>>>> diff --git a/arch/loongarch/kernel/acpi.c b/arch/loongarch/kernel/acpi.c
>>>> index f1a74b80f22c..84d9812d5f38 100644
>>>> --- a/arch/loongarch/kernel/acpi.c
>>>> +++ b/arch/loongarch/kernel/acpi.c
>>>> @@ -78,10 +78,10 @@ static int set_processor_mask(u32 id, u32 flags)
>>>> return -ENODEV;
>>>>
>>>> }
>>>> - if (cpuid == loongson_sysconf.boot_cpu_id)
>>>> - cpu = 0;
>>>> - else
>>>> - cpu = find_first_zero_bit(cpumask_bits(cpu_present_mask), NR_CPUS);
>>>> +
>>>> + cpu = topo_add_cpu(cpuid);
>>>> + if (cpu < 0)
>>>> + return -EEXIST;
>>>>
>>>> if (!cpu_enumerated)
>>>> set_cpu_possible(cpu, true);
>>>> @@ -203,8 +203,6 @@ void __init acpi_boot_table_init(void)
>>>> goto fdt_earlycon;
>>>> }
>>>>
>>>> - loongson_sysconf.boot_cpu_id = read_csr_cpuid();
>>>> -
>>>> /*
>>>> * Process the Multiple APIC Description Table (MADT), if present
>>>> */
>>>> @@ -257,7 +255,7 @@ void __init numa_set_distance(int from, int to, int distance)
>>>> void __init
>>>> acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
>>>> {
>>>> - int pxm, node;
>>>> + int pxm, node, cpu;
>>>>
>>>> if (srat_disabled())
>>>> return;
>>>> @@ -286,6 +284,11 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
>>>> return;
>>>> }
>>>>
>>>> + cpu = topo_get_cpu(pa->apic_id);
>>>> + /* Check whether apic_id exists in MADT table */
>>>> + if (cpu < 0)
>>>> + return;
>>>> +
>>>> early_numa_add_cpu(pa->apic_id, node);
>>>>
>>>> set_cpuid_to_node(pa->apic_id, node);
>>>> @@ -324,12 +327,17 @@ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id, int *pcpu
>>>> {
>>>> int cpu;
>>>>
>>>> - cpu = set_processor_mask(physid, ACPI_MADT_ENABLED);
>>>> + cpu = topo_get_cpu(physid);
>>>> + /* Check whether apic_id exists in MADT table */
>>>> if (cpu < 0) {
>>>> pr_info(PREFIX "Unable to map lapic to logical cpu number\n");
>>>> return cpu;
>>>> }
>>>>
>>>> + num_processors++;
>>>> + set_cpu_present(cpu, true);
>>>> + __cpu_number_map[physid] = cpu;
>>>> + __cpu_logical_map[cpu] = physid;
>>>> acpi_map_cpu2node(handle, cpu, physid);
>>>>
>>>> *pcpu = cpu;
>>>> diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
>>>> index 00e307203ddb..649e98640076 100644
>>>> --- a/arch/loongarch/kernel/setup.c
>>>> +++ b/arch/loongarch/kernel/setup.c
>>>> @@ -65,6 +65,8 @@ EXPORT_SYMBOL(cpu_data);
>>>>
>>>> struct loongson_board_info b_info;
>>>> static const char dmi_empty_string[] = " ";
>>>> +static int possible_cpus;
>>>> +static bool bsp_added;
>>>>
>>>> /*
>>>> * Setup information
>>>> @@ -346,10 +348,55 @@ static void __init bootcmdline_init(char **cmdline_p)
>>>> *cmdline_p = boot_command_line;
>>>> }
>>>>
>>>> +int topo_get_cpu(int physid)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < possible_cpus; i++)
>>>> + if (cpu_logical_map(i) == physid)
>>>> + break;
>>>> +
>>>> + if (i == possible_cpus)
>>>> + return -ENOENT;
>>>> +
>>>> + return i;
>>>> +}
>>>> +
>>>> +int topo_add_cpu(int physid)
>>>> +{
>>>> + int cpu;
>>>> +
>>>> + if (!bsp_added && (physid == loongson_sysconf.boot_cpu_id)) {
>>>> + bsp_added = true;
>>>> + return 0;
>>>> + }
>>>> +
>>>> + cpu = topo_get_cpu(physid);
>>>> + if (cpu >= 0) {
>>>> + pr_warn("Adding duplicated physical cpuid 0x%x\n", physid);
>>>> + return -EEXIST;
>>>> + }
>>>> +
>>>> + if (possible_cpus >= nr_cpu_ids)
>>>> + return -ERANGE;
>>>> +
>>>> + __cpu_logical_map[possible_cpus] = physid;
>>>> + cpu = possible_cpus++;
>>>> + return cpu;
>>>> +}
>>>> +
>>>> +static void __init topo_init(void)
>>>> +{
>>>> + loongson_sysconf.boot_cpu_id = read_csr_cpuid();
>>>> + __cpu_logical_map[0] = loongson_sysconf.boot_cpu_id;
>>>> + possible_cpus++;
>>>> +}
>>>> +
>>>> void __init platform_init(void)
>>>> {
>>>> arch_reserve_vmcore();
>>>> arch_reserve_crashkernel();
>>>> + topo_init();
>>>>
>>>> #ifdef CONFIG_ACPI
>>>> acpi_table_upgrade();
>>>> diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
>>>> index 9afc2d8b3414..a3f466b89179 100644
>>>> --- a/arch/loongarch/kernel/smp.c
>>>> +++ b/arch/loongarch/kernel/smp.c
>>>> @@ -291,10 +291,9 @@ static void __init fdt_smp_setup(void)
>>>> if (cpuid >= nr_cpu_ids)
>>>> continue;
>>>>
>>>> - if (cpuid == loongson_sysconf.boot_cpu_id)
>>>> - cpu = 0;
>>>> - else
>>>> - cpu = find_first_zero_bit(cpumask_bits(cpu_present_mask), NR_CPUS);
>>>> + cpu = topo_add_cpu(cpuid);
>>>> + if (cpu < 0)
>>>> + continue;
>>>>
>>>> num_processors++;
>>>> set_cpu_possible(cpu, true);
>>>> @@ -302,7 +301,7 @@ static void __init fdt_smp_setup(void)
>>>> __cpu_number_map[cpuid] = cpu;
>>>> __cpu_logical_map[cpu] = cpuid;
>>>>
>>>> - early_numa_add_cpu(cpu, 0);
>>>> + early_numa_add_cpu(cpuid, 0);
>>>> set_cpuid_to_node(cpuid, 0);
>>>> }
>>>>
>>>>
>>>> base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
>>>> --
>>>> 2.39.3
>>>>
>>
>>
Powered by blists - more mailing lists