| Message ID | 20190801034634.26913-3-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | arm64/PPTT ACPI 6.3 thread flag support | expand |
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
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);
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);
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);
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(-)