| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <10082e64-b00a-a30b-b9c5-1401a54f6717@huawei.com>
Date: Thu, 5 Sep 2024 20:02:20 +0800
From: Yicong Yang <yangyicong@...wei.com>
To: Pierre Gondois <pierre.gondois@....com>
CC: <yangyicong@...ilicon.com>, <linuxppc-dev@...ts.ozlabs.org>,
<bp@...en8.de>, <dave.hansen@...ux.intel.com>, <mingo@...hat.com>,
<linux-arm-kernel@...ts.infradead.org>, <mpe@...erman.id.au>,
<peterz@...radead.org>, <tglx@...utronix.de>, <sudeep.holla@....com>,
<will@...nel.org>, <catalin.marinas@....com>, <x86@...nel.org>,
<linux-kernel@...r.kernel.org>, <dietmar.eggemann@....com>,
<gregkh@...uxfoundation.org>, <rafael@...nel.org>,
<jonathan.cameron@...wei.com>, <prime.zeng@...ilicon.com>,
<linuxarm@...wei.com>, <xuwei5@...wei.com>, <guohanjun@...wei.com>
Subject: Re: [PATCH v5 3/4] arm64: topology: Support SMT control on ACPI based
system
On 2024/9/5 16:34, Pierre Gondois wrote:
> Hello Yicong,
>
>>>> Wondering if we can avoid this 2nd loop. Greg express the worries of looping twice on large scale
>>>> system in v1. Maybe we could use the hetero_id and get the necessary information in one loop, I need
>>>> further think.
>>>
>>> I found this comments (not sure this is what you are refering to):
>>> - https://lore.kernel.org/linux-arm-kernel/20231011103303.00002d8f@Huawei.com/
>>> - https://lore.kernel.org/all/20230921150333.c2zqigs3xxwcg4ln@bogus/T/#m406c4c16871ca7ae431beb20feccfb5e14498452
>>>
>>> I don't see another way to do it right now. Also, I thing the complexity is in
>>> O(2n), which should be better than the original O(n**2),
>>>
>>
>> yes it's less complex. I'm wondering build up the xarray in another way then we can avoid the
>> long loops. What about below:
>
> I tried the patch on a ThunderX2 with 4 threads per CPU. PPTT topology describes
> 2 clusters of 128 cores each. Each cluster is described as independent (i.e. there
> is no root node in the PPTT). The PPTT is of revision 1, so the IDENTICAL
> flag is not available on the platform.
>
I think the SMT detection should work on this case, does it? In this case there should be
2 entries for each cluster respectively in the hetero_cpu array. Since the thread number
are the same, we'll detect a symmetric SMT topology.
> I also tried it on a faked SMT asymmetric platform and there might be a small
> correction required.
>
>>
>> From 5ff5d0100435982764cd85566a6fe006e60ee98e Mon Sep 17 00:00:00 2001
>> From: Yicong Yang <yangyicong@...ilicon.com>
>> Date: Fri, 20 Oct 2023 15:38:38 +0800
>> Subject: [PATCH] arm64: topology: Support SMT control on ACPI based system
>>
>> For ACPI we'll build the topology from PPTT and we cannot directly
>> get the SMT number of each core. Instead using a temporary xarray
>> to record the heterogeneous information (from ACPI_PPTT_ACPI_IDENTICAL)
>> and SMT information of the first core in its heterogeneous CPU cluster
>> when building the topology. Then we can know the largest SMT number
>> in the system. Warn if heterogeneous SMT topology exists (multiple
>> heterogeneous CPU clusters with different SMT thread number) since the
>> SMT control cannot handle this well. Then enable the support of SMT
>> control.
>>
>> Signed-off-by: Yicong Yang <yangyicong@...ilicon.com>
>> ---
>> arch/arm64/kernel/topology.c | 60 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 60 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 1a2c72f3e7f8..f6ec30fae70e 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -15,8 +15,10 @@
>> #include <linux/arch_topology.h>
>> #include <linux/cacheinfo.h>
>> #include <linux/cpufreq.h>
>> +#include <linux/cpu_smt.h>
>> #include <linux/init.h>
>> #include <linux/percpu.h>
>> +#include <linux/xarray.h>
>>
>> #include <asm/cpu.h>
>> #include <asm/cputype.h>
>> @@ -37,17 +39,29 @@ static bool __init acpi_cpu_is_threaded(int cpu)
>> return !!is_threaded;
>> }
>>
>> +struct cpu_smt_info {
>> + int thread_num;
>> + int core_id;
>> + int cpu;
>> +};
>> +
>> /*
>> * Propagate the topology information of the processor_topology_node tree to the
>> * cpu_topology array.
>> */
>> int __init parse_acpi_topology(void)
>> {
>> + int max_smt_thread_num = 1;
>> + struct cpu_smt_info *entry;
>> + struct xarray hetero_cpu;
>> + unsigned long hetero_id;
>> int cpu, topology_id;
>>
>> if (acpi_disabled)
>> return 0;
>>
>> + xa_init(&hetero_cpu);
>> +
>> for_each_possible_cpu(cpu) {
>> topology_id = find_acpi_cpu_topology(cpu, 0);
>> if (topology_id < 0)
>> @@ -57,6 +71,30 @@ int __init parse_acpi_topology(void)
>> cpu_topology[cpu].thread_id = topology_id;
>> topology_id = find_acpi_cpu_topology(cpu, 1);
>> cpu_topology[cpu].core_id = topology_id;
>> +
>> + /*
>> + * Build up the XArray using the heterogeneous ID of
>> + * the CPU cluster. Store the CPU and SMT information
>> + * of the first appeared CPU in the CPU cluster of this
>> + * heterogeneous ID since the SMT information should be
>> + * the same in this CPU cluster. Then we can know the
>> + * SMT information of each heterogeneous CPUs in the
>> + * system.
>> + */
>> + hetero_id = find_acpi_cpu_topology_hetero_id(cpu);
>> + entry = (struct cpu_smt_info *)xa_load(&hetero_cpu, hetero_id);
>> + if (!entry) {
>> + entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>> + WARN_ON(!entry);
>> +
>> + entry->cpu = cpu;
>> + entry->core_id = topology_id;
>> + entry->thread_num = 1;
>> + xa_store(&hetero_cpu, hetero_id,
>> + entry, GFP_KERNEL);
>> + } else if (entry->core_id == topology_id) {
>> + entry->thread_num++;
>> + }
>> } else {
>> cpu_topology[cpu].thread_id = -1;
>> cpu_topology[cpu].core_id = topology_id;
>> @@ -67,6 +105,28 @@ int __init parse_acpi_topology(void)
>> cpu_topology[cpu].package_id = topology_id;
>> }
>>
>> + /*
>> + * This should be a short loop depending on the number of heterogeneous
>> + * CPU clusters. Typically on a homogeneous system there's only one
>> + * entry in the XArray.
>> + */
>> + xa_for_each(&hetero_cpu, hetero_id, entry) {
>> + if (entry->thread_num == 1)
>> + continue;
>
> If a platform has CPUs with:
> - 1 thread
> - X (!= 1) threads
> Then I think that the asymmetry is not detected
Ah ok, I only handle the case where there are several thread numbers except no SMT CPUs in the
system. For this case I was thinking we don't need to handle this since there's only one kind
of SMT core in the system, control should works fine for the SMT CPU clusters and we may not
care about the CPUs with no SMT.
Below code should handle the case if we initialize the max_smt_thread_num to 0. I also
reword the warning messages to match the fact. For heterogeneous SMT topology we still
support control SMT by on/off toggle but not fully support setting the thread number.
int max_smt_thread_num = 0;
[...]
/*
* This should be a short loop depending on the number of heterogeneous
* CPU clusters. Typically on a homogeneous system there's only one
* entry in the XArray.
*/
xa_for_each(&hetero_cpu, hetero_id, entry) {
/*
* If max_smt_thread_num has been initialized and doesn't match
* the thread number of this entry, then the system has
* heterogeneous SMT topology.
*/
if (entry->thread_num != max_smt_thread_num && max_smt_thread_num)
pr_warn_once("Heterogeneous SMT topology is partly supported by SMT control\n");
if (entry->thread_num > max_smt_thread_num)
max_smt_thread_num = entry->thread_num;
xa_erase(&hetero_cpu, hetero_id);
kfree(entry);
}
Thanks.
>
>> +
>> + if (entry->thread_num != max_smt_thread_num &&
>> + max_smt_thread_num != 1)
>> + pr_warn("Heterogeneous SMT topology not handled");
>> +
>> + if (entry->thread_num > max_smt_thread_num)
>> + max_smt_thread_num = entry->thread_num;
>> +
>> + xa_erase(&hetero_cpu, hetero_id);
>> + kfree(entry);
>> + }
>> +
>> + cpu_smt_set_num_threads(max_smt_thread_num, max_smt_thread_num);
>> + xa_destroy(&hetero_cpu);
>> return 0;
>> }
>> #endif
>
> .
Powered by blists - more mailing lists