From: Jeremy Linton <jeremy.linton@arm.com> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com, hanjun.guo@linaro.org, rjw@rjwysocki.net, will.deacon@arm.com, catalin.marinas@arm.com, gregkh@linuxfoundation.org, viresh.kumar@linaro.org, mark.rutland@arm.com, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, jhugo@codeaurora.org, wangxiongfeng2@huawei.com, Jonathan.Zhang@cavium.com, ahs3@redhat.com, Jayachandran.Nair@cavium.com, austinwc@codeaurora.org Subject: Re: [PATCH v3 6/7] arm64: topology: Enable ACPI/PPTT based CPU topology. Date: Thu, 19 Oct 2017 11:54:22 -0500 [thread overview] Message-ID: <b6c3fae3-eb6d-e5f2-6887-de1471978897@arm.com> (raw) In-Reply-To: <20171019155622.GC18883@red-moon> 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@huawei.com> >> John Garry <john.garry@huawei.com> >> Jeffrey Hugo <jhugo@codeaurora.org> >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.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 >>
WARNING: multiple messages have this Message-ID (diff)
From: jeremy.linton@arm.com (Jeremy Linton) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 6/7] arm64: topology: Enable ACPI/PPTT based CPU topology. Date: Thu, 19 Oct 2017 11:54:22 -0500 [thread overview] Message-ID: <b6c3fae3-eb6d-e5f2-6887-de1471978897@arm.com> (raw) In-Reply-To: <20171019155622.GC18883@red-moon> 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@huawei.com> >> John Garry <john.garry@huawei.com> >> Jeffrey Hugo <jhugo@codeaurora.org> >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.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 >>
next prev parent reply other threads:[~2017-10-19 16:54 UTC|newest] Thread overview: 104+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-12 19:48 [PATCH v3 0/7] Support PPTT for ARM64 Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-12 19:48 ` [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-13 9:56 ` Julien Thierry 2017-10-13 9:56 ` Julien Thierry 2017-10-13 22:41 ` Jeremy Linton 2017-10-13 22:41 ` Jeremy Linton 2017-10-13 14:23 ` tn 2017-10-13 14:23 ` tn 2017-10-13 19:58 ` Jeremy Linton 2017-10-13 19:58 ` Jeremy Linton 2017-10-16 14:24 ` John Garry 2017-10-16 14:24 ` John Garry 2017-10-16 14:24 ` John Garry 2017-10-17 13:25 ` Tomasz Nowicki 2017-10-17 13:25 ` Tomasz Nowicki 2017-10-17 15:22 ` Jeremy Linton 2017-10-17 15:22 ` Jeremy Linton 2017-10-18 1:10 ` Xiongfeng Wang 2017-10-18 1:10 ` Xiongfeng Wang 2017-10-18 1:10 ` Xiongfeng Wang 2017-10-18 5:39 ` Tomasz Nowicki 2017-10-18 5:39 ` Tomasz Nowicki 2017-10-18 10:24 ` Tomasz Nowicki 2017-10-18 10:24 ` Tomasz Nowicki 2017-10-18 17:30 ` Jeremy Linton 2017-10-18 17:30 ` Jeremy Linton 2017-10-19 5:18 ` Tomasz Nowicki 2017-10-19 5:18 ` Tomasz Nowicki 2017-10-19 10:25 ` John Garry 2017-10-19 10:25 ` John Garry 2017-10-19 10:25 ` John Garry 2017-10-27 5:21 ` Tomasz Nowicki 2017-10-27 5:21 ` Tomasz Nowicki 2017-10-19 14:24 ` Jeremy Linton 2017-10-19 14:24 ` Jeremy Linton 2017-10-19 10:22 ` Lorenzo Pieralisi 2017-10-19 10:22 ` Lorenzo Pieralisi 2017-10-19 15:43 ` Jeremy Linton 2017-10-19 15:43 ` Jeremy Linton 2017-10-20 10:15 ` Lorenzo Pieralisi 2017-10-20 10:15 ` Lorenzo Pieralisi 2017-10-20 19:53 ` Christ, Austin 2017-10-20 19:53 ` Christ, Austin 2017-10-23 21:14 ` Jeremy Linton 2017-10-23 21:14 ` Jeremy Linton 2017-10-12 19:48 ` [PATCH v3 2/7] ACPI: Enable PPTT support on ARM64 Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-13 9:53 ` Hanjun Guo 2017-10-13 9:53 ` Hanjun Guo 2017-10-13 9:53 ` Hanjun Guo 2017-10-13 17:51 ` Jeremy Linton 2017-10-13 17:51 ` Jeremy Linton 2017-10-18 16:47 ` Lorenzo Pieralisi 2017-10-18 16:47 ` Lorenzo Pieralisi 2017-10-18 17:38 ` Jeremy Linton 2017-10-18 17:38 ` Jeremy Linton 2017-10-19 9:12 ` Lorenzo Pieralisi 2017-10-19 9:12 ` Lorenzo Pieralisi 2017-10-12 19:48 ` [PATCH v3 3/7] drivers: base: cacheinfo: arm64: Add support for ACPI based firmware tables Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-19 15:20 ` Lorenzo Pieralisi 2017-10-19 15:20 ` Lorenzo Pieralisi 2017-10-19 15:52 ` Jeremy Linton 2017-10-19 15:52 ` Jeremy Linton 2017-10-12 19:48 ` [PATCH v3 4/7] Topology: Add cluster on die macros and arm64 decoding Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-12 19:48 ` [PATCH v3 5/7] arm64: Fixup users of topology_physical_package_id Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-12 19:48 ` [PATCH v3 6/7] arm64: topology: Enable ACPI/PPTT based CPU topology Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-19 15:56 ` Lorenzo Pieralisi 2017-10-19 15:56 ` Lorenzo Pieralisi 2017-10-19 16:13 ` Jeremy Linton 2017-10-19 16:13 ` Jeremy Linton 2017-10-20 9:14 ` Lorenzo Pieralisi 2017-10-20 9:14 ` Lorenzo Pieralisi 2017-10-20 16:14 ` Jeremy Linton 2017-10-20 16:14 ` Jeremy Linton 2017-10-20 16:42 ` Sudeep Holla 2017-10-20 16:42 ` Sudeep Holla 2017-10-20 19:55 ` Jeffrey Hugo 2017-10-20 19:55 ` Jeffrey Hugo 2017-10-23 21:26 ` Jeremy Linton 2017-10-23 21:26 ` Jeremy Linton 2017-10-19 16:54 ` Jeremy Linton [this message] 2017-10-19 16:54 ` Jeremy Linton 2017-10-20 9:22 ` Lorenzo Pieralisi 2017-10-20 9:22 ` Lorenzo Pieralisi 2017-11-01 20:29 ` Al Stone 2017-11-01 20:29 ` Al Stone 2017-11-02 10:48 ` Lorenzo Pieralisi 2017-11-02 10:48 ` Lorenzo Pieralisi 2017-10-12 19:48 ` [PATCH v3 7/7] ACPI: Add PPTT to injectable table list Jeremy Linton 2017-10-12 19:48 ` Jeremy Linton 2017-10-13 11:08 ` [PATCH v3 0/7] Support PPTT for ARM64 John Garry 2017-10-13 11:08 ` John Garry 2017-10-13 11:08 ` John Garry 2017-10-13 19:34 ` Jeremy Linton 2017-10-13 19:34 ` Jeremy Linton 2017-10-31 12:46 ` Jon Masters 2017-10-31 12:46 ` Jon Masters
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b6c3fae3-eb6d-e5f2-6887-de1471978897@arm.com \ --to=jeremy.linton@arm.com \ --cc=Jayachandran.Nair@cavium.com \ --cc=Jonathan.Zhang@cavium.com \ --cc=ahs3@redhat.com \ --cc=austinwc@codeaurora.org \ --cc=catalin.marinas@arm.com \ --cc=gregkh@linuxfoundation.org \ --cc=hanjun.guo@linaro.org \ --cc=jhugo@codeaurora.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=mark.rutland@arm.com \ --cc=rjw@rjwysocki.net \ --cc=sudeep.holla@arm.com \ --cc=viresh.kumar@linaro.org \ --cc=wangxiongfeng2@huawei.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.