Re: [PATCH 1/5] powerpc/smp: use cpu_to_chip_id() to find siblings

On Wed, Mar 15, 2017 at 10:18 PM, Michael Ellerman [off-list ref] wrote:

Oliver O'Halloran [off-list ref] writes:

quoted

To determine which logical CPUs are on the same core the kernel uses the
ibm,chipid property from the device tree node associated with that cpu.
The lookup for this this information is currently open coded in both
traverse_siblings() and traverse_siblings_chip_id(). This patch replaces
these manual lookups with the existing cpu_to_chip_id() function.

Some minor nits.

cpu_to_chip_id() actually searches recursively up the parents until it
finds a ibm,chip-id, so it's not a 1:1 replacement for the existing
logic, but it's probably still an OK conversion. It's still worth
mentioning in the change log thought.

fair enough

quoted

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 893bd7f79be6..dfe0e1d9cd06 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c

@@ -664,23 +655,19 @@ static void traverse_core_siblings(int cpu, bool add)
 {
      struct device_node *l2_cache, *np;
      const struct cpumask *mask;
-     int i, chip, plen;
-     const __be32 *prop;
+     int chip_id;
+     int i;

-     /* First see if we have ibm,chip-id properties in cpu nodes */
-     np = of_get_cpu_node(cpu, NULL);
-     if (np) {
-             chip = -1;
-             prop = of_get_property(np, "ibm,chip-id", &plen);
-             if (prop && plen == sizeof(int))
-                     chip = of_read_number(prop, 1);
-             of_node_put(np);
-             if (chip >= 0) {
-                     traverse_siblings_chip_id(cpu, add, chip);
-                     return;
-             }
+     /* threads that share a chip-id are considered siblings (same die) */

You might know it means the "same die", but AFAIK there's no actual
definition for what the chip-id means, so let's not write comments that
might be wrong in future. Just saying they're considered siblings is
sufficient.

Also "Threads" :)

The cpus masks are all built in terms of threads, so this is
technically correct even if it sounds stupid. Maybe "logical cpus"
would be better?

cheers