| 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
| ||
|
Date: Thu, 19 Oct 2017 11:54:22 -0500
From: Jeremy Linton <jeremy.linton@....com>
To: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc: linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
sudeep.holla@....com, hanjun.guo@...aro.org, rjw@...ysocki.net,
will.deacon@....com, catalin.marinas@....com,
gregkh@...uxfoundation.org, viresh.kumar@...aro.org,
mark.rutland@....com, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, jhugo@...eaurora.org,
wangxiongfeng2@...wei.com, Jonathan.Zhang@...ium.com,
ahs3@...hat.com, Jayachandran.Nair@...ium.com,
austinwc@...eaurora.org
Subject: Re: [PATCH v3 6/7] arm64: topology: Enable ACPI/PPTT based CPU
topology.
Hi,
I missed the rest of the comment below..
On 10/19/2017 10:56 AM, Lorenzo Pieralisi wrote:
> On Thu, Oct 12, 2017 at 02:48:55PM -0500, Jeremy Linton wrote:
>> Propagate the topology information from the PPTT tree to the
>> cpu_topology array. We can get the thread id, core_id and
>> cluster_id by assuming certain levels of the PPTT tree correspond
>> to those concepts. The package_id is flagged in the tree and can be
>> found by passing an arbitrary large level to setup_acpi_cpu_topology()
>> which terminates its search when it finds an ACPI node flagged
>> as the physical package. If the tree doesn't contain enough
>> levels to represent all of thread/core/cod/package then the package
>> id will be used for the missing levels.
>>
>> Since server/ACPI machines are more likely to be multisocket and NUMA,
>
> I think this stuff is vague enough already so to start with I would drop
> patch 4 and 5 and stop assuming what machines are more likely to ship
> with ACPI than DT.
>
> I am just saying, for the umpteenth time, that these levels have no
> architectural meaning _whatsoever_, level is a hierarchy concept
> with no architectural meaning attached.
>
> The only consistent thing PPTT is bringing about is the hierarchy
> levels/grouping (and _possibly_ - what a package boundary is), let's
> stick to that for the time being.
>
>> this patch also modifies the default clusters=sockets behavior
>> for ACPI machines to sockets=sockets. DT machines continue to
>> represent sockets as clusters. For ACPI machines, this results in a
>> more normalized view of the topology. Cluster level scheduler decisions
>> are still being made due to the "MC" level in the scheduler which has
>> knowledge of cache sharing domains.
>>
>> This code is loosely based on a combination of code from:
>> Xiongfeng Wang <wangxiongfeng2@...wei.com>
>> John Garry <john.garry@...wei.com>
>> Jeffrey Hugo <jhugo@...eaurora.org>
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@....com>
>> ---
>> arch/arm64/kernel/topology.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/topology.h | 1 +
>> 2 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 9147e5b6326d..42f3e7f28b2b 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -11,6 +11,7 @@
>> * for more details.
>> */
>>
>> +#include <linux/acpi.h>
>> #include <linux/arch_topology.h>
>> #include <linux/cpu.h>
>> #include <linux/cpumask.h>
>> @@ -22,6 +23,7 @@
>> #include <linux/sched.h>
>> #include <linux/sched/topology.h>
>> #include <linux/slab.h>
>> +#include <linux/smp.h>
>> #include <linux/string.h>
>>
>> #include <asm/cpu.h>
>> @@ -304,6 +306,54 @@ static void __init reset_cpu_topology(void)
>> }
>> }
>>
>> +#ifdef CONFIG_ACPI
>> +/*
>> + * Propagate the topology information of the processor_topology_node tree to the
>> + * cpu_topology array.
>> + */
>> +static int __init parse_acpi_topology(void)
>> +{
>> + u64 is_threaded;
>> + int cpu;
>> + int topology_id;
>> + /* set a large depth, to hit ACPI_PPTT_PHYSICAL_PACKAGE if one exists */
>> + const int max_topo = 0xFF;
>> +
>> + is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
>> +
>> + for_each_possible_cpu(cpu) {
>> + topology_id = setup_acpi_cpu_topology(cpu, 0);
>> + if (topology_id < 0)
>> + return topology_id;
>> +
>> + if (is_threaded) {
>> + cpu_topology[cpu].thread_id = topology_id;
>> + topology_id = setup_acpi_cpu_topology(cpu, 1);
>
> Nit: you can move setup_acpi_cpu_topology() to include/linux/acpi.h,
> provide an empty inline function for the !ACPI case and remove
> this function ACPI ifdeffery.
Yah sure..
>
>> + cpu_topology[cpu].core_id = topology_id;
>> + topology_id = setup_acpi_cpu_topology(cpu, 2);
>> + cpu_topology[cpu].cluster_id = topology_id;
>> + topology_id = setup_acpi_cpu_topology(cpu, max_topo);
>
> If you want a package id (that's just a package tag to group cores), you
> should not use a large level because you know how setup_acpi_cpu_topology()works, you should add an API that allows you to retrieve the package id
> (so that you can use th ACPI_PPTT_PHYSICAL_PACKAGE flag consistenly,
> whatever it represents).
I don't think the spec requires the use of PHYSICAL_PACKAGE... Am I
misreading it? Which means we need to "pick" a node level to represent
the physical package if one doesn't exist...
>
> Lorenzo
>
>> + cpu_topology[cpu].package_id = topology_id;
>> + } else {
>> + cpu_topology[cpu].thread_id = -1;
>> + cpu_topology[cpu].core_id = topology_id;
>> + topology_id = setup_acpi_cpu_topology(cpu, 1);
>> + cpu_topology[cpu].cluster_id = topology_id;
>> + topology_id = setup_acpi_cpu_topology(cpu, max_topo);
>> + cpu_topology[cpu].package_id = topology_id;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +#else
>> +static int __init parse_acpi_topology(void)
>> +{
>> + /*ACPI kernels should be built with PPTT support*/
>> + return -EINVAL;
>> +}
>> +#endif
>> +
>> void __init init_cpu_topology(void)
>> {
>> reset_cpu_topology();
>> @@ -312,6 +362,8 @@ void __init init_cpu_topology(void)
>> * Discard anything that was parsed if we hit an error so we
>> * don't use partial information.
>> */
>> - if (of_have_populated_dt() && parse_dt_topology())
>> + if ((!acpi_disabled) && parse_acpi_topology())
>> + reset_cpu_topology();
>> + else if (of_have_populated_dt() && parse_dt_topology())
>> reset_cpu_topology();
>> }
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index 4660749a7303..cbf2fb13bf92 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -43,6 +43,7 @@
>> if (nr_cpus_node(node))
>>
>> int arch_update_cpu_topology(void);
>> +int setup_acpi_cpu_topology(unsigned int cpu, int level);
>>
>> /* Conform to ACPI 2.0 SLIT distance definitions */
>> #define LOCAL_DISTANCE 10
>> --
>> 2.13.5
>>
Powered by blists - more mailing lists