[PATCH 0/2] arm64 topology updates

[PATCH 0/2] arm64 topology updates

[Zi Shen Lim]

First patch in this series purges unused mc_capable() and smt_capable(),
similar to what went into 3.15-rc1 for other architectures. [1]

Since this is fairly straightforward, please pick this one up soon, so
we're in line with other architectures.

Second patch in this series adds MPIDR-based detection of cpu topology.
MPIDR support is independent of topology information from DT or ACPI.
MPIDR should always work for hardware that sets those bits with sane values.

For cpu topology, MPIDR can be used as default method or as fallback for
DT or ACPI. I lean towards MPIDR as default (at least on host/hypervisor),
but will leave it up to the maintainers on detection priority :)

I've tested this on my model with MT and it works for me.
The code should cover known use cases. I hope to get confirmation,
or more discussions, from others.

This series applies on 3.15-rc1.

Note that this series is functionally orthogonal to the series Mark Brown
just posted. In off-list discussion with Mark, he's currently focusing on
DT-related bits. For reference, his patches are:

  [PATCH 1/4] arm64: topology: Initialise default topology state immediately
  [PATCH 2/4] arm64: topology: Add support for topology DT bindings
  [PATCH 3/4] arm64: topology: Tell the scheduler about the relative power of cores
  [PATCH 4/4] arm64: topology: Provide relative power numbers for cores

Thanks,
z

References:
[1] Clean up from Bjorn Helgaas 36fc5500bb19 ("sched: Remove unused
    mc_capable() and smt_capable()"). Link: https://lkml.org/lkml/2014/3/11/463

Links to Mark's series:
[1/4] http://www.spinics.net/lists/arm-kernel/msg324508.html
[2/4] http://www.spinics.net/lists/arm-kernel/msg324507.html
[3/4] http://www.spinics.net/lists/arm-kernel/msg324505.html
[4/4] http://www.spinics.net/lists/arm-kernel/msg324506.html

Zi Shen Lim (2):
  arm64, sched: Remove unused mc_capable() and smt_capable()
  arm64: topology: add MPIDR-based detection

 arch/arm64/include/asm/cputype.h  |  2 ++
 arch/arm64/include/asm/topology.h |  3 ---
 arch/arm64/kernel/topology.c      | 31 +++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 3 deletions(-)

-- 
1.8.4.3

[PATCH 1/2] arm64, sched: Remove unused mc_capable() and smt_capable()

[Zi Shen Lim]

Remove unused and deprecated mc_capable() and smt_capable().

Both were added recently by f6e763b93a6c ("arm64: topology:
Implement basic CPU topology support"). Uses of both were removed
by 8e7fbcbc22c1 ("sched: Remove stale power aware scheduling
remnants and dysfunctional knobs").

Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
---
 arch/arm64/include/asm/topology.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 0172e6d..7ebcd31 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -20,9 +20,6 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
 #define topology_core_cpumask(cpu)	(&cpu_topology[cpu].core_sibling)
 #define topology_thread_cpumask(cpu)	(&cpu_topology[cpu].thread_sibling)
 
-#define mc_capable()	(cpu_topology[0].cluster_id != -1)
-#define smt_capable()	(cpu_topology[0].thread_id != -1)
-
 void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
-- 
1.8.4.3

Re: [PATCH 1/2] arm64, sched: Remove unused mc_capable() and smt_capable()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

On Tue, Apr 22, 2014 at 06:40:13PM -0700, Zi Shen Lim wrote:
> Remove unused and deprecated mc_capable() and smt_capable().
> 
> Both were added recently by f6e763b93a6c ("arm64: topology:
> Implement basic CPU topology support"). Uses of both were removed
> by 8e7fbcbc22c1 ("sched: Remove stale power aware scheduling
> remnants and dysfunctional knobs").

Reviewed-by: Mark Brown <broonie@linaro.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/2] arm64: topology: add MPIDR-based detection

[Zi Shen Lim]

Create cpu topology based on MPIDR. When hardware sets MPIDR to sane
values, this method will always work. Therefore it should also work well
as the fallback method. [1]

[1] http://www.spinics.net/lists/arm-kernel/msg317445.html

Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
---
 arch/arm64/include/asm/cputype.h |  2 ++
 arch/arm64/kernel/topology.c     | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
index c404fb0..7639e8b 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -18,6 +18,8 @@
 
 #define INVALID_HWID		ULONG_MAX
 
+#define MPIDR_UP_BITMASK	(0x1 << 30)
+#define MPIDR_MT_BITMASK	(0x1 << 24)
 #define MPIDR_HWID_BITMASK	0xff00ffffff
 
 #define MPIDR_LEVEL_BITS_SHIFT	3
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 3e06b0b..ef3bb7e 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -19,6 +19,7 @@
 #include <linux/nodemask.h>
 #include <linux/sched.h>
 
+#include <asm/cputype.h>
 #include <asm/topology.h>
 
 /*
@@ -71,6 +72,36 @@ static void update_siblings_masks(unsigned int cpuid)
 
 void store_cpu_topology(unsigned int cpuid)
 {
+	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
+	u64 mpidr;
+
+	mpidr = read_cpuid_mpidr();
+
+	/* Create cpu topology mapping based on MPIDR. */
+	if (mpidr & MPIDR_UP_BITMASK) {
+		/* Uniprocessor system */
+		cpuid_topo->thread_id  = -1;
+		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+		cpuid_topo->cluster_id = -1;
+	} else {
+		/* Multiprocessor system */
+		if (mpidr & MPIDR_MT_BITMASK) {
+			/* Multi-threads per core */
+			cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+			cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+			cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
+		} else {
+			/* Single-thread per core */
+			cpuid_topo->thread_id  = -1;
+			cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+			cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+		}
+	}
+
+	pr_info("CPU%u: cluster %d core %d thread %d mpidr %llx\n",
+		cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
+		cpuid_topo->thread_id, mpidr);
+
 	update_siblings_masks(cpuid);
 }
 
-- 
1.8.4.3

Re: [PATCH 2/2] arm64: topology: add MPIDR-based detection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

On Tue, Apr 22, 2014 at 06:40:14PM -0700, Zi Shen Lim wrote:

> +		/* Multiprocessor system */
> +		if (mpidr & MPIDR_MT_BITMASK) {
> +			/* Multi-threads per core */
> +			cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +			cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +			cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
> +		} else {
> +			/* Single-thread per core */
> +			cpuid_topo->thread_id  = -1;
> +			cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +			cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +		}
> +	}

This means that we ignore affinity level 3 and on non-MT cores we ignore
affinity level 2.  That means that if it runs on some system where we do
have multiple levels of clustering (for example some future multi socket
server) or if for some reason the hardware engineers have decided to use
one of the higher affinity levels then we will incorrectly report cores
from several clusters as being part of a single cluster.

I had been intending to just combine all the bits from affinitly levels
above the CPU number into a single number until we know what to do with
them individually.  We shouldn't just ignore them.

> +	pr_info("CPU%u: cluster %d core %d thread %d mpidr %llx\n",
> +		cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
> +		cpuid_topo->thread_id, mpidr);
> +

Catalin or Lorenzo asked for stuff like that to be taken out.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] arm64: topology: add MPIDR-based detection

[Zi Shen Lim]

On Wed, Apr 23, 2014 at 11:36:48AM +0100, Mark Brown wrote:
> On Tue, Apr 22, 2014 at 06:40:14PM -0700, Zi Shen Lim wrote:
> 
> > +		/* Multiprocessor system */
> > +		if (mpidr & MPIDR_MT_BITMASK) {
> > +			/* Multi-threads per core */
> > +			cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > +			cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> > +			cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
> > +		} else {
> > +			/* Single-thread per core */
> > +			cpuid_topo->thread_id  = -1;
> > +			cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > +			cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> > +		}
> > +	}
> 
> This means that we ignore affinity level 3 and on non-MT cores we ignore
> affinity level 2.  That means that if it runs on some system where we do
> have multiple levels of clustering (for example some future multi socket
> server) or if for some reason the hardware engineers have decided to use
> one of the higher affinity levels then we will incorrectly report cores
> from several clusters as being part of a single cluster.
> 

Good points.

"Cluster" in my case is actually "Socket".

I guess single-socket in b.L configuration could have multiple clusters,
in which case, a new 'socket_id' would map to aff3 in MT case.

	thread	= aff0
	core	= aff1
	cluster	= aff2
	socket	= aff3

For non-MT, it could very well be aff2?

	thread	= -1
	core	= aff0
	cluster	= aff1
	socket	= aff2

Or is it likely that some folks may opt to skip aff2, and simply use aff3?
Mark, is there precedence for such usage of affinity levels?

Maybe ARM folks can point us to documentation or conventions w.r.t.
usage of those affinity levels.  Otherwise, this has potential to become
a creative playground :)

> I had been intending to just combine all the bits from affinitly levels
> above the CPU number into a single number until we know what to do with
> them individually.  We shouldn't just ignore them.
> 

I agree we shouldn't ignore aff3 if someone is already using it.

I'm not sure how combining them into a single number helps with topology.
We already started out with a cpuid, no?

Perhaps we should just add a new 'socket_id' and that will accommodate
all cases (up to aff3).

> > +	pr_info("CPU%u: cluster %d core %d thread %d mpidr %llx\n",
> > +		cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
> > +		cpuid_topo->thread_id, mpidr);
> > +
> 
> Catalin or Lorenzo asked for stuff like that to be taken out.

Is it ok to change it pr_debug instead? Or drop it completely?

Re: [PATCH 2/2] arm64: topology: add MPIDR-based detection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]

On Wed, Apr 23, 2014 at 10:27:20AM -0700, Zi Shen Lim wrote:

> Or is it likely that some folks may opt to skip aff2, and simply use aff3?
> Mark, is there precedence for such usage of affinity levels?

Not that I'm aware of at the minute myself but of course this code may
end up running on some enterprise distribution with an extended support
time with hardware that isn't even on the drawing board now.

> > I had been intending to just combine all the bits from affinitly levels
> > above the CPU number into a single number until we know what to do with
> > them individually.  We shouldn't just ignore them.

> I agree we shouldn't ignore aff3 if someone is already using it.

> I'm not sure how combining them into a single number helps with topology.
> We already started out with a cpuid, no?

It will at least ensure that all clusters get assigned a unique ID and
we don't end up discarding some of the information and coming out with
two identically numbered clusters which then have identically numbered
CPUs inside of them which doesn't seem clever.

When I was looking at this it wasn't sufficiently clear to me that the
cluster clustering would be well modelled by sockets as the scheduler
currently assumes them, nor what to do with additional levels of that
(the DT binding allows for infinite levels).  Punting and just putting
all clusters at the same level avoids active bugs and seems fairly
conservative.

> Perhaps we should just add a new 'socket_id' and that will accommodate
> all cases (up to aff3).

Not in the non-MT case where we've got two levels above the cluster ID
in affinity level 1 unless we just combine 2 and 3 (which would be
reasonable enough of course).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] arm64: topology: add MPIDR-based detection

[Zi Shen Lim]

On Wed, Apr 23, 2014 at 07:26:11PM +0100, Mark Brown wrote:
> On Wed, Apr 23, 2014 at 10:27:20AM -0700, Zi Shen Lim wrote:
> 
> It will at least ensure that all clusters get assigned a unique ID and
> we don't end up discarding some of the information and coming out with
> two identically numbered clusters which then have identically numbered
> CPUs inside of them which doesn't seem clever.

I agree with you. Simply ignoring aff3 is not acceptable, whether or not
someone is using it.

> 
> When I was looking at this it wasn't sufficiently clear to me that the
> cluster clustering would be well modelled by sockets as the scheduler
> currently assumes them, nor what to do with additional levels of that
> (the DT binding allows for infinite levels).  Punting and just putting
> all clusters at the same level avoids active bugs and seems fairly
> conservative.
> 

Sounds like you prefer "cluster of clusters" over "socket", correct?

In any case, with only 4 affinity levels defined in the arch, as long as
we also have 4 variables to capture that information, we should be good,
right?

Anything more exotic not expressable by these 4 affinity levels in MPIDR
will require additional information from other sources such as DT or ACPI.

> > Perhaps we should just add a new 'socket_id' and that will accommodate
> > all cases (up to aff3).
> 
> Not in the non-MT case where we've got two levels above the cluster ID
> in affinity level 1 unless we just combine 2 and 3 (which would be
> reasonable enough of course).

Is the following an accurate description of your proposal for non-MT?

	thread_id   = -1
	core_id     = aff0
	cluster_id  = aff1
	clusters_id = combine(aff2,aff3)

Re: [PATCH 2/2] arm64: topology: add MPIDR-based detection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

On Wed, Apr 23, 2014 at 12:22:28PM -0700, Zi Shen Lim wrote:
> On Wed, Apr 23, 2014 at 07:26:11PM +0100, Mark Brown wrote:

> > When I was looking at this it wasn't sufficiently clear to me that the
> > cluster clustering would be well modelled by sockets as the scheduler
> > currently assumes them, nor what to do with additional levels of that
> > (the DT binding allows for infinite levels).  Punting and just putting
> > all clusters at the same level avoids active bugs and seems fairly
> > conservative.

> Sounds like you prefer "cluster of clusters" over "socket", correct?

I don't actually care that much, I guess I was using the hardware
terminology because I'm not sure how well it will map onto the current
Linux software model of what a socket is but if we decide it maps well
onto a socket that's fine also.

> In any case, with only 4 affinity levels defined in the arch, as long as
> we also have 4 variables to capture that information, we should be good,
> right?

> Anything more exotic not expressable by these 4 affinity levels in MPIDR
> will require additional information from other sources such as DT or ACPI.

Yes.  I'd really only thought it through properly for the DT case since
at the time Catalin was saying that it would not be possible to use
MPIDR.

> > > Perhaps we should just add a new 'socket_id' and that will accommodate
> > > all cases (up to aff3).

> > Not in the non-MT case where we've got two levels above the cluster ID
> > in affinity level 1 unless we just combine 2 and 3 (which would be
> > reasonable enough of course).

> Is the following an accurate description of your proposal for non-MT?

> 	thread_id   = -1
> 	core_id     = aff0
> 	cluster_id  = aff1
> 	clusters_id = combine(aff2,aff3)

Yes, exactly in the combining case - probably just shift one of them
left and or them together.  Otherwise just only have the cluster_id and
combine everything else into one (that's what the DT code does
effectively).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]