Re: [PATCH v5 8/9] arm64: topology: Enable ACPI/PPTT based CPU topology.

Lorenzo Pieralisi
2017-12-13T18:21:43Z

Nit: remove the period in $SUBJECT and capitalize with a coherent
policy for the patches touching the same code.

On Fri, Dec 01, 2017 at 04:23:29PM -0600, 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 calling find_acpi_cpu_topology_package() 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 the requested levels then the root node will be returned
> for all subsequent levels.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> arch/arm64/kernel/topology.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
> include/linux/topology.h | 2 ++
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 74a8a5173a35..198714aca9e8 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>
> @@ -300,6 +302,47 @@ 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;

Nit: a bool would be preferable.

> + int cpu;
> + int topology_id;

int cpu, topology_id;

> + is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;

> + for_each_possible_cpu(cpu) {
> + topology_id = find_acpi_cpu_topology(cpu, 0);
> + if (topology_id < 0)
> + return topology_id;
> +
> + if (is_threaded) {
> + cpu_topology[cpu].thread_id = topology_id;
> + topology_id = find_acpi_cpu_topology(cpu, 1);
> + cpu_topology[cpu].core_id = topology_id;
> + topology_id = find_acpi_cpu_topology_package(cpu);
> + cpu_topology[cpu].physical_id = topology_id;
> + } else {
> + cpu_topology[cpu].thread_id = -1;
> + cpu_topology[cpu].core_id = topology_id;
> + topology_id = find_acpi_cpu_topology_package(cpu);
> + cpu_topology[cpu].physical_id = topology_id;
> + }
> + }

Add a space.

It is probably my fault so apologies if that's the case. The

find_acpi_cpu_topology()

API is a bit strange since it behaves differently according to the
level passed in.

I think it is better to define two calls (it might well have been like
that in one of the previous series versions):

- find_acpi_cpu_package_level() (returns: package level if success, <0 on
failure)
- acpi_cpu_topology_id()

It would even be better to lump the two calls together but you do not
know how many topology levels are there so it becomes a bit complicated
to handle.

> + return 0;
> +}
> +
> +#else
> +static int __init parse_acpi_topology(void)

static inline ?

> +{
> + /*ACPI kernels should be built with PPTT support*/

I think you can remove this comment.

> + return -EINVAL;
> +}
> +#endif
>
> void __init init_cpu_topology(void)
> {
> @@ -309,6 +352,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 cb0775e1ee4b..170ce87edd88 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -43,6 +43,8 @@
> if (nr_cpus_node(node))
>
> int arch_update_cpu_topology(void);
> +int find_acpi_cpu_topology(unsigned int cpu, int level);
> +int find_acpi_cpu_topology_package(unsigned int cpu);

I do not think these two declarations:

a) belong in this patch
b) belong in include/linux/topology.h (should be acpi.h)

Lorenzo