[v4,2/2] arm64: topology: Use PPTT to determine if PE is a thread

Message ID 20190801034634.26913-3-jeremy.linton@arm.com
State New
Headers show
Series
  • arm64/PPTT ACPI 6.3 thread flag support
Related show

Commit Message

Jeremy Linton Aug. 1, 2019, 3:46 a.m.
ACPI 6.3 adds a thread flag to represent if a CPU/PE is
actually a thread. Given that the MPIDR_MT bit may not
represent this information consistently on homogeneous machines
we should prefer the PPTT flag if its available.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

---
 arch/arm64/kernel/topology.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.21.0

Comments

Sudeep Holla Aug. 1, 2019, 3:58 p.m. | #1
On Wed, Jul 31, 2019 at 10:46:34PM -0500, Jeremy Linton wrote:
> ACPI 6.3 adds a thread flag to represent if a CPU/PE is

> actually a thread. Given that the MPIDR_MT bit may not

> represent this information consistently on homogeneous machines

> we should prefer the PPTT flag if its available.

>


Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>


--
Regards,
Sudeep
Robert Richter Aug. 2, 2019, 1:44 p.m. | #2
On 31.07.19 22:46:34, Jeremy Linton wrote:

> @@ -358,6 +356,10 @@ static int __init parse_acpi_topology(void)

>  		if (topology_id < 0)

>  			return topology_id;

>  

> +		is_threaded = acpi_pptt_cpu_is_thread(cpu);

> +		if (is_threaded < 0)

> +			is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;

> +


I think the return code handling is error-prone, as in the kernel such
functions are typically used like:

	if (something_is_thread) { ... }

I see this is due to acpi and arch code separation so we cannot simply
move the fallback to pptt code.

So maybe we have a static function cpu_is_thread() in this file that
handles all the logic and directly use check_acpi_cpu_flag() from
there. However, code may change here in case of a rework as I
suggested in patch #1. In both cases the acpi api is more straight
then.

-Robert

>  		if (is_threaded) {

>  			cpu_topology[cpu].thread_id = topology_id;

>  			topology_id = find_acpi_cpu_topology(cpu, 1);
Jeremy Linton Aug. 2, 2019, 4:04 p.m. | #3
Hi,


On 8/2/19 8:44 AM, Robert Richter wrote:
> On 31.07.19 22:46:34, Jeremy Linton wrote:

> 

>> @@ -358,6 +356,10 @@ static int __init parse_acpi_topology(void)

>>   		if (topology_id < 0)

>>   			return topology_id;

>>   

>> +		is_threaded = acpi_pptt_cpu_is_thread(cpu);

>> +		if (is_threaded < 0)

>> +			is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;

>> +

> 

> I think the return code handling is error-prone, as in the kernel such

> functions are typically used like:

> 

> 	if (something_is_thread) { ... }


I don't really understand why this keeps getting repeated. The negative 
error code return is used by huge swaths of the kernel API. A couple 
lines up the exact same paradigm is used in get_cpu_for_node() and a few 
other places.

> 

> I see this is due to acpi and arch code separation so we cannot simply

> move the fallback to pptt code.


Right, the PPTT->arch data structure translation is arch specific. 
During the initial PPTT drop a lot of discussion when into how arm64 was 
doing that translation, as well as the corresponding translation to the 
core scheduler/etc.

> 

> So maybe we have a static function cpu_is_thread() in this file that

> handles all the logic and directly use check_acpi_cpu_flag() from

> there. However, code may change here in case of a rework as I

> suggested in patch #1. In both cases the acpi api is more straight

> then.


I'm ok with that, it effectively only moves those three lines to a 
standalone single call-site function. To be clear, that isn't a generic 
routine for anyone to call. Functions that need to know if the core is a 
threaded should be checking the topology thread_id directly rather than 
re-coding the acpi/dt/mpidr logic which populates it.


> 

> -Robert

> 

>>   		if (is_threaded) {

>>   			cpu_topology[cpu].thread_id = topology_id;

>>   			topology_id = find_acpi_cpu_topology(cpu, 1);

Patch

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 0825c4a856e3..cbbedb53cf06 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -346,11 +346,9 @@  void remove_cpu_topology(unsigned int cpu)
  */
 static int __init parse_acpi_topology(void)
 {
-	bool is_threaded;
+	int is_threaded;
 	int cpu, topology_id;
 
-	is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
-
 	for_each_possible_cpu(cpu) {
 		int i, cache_id;
 
@@ -358,6 +356,10 @@  static int __init parse_acpi_topology(void)
 		if (topology_id < 0)
 			return topology_id;
 
+		is_threaded = acpi_pptt_cpu_is_thread(cpu);
+		if (is_threaded < 0)
+			is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
+
 		if (is_threaded) {
 			cpu_topology[cpu].thread_id = topology_id;
 			topology_id = find_acpi_cpu_topology(cpu, 1);