From: Conor Dooley <[email protected]>
Hey all,
It's my first time messing around with arch/ code at all, let alone
more than one arch, so forgive me if I have screwed up how to do a
migration like this.
The goal here is the fix the incorrectly reported arch topology on
RISC-V which seems to have been broken since it was added.
cpu, package and thread IDs are all currently reported as -1, so tools
like lstopo think systems have multiple threads on the same core when
this is not true:
https://github.com/open-mpi/hwloc/issues/536
arm64's topology code basically applies to RISC-V too, so it has been
made generic along with the removal of MPIDR related code, which
appears to be redudant code since '3102bc0e6ac7 ("arm64: topology: Stop
using MPIDR for topology information")' replaced the code that actually
interacted with MPIDR with default values.
I only built tested for arm64, so hopefully it is not broken when used.
Testing on both arm64 & !SMP RISC-V would really be appreciated!
For V2, I dropped the idea of doing a RISC-V specific implementation
followed by a move to the generic code & just went for the more straight
forward method of moving to the shared version first. I also dropped the
RFC :)
Thanks,
Conor.
Conor Dooley (2):
arm64: topology: move store_cpu_topology() to shared code
riscv: topology: fix default topology reporting
arch/arm64/kernel/topology.c | 40 ------------------------------------
arch/riscv/Kconfig | 2 +-
arch/riscv/kernel/smpboot.c | 4 +++-
drivers/base/arch_topology.c | 19 +++++++++++++++++
4 files changed, 23 insertions(+), 42 deletions(-)
base-commit: b6f1f2fa2bddd69ff46a190b8120bd440fd50563
--
2.37.0
From: Conor Dooley <[email protected]>
arm64's method of defining a default cpu topology requires only minimal
changes to apply to RISC-V also. The current arm64 implementation exits
early in a uniprocessor configuration by reading MPIDR & claiming that
uniprocessor can rely on the default values.
This is appears to be a hangover from prior to '3102bc0e6ac7 ("arm64:
topology: Stop using MPIDR for topology information")', because the
current code just assigns default values for multiprocessor systems.
With the MPIDR references removed, store_cpu_topolgy() can be moved to
the common arch_topology code.
CC: [email protected]
Signed-off-by: Conor Dooley <[email protected]>
---
arch/arm64/kernel/topology.c | 40 ------------------------------------
drivers/base/arch_topology.c | 19 +++++++++++++++++
2 files changed, 19 insertions(+), 40 deletions(-)
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 869ffc4d4484..7889a00f5487 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -22,46 +22,6 @@
#include <asm/cputype.h>
#include <asm/topology.h>
-void store_cpu_topology(unsigned int cpuid)
-{
- struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
- u64 mpidr;
-
- if (cpuid_topo->package_id != -1)
- goto topology_populated;
-
- mpidr = read_cpuid_mpidr();
-
- /* Uniprocessor systems can rely on default topology values */
- if (mpidr & MPIDR_UP_BITMASK)
- return;
-
- /*
- * This would be the place to create cpu topology based on MPIDR.
- *
- * However, it cannot be trusted to depict the actual topology; some
- * pieces of the architecture enforce an artificial cap on Aff0 values
- * (e.g. GICv3's ICC_SGI1R_EL1 limits it to 15), leading to an
- * artificial cycling of Aff1, Aff2 and Aff3 values. IOW, these end up
- * having absolutely no relationship to the actual underlying system
- * topology, and cannot be reasonably used as core / package ID.
- *
- * If the MT bit is set, Aff0 *could* be used to define a thread ID, but
- * we still wouldn't be able to obtain a sane core ID. This means we
- * need to entirely ignore MPIDR for any topology deduction.
- */
- cpuid_topo->thread_id = -1;
- cpuid_topo->core_id = cpuid;
- cpuid_topo->package_id = cpu_to_node(cpuid);
-
- pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n",
- cpuid, cpuid_topo->package_id, cpuid_topo->core_id,
- cpuid_topo->thread_id, mpidr);
-
-topology_populated:
- update_siblings_masks(cpuid);
-}
-
#ifdef CONFIG_ACPI
static bool __init acpi_cpu_is_threaded(int cpu)
{
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 441e14ac33a4..07e84c6ac5c2 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
}
}
+void __weak store_cpu_topology(unsigned int cpuid)
+{
+ struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
+
+ if (cpuid_topo->package_id != -1)
+ goto topology_populated;
+
+ cpuid_topo->thread_id = -1;
+ cpuid_topo->core_id = cpuid;
+ cpuid_topo->package_id = cpu_to_node(cpuid);
+
+ pr_debug("CPU%u: package %d core %d thread %d\n",
+ cpuid, cpuid_topo->package_id, cpuid_topo->core_id,
+ cpuid_topo->thread_id);
+
+topology_populated:
+ update_siblings_masks(cpuid);
+}
+
static void clear_cpu_topology(int cpu)
{
struct cpu_topology *cpu_topo = &cpu_topology[cpu];
--
2.37.0
On 08/07/2022 21:33, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> arm64's method of defining a default cpu topology requires only minimal
> changes to apply to RISC-V also. The current arm64 implementation exits
> early in a uniprocessor configuration by reading MPIDR & claiming that
> uniprocessor can rely on the default values.
>
> This is appears to be a hangover from prior to '3102bc0e6ac7 ("arm64:
> topology: Stop using MPIDR for topology information")', because the
> current code just assigns default values for multiprocessor systems.
>
> With the MPIDR references removed, store_cpu_topolgy() can be moved to
> the common arch_topology code.
>
> CC: [email protected]
> Signed-off-by: Conor Dooley <[email protected]>
> ---
> arch/arm64/kernel/topology.c | 40 ------------------------------------
> drivers/base/arch_topology.c | 19 +++++++++++++++++
> 2 files changed, 19 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 869ffc4d4484..7889a00f5487 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -22,46 +22,6 @@
> #include <asm/cputype.h>
> #include <asm/topology.h>
>
> -void store_cpu_topology(unsigned int cpuid)
> -{
> - struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> - u64 mpidr;
> -
> - if (cpuid_topo->package_id != -1)
> - goto topology_populated;
> -
> - mpidr = read_cpuid_mpidr();
> -
> - /* Uniprocessor systems can rely on default topology values */
> - if (mpidr & MPIDR_UP_BITMASK)
> - return;
> -
> - /*
> - * This would be the place to create cpu topology based on MPIDR.
> - *
> - * However, it cannot be trusted to depict the actual topology; some
> - * pieces of the architecture enforce an artificial cap on Aff0 values
> - * (e.g. GICv3's ICC_SGI1R_EL1 limits it to 15), leading to an
> - * artificial cycling of Aff1, Aff2 and Aff3 values. IOW, these end up
> - * having absolutely no relationship to the actual underlying system
> - * topology, and cannot be reasonably used as core / package ID.
> - *
> - * If the MT bit is set, Aff0 *could* be used to define a thread ID, but
> - * we still wouldn't be able to obtain a sane core ID. This means we
> - * need to entirely ignore MPIDR for any topology deduction.
> - */
> - cpuid_topo->thread_id = -1;
> - cpuid_topo->core_id = cpuid;
> - cpuid_topo->package_id = cpu_to_node(cpuid);
> -
> - pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n",
> - cpuid, cpuid_topo->package_id, cpuid_topo->core_id,
> - cpuid_topo->thread_id, mpidr);
> -
> -topology_populated:
> - update_siblings_masks(cpuid);
> -}
> -
> #ifdef CONFIG_ACPI
> static bool __init acpi_cpu_is_threaded(int cpu)
> {
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 441e14ac33a4..07e84c6ac5c2 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
> }
> }
>
> +void __weak store_cpu_topology(unsigned int cpuid)
Ahh crap, I forgot to remove the __weak.
I won't immediately respin since it is minor. I've pushed it (without
the __weak) to https://git.kernel.org/conor/h/arch-topo so it'll get
the lkp coverage.
Thanks,
Conor.
> +{
> + struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> +
> + if (cpuid_topo->package_id != -1)
> + goto topology_populated;
> +
> + cpuid_topo->thread_id = -1;
> + cpuid_topo->core_id = cpuid;
> + cpuid_topo->package_id = cpu_to_node(cpuid);
> +
> + pr_debug("CPU%u: package %d core %d thread %d\n",
> + cpuid, cpuid_topo->package_id, cpuid_topo->core_id,
> + cpuid_topo->thread_id);
> +
> +topology_populated:
> + update_siblings_masks(cpuid);
> +}
> +
> static void clear_cpu_topology(int cpu)
> {
> struct cpu_topology *cpu_topo = &cpu_topology[cpu];
+CC Russel, Arnd
On 08/07/2022 21:45, Conor Dooley - M52691 wrote:
> On 08/07/2022 21:33, Conor Dooley wrote:
>> From: Conor Dooley <[email protected]>
>>
>> arm64's method of defining a default cpu topology requires only minimal
>> changes to apply to RISC-V also. The current arm64 implementation exits
>> early in a uniprocessor configuration by reading MPIDR & claiming that
>> uniprocessor can rely on the default values.
>>
>> This is appears to be a hangover from prior to '3102bc0e6ac7 ("arm64:
>> topology: Stop using MPIDR for topology information")', because the
>> current code just assigns default values for multiprocessor systems.
>>
>> With the MPIDR references removed, store_cpu_topolgy() can be moved to
>> the common arch_topology code.
>>
>> CC: [email protected]
>> Signed-off-by: Conor Dooley <[email protected]>
>> ---
---8<---
>> #ifdef CONFIG_ACPI
>> static bool __init acpi_cpu_is_threaded(int cpu)
>> {
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index 441e14ac33a4..07e84c6ac5c2 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
>> }
>> }
>>
>> +void __weak store_cpu_topology(unsigned int cpuid)
>
> Ahh crap, I forgot to remove the __weak.
> I won't immediately respin since it is minor. I've pushed it (without
> the __weak) to https://git.kernel.org/conor/h/arch-topo so it'll get
> the lkp coverage.
And build failure for arm32:
> tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git arch-topo
> branch HEAD: df379c4b12f6b22fb8c07c2be16fd821a4fcbfc5 riscv: topology: fix default topology reporting
>
> Error/Warning: (recently discovered and may have been fixed)
>
> arch_topology.c:(.text+0xbac): multiple definition of `store_cpu_topology'; arch/arm/kernel/topology.o:topology.c:(.text+0x0): first defined here
>
> Error/Warning ids grouped by kconfigs:
>
> gcc_recent_errors
> `-- arm-defconfig
> `-- multiple-definition-of-store_cpu_topology-arch-arm-kernel-topology.o:topology.c:(.text):first-defined-here
>
> elapsed time: 721m
Looking at the arm32 implementation - it appears to be mostly the sort of MPIDR
stuff that was removed from the arm64 implementation in 3102bc0e6ac7 ("arm64:
topology: Stop using MPIDR for topology information"). Could arm32 benefit from
the same shared implemenation too, or is usage of MPIDR only invalid for arm64?
The other difference is a call to update_cpu_capacity() in the arm32
implementation. Could that be moved to smp_store_cpu_info() which is the only
callsite of store_cpu_topology()?
Either way, will respin a v3 that doesn't break the arm32 build when
CONFIG_GENERIC_ARCH_TOPOLOGY is enabled :)
Thanks,
Conor.
On Sat, Jul 09, 2022 at 12:58:57PM +0000, [email protected] wrote:
> +CC Russel, Arnd
>
> On 08/07/2022 21:45, Conor Dooley - M52691 wrote:
> > On 08/07/2022 21:33, Conor Dooley wrote:
> >> From: Conor Dooley <[email protected]>
> >>
> >> arm64's method of defining a default cpu topology requires only minimal
> >> changes to apply to RISC-V also. The current arm64 implementation exits
> >> early in a uniprocessor configuration by reading MPIDR & claiming that
> >> uniprocessor can rely on the default values.
> >>
> >> This is appears to be a hangover from prior to '3102bc0e6ac7 ("arm64:
> >> topology: Stop using MPIDR for topology information")', because the
> >> current code just assigns default values for multiprocessor systems.
> >>
> >> With the MPIDR references removed, store_cpu_topolgy() can be moved to
> >> the common arch_topology code.
> >>
> >> CC: [email protected]
> >> Signed-off-by: Conor Dooley <[email protected]>
> >> ---
> ---8<---
> >> #ifdef CONFIG_ACPI
> >> static bool __init acpi_cpu_is_threaded(int cpu)
> >> {
> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> >> index 441e14ac33a4..07e84c6ac5c2 100644
> >> --- a/drivers/base/arch_topology.c
> >> +++ b/drivers/base/arch_topology.c
> >> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
> >> }
> >> }
> >>
> >> +void __weak store_cpu_topology(unsigned int cpuid)
> >
> > Ahh crap, I forgot to remove the __weak.
> > I won't immediately respin since it is minor. I've pushed it (without
> > the __weak) to https://git.kernel.org/conor/h/arch-topo so it'll get
> > the lkp coverage.
>
> And build failure for arm32:
>
> > tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git arch-topo
> > branch HEAD: df379c4b12f6b22fb8c07c2be16fd821a4fcbfc5 riscv: topology: fix default topology reporting
> >
> > Error/Warning: (recently discovered and may have been fixed)
> >
> > arch_topology.c:(.text+0xbac): multiple definition of `store_cpu_topology'; arch/arm/kernel/topology.o:topology.c:(.text+0x0): first defined here
> >
> > Error/Warning ids grouped by kconfigs:
> >
> > gcc_recent_errors
> > `-- arm-defconfig
> > `-- multiple-definition-of-store_cpu_topology-arch-arm-kernel-topology.o:topology.c:(.text):first-defined-here
> >
> > elapsed time: 721m
>
> Looking at the arm32 implementation - it appears to be mostly the sort of MPIDR
> stuff that was removed from the arm64 implementation in 3102bc0e6ac7 ("arm64:
> topology: Stop using MPIDR for topology information"). Could arm32 benefit from
> the same shared implemenation too, or is usage of MPIDR only invalid for arm64?
Don't look at me... this code was contributed by Linaro, presumably for
systems they have. I've never had anything that would require this so
the code never interested me, so I never took much notice of it.
Sorry, I can't be of more help.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Sat, Jul 09, 2022 at 12:58:57PM +0000, [email protected] wrote:
> Looking at the arm32 implementation - it appears to be mostly the sort of MPIDR
> stuff that was removed from the arm64 implementation in 3102bc0e6ac7 ("arm64:
> topology: Stop using MPIDR for topology information"). Could arm32 benefit from
> the same shared implemenation too, or is usage of MPIDR only invalid for arm64?
I don't recall all the details but IIRC there are parts if arch_topology
that are ARM64/RISC-V only. ARM32 doesn't use it as it may break old
platforms. Some of the functions that still arm32 specific are retained
in arch/arm
> The other difference is a call to update_cpu_capacity() in the arm32
> implementation. Could that be moved to smp_store_cpu_info() which is the only
> callsite of store_cpu_topology()?
>
No please, leave arm32 as is. It was done for a reason like that and it
help to not break some of the old 32-by platforms.
> Either way, will respin a v3 that doesn't break the arm32 build when
> CONFIG_GENERIC_ARCH_TOPOLOGY is enabled :)
>
Thanks.
--
Regards,
Sudeep
On 11/07/2022 11:02, Sudeep Holla wrote:
> On Sat, Jul 09, 2022 at 12:58:57PM +0000, [email protected] wrote:
>> Looking at the arm32 implementation - it appears to be mostly the sort of MPIDR
>> stuff that was removed from the arm64 implementation in 3102bc0e6ac7 ("arm64:
>> topology: Stop using MPIDR for topology information"). Could arm32 benefit from
>> the same shared implemenation too, or is usage of MPIDR only invalid for arm64?
>
> I don't recall all the details but IIRC there are parts if arch_topology
> that are ARM64/RISC-V only. ARM32 doesn't use it as it may break old
> platforms. Some of the functions that still arm32 specific are retained
> in arch/arm
>
>> The other difference is a call to update_cpu_capacity() in the arm32
>> implementation. Could that be moved to smp_store_cpu_info() which is the only
>> callsite of store_cpu_topology()?
>>
>
> No please, leave arm32 as is. It was done for a reason like that and it
> help to not break some of the old 32-by platforms.
Thought that might be the case, I won't (and didn't) touch arm32!
Thanks,
Conor.