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

[Jeremy Linton <jeremy.linton@arm.com>]

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);