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 arm{,64} , 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.
V3 moves store_cpu_topology()'s definition down inside the arch check
alongside the init function so that boot on 32bit arm is not broken.
V4 has moved the RISC-V boot hart's call to store_cpu_topology() later
into the boot process it is now right before SMP is brought up (or not
in the case of !SMP). This prevents calling detect_cache_attributes()
while we cannot allocate memory.
V4 is also rebased on next-20220715 to get Sudeep's most recent
arch_topology patchset.
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 | 3 ++-
drivers/base/arch_topology.c | 19 +++++++++++++++++
4 files changed, 22 insertions(+), 42 deletions(-)
base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
--
2.37.1
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]>
Reviewed-by: Sudeep Holla <[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 0424b59b695e..0e2c6b30dd69 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -841,4 +841,23 @@ void __init init_cpu_topology(void)
return;
}
}
+
+void 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);
+}
#endif
--
2.37.1
On 15/07/2022 18:51, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> 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 arm{,64} , so hopefully it is not broken when
> used. Testing on both arm64 & !SMP RISC-V would really be appreciated!
FWIW, I pushed it to a branch for the sake of LKP testing and all
archs + randconfigs built fine.
Thanks,
Conor.
>
> 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.
>
> V3 moves store_cpu_topology()'s definition down inside the arch check
> alongside the init function so that boot on 32bit arm is not broken.
>
> V4 has moved the RISC-V boot hart's call to store_cpu_topology() later
> into the boot process it is now right before SMP is brought up (or not
> in the case of !SMP). This prevents calling detect_cache_attributes()
> while we cannot allocate memory.
>
> V4 is also rebased on next-20220715 to get Sudeep's most recent
> arch_topology patchset.
>
> 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 | 3 ++-
> drivers/base/arch_topology.c | 19 +++++++++++++++++
> 4 files changed, 22 insertions(+), 42 deletions(-)
>
>
> base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
> --
> 2.37.1
>
On Fri, Jul 15, 2022 at 06:51:55PM +0100, 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]
I'd quantify how far back you want this to go. IIUC based on the Fixes
tag in the other patch, it should stop at 5.4. If you send a pull
request instead and have a fixed commit id, you could add it as a
prerequisite on the following patch without a cc stable here.
Either way:
Acked-by: Catalin Marinas <[email protected]>
On 19/07/2022 12:41, Catalin Marinas wrote:
> On Fri, Jul 15, 2022 at 06:51:55PM +0100, 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]
>
> I'd quantify how far back you want this to go. IIUC based on the Fixes
> tag in the other patch, it should stop at 5.4. If you send a pull
> request instead and have a fixed commit id, you could add it as a
> prerequisite on the following patch without a cc stable here.
I guess a PR might be the easiest way for it anyway, so that both
yourself and Palmer could merge it?
>
> Either way:
>
> Acked-by: Catalin Marinas <[email protected]>
Thanks.
On Tue, Jul 19, 2022 at 11:51:04AM +0000, [email protected] wrote:
> On 19/07/2022 12:41, Catalin Marinas wrote:
> > On Fri, Jul 15, 2022 at 06:51:55PM +0100, 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]
> >
> > I'd quantify how far back you want this to go. IIUC based on the Fixes
> > tag in the other patch, it should stop at 5.4. If you send a pull
> > request instead and have a fixed commit id, you could add it as a
> > prerequisite on the following patch without a cc stable here.
>
> I guess a PR might be the easiest way for it anyway, so that both
> yourself and Palmer could merge it?
I guess so, a stable branch would do. Note that Will is handling the
upcoming merging window.
--
Catalin
On 15/07/2022 18:51, Conor Dooley wrote:
> 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
Hey,
Not got any feedback on the smpboot changes from the RISC-V side.
I tested it on polarfire, the d1 (with both SMP & !SMP set iirc)
& on the u540. It all looked good to me.
I'd like to have this fixed for v5.20, but there isn't too much
time left before the mw. Not too sure about the cross-tree changes,
does it need an immutable branch or could it go through driver-core?
Catalin suggested removing the CC stable from patch 1/2 & adding it
as a dependency for the 2/2 patch - but obviously that's up to the
committer to sort out.
I guess since it is a fix, it could also go into rc1<
Thanks,
Conor.
>
> 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 arm{,64} , 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.
>
> V3 moves store_cpu_topology()'s definition down inside the arch check
> alongside the init function so that boot on 32bit arm is not broken.
>
> V4 has moved the RISC-V boot hart's call to store_cpu_topology() later
> into the boot process it is now right before SMP is brought up (or not
> in the case of !SMP). This prevents calling detect_cache_attributes()
> while we cannot allocate memory.
>
> V4 is also rebased on next-20220715 to get Sudeep's most recent
> arch_topology patchset.
>
> 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 | 3 ++-
> drivers/base/arch_topology.c | 19 +++++++++++++++++
> 4 files changed, 22 insertions(+), 42 deletions(-)
>
>
> base-commit: 6014cfa5bf32cf8c5c58b3cfd5ee0e1542c8a825
On Sat, Jul 23, 2022 at 11:22:01AM +0000, [email protected] wrote:
> On 15/07/2022 18:51, Conor Dooley wrote:
> > 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
>
> Hey,
>
> Not got any feedback on the smpboot changes from the RISC-V side.
> I tested it on polarfire, the d1 (with both SMP & !SMP set iirc)
> & on the u540. It all looked good to me.
>
> I'd like to have this fixed for v5.20, but there isn't too much
> time left before the mw. Not too sure about the cross-tree changes,
> does it need an immutable branch or could it go through driver-core?
> Catalin suggested removing the CC stable from patch 1/2 & adding it
> as a dependency for the 2/2 patch - but obviously that's up to the
> committer to sort out.
I'm finalising the arm64 queue today, so I don't really want to pull in
additional changes beyond critical fixes at this point, I'm afraid. I was
half-expecting a pull request from the riscv side last week but I didn't
see anything.
FWIW, if there's still no movement by -rc1, then I'm happy to queue all
of this on its own branch in the arm64 tree for 5.21.
Let me know.
Will
On 25/07/2022 10:13, Will Deacon wrote:
> On Sat, Jul 23, 2022 at 11:22:01AM +0000, [email protected] wrote:
>> On 15/07/2022 18:51, Conor Dooley wrote:
>>
>> Hey,
>>
>> Not got any feedback on the smpboot changes from the RISC-V side.
>> I tested it on polarfire, the d1 (with both SMP & !SMP set iirc)
>> & on the u540. It all looked good to me.
>>
>> I'd like to have this fixed for v5.20, but there isn't too much
>> time left before the mw. Not too sure about the cross-tree changes,
>> does it need an immutable branch or could it go through driver-core?
>> Catalin suggested removing the CC stable from patch 1/2 & adding it
>> as a dependency for the 2/2 patch - but obviously that's up to the
>> committer to sort out.
>
> I'm finalising the arm64 queue today, so I don't really want to pull in
> additional changes beyond critical fixes at this point, I'm afraid. I was
> half-expecting a pull request from the riscv side last week but I didn't
> see anything.
Yeah, that's fair. It's late in the game for cross-tree messing.
I know Palmer has been p busy recently.
> FWIW, if there's still no movement by -rc1, then I'm happy to queue all
> of this on its own branch in the arm64 tree for 5.21.
Hopefully someone on the riscv side will have confirmed what I am doing
is sane by then.
>
> Let me know.
I will, thanks!
On Fri, Jul 15, 2022 at 10:53 AM Conor Dooley <[email protected]> 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]>
> Reviewed-by: Sudeep Holla <[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 0424b59b695e..0e2c6b30dd69 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -841,4 +841,23 @@ void __init init_cpu_topology(void)
> return;
> }
> }
> +
> +void 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);
> +}
> #endif
> --
> 2.37.1
>
LGTM.
Reviewed-by: Atish Patra <[email protected]>
--
Regards,
Atish
On Mon, Jul 25, 2022 at 2:20 AM <[email protected]> wrote:
>
> On 25/07/2022 10:13, Will Deacon wrote:
> > On Sat, Jul 23, 2022 at 11:22:01AM +0000, [email protected] wrote:
> >> On 15/07/2022 18:51, Conor Dooley wrote:
> >>
> >> Hey,
> >>
> >> Not got any feedback on the smpboot changes from the RISC-V side.
> >> I tested it on polarfire, the d1 (with both SMP & !SMP set iirc)
> >> & on the u540. It all looked good to me.
> >>
> >> I'd like to have this fixed for v5.20, but there isn't too much
> >> time left before the mw. Not too sure about the cross-tree changes,
> >> does it need an immutable branch or could it go through driver-core?
> >> Catalin suggested removing the CC stable from patch 1/2 & adding it
> >> as a dependency for the 2/2 patch - but obviously that's up to the
> >> committer to sort out.
> >
> > I'm finalising the arm64 queue today, so I don't really want to pull in
> > additional changes beyond critical fixes at this point, I'm afraid. I was
> > half-expecting a pull request from the riscv side last week but I didn't
> > see anything.
>
> Yeah, that's fair. It's late in the game for cross-tree messing.
> I know Palmer has been p busy recently.
>
> > FWIW, if there's still no movement by -rc1, then I'm happy to queue all
> > of this on its own branch in the arm64 tree for 5.21.
>
> Hopefully someone on the riscv side will have confirmed what I am doing
> is sane by then.
>
> >
> > Let me know.
>
> I will, thanks!
Sorry for the delayed response here. I was planning to test the series
last week itself
but got dragged into something else and a qemu bug for NUMA.
Thanks for the fixes. I have tested this on Qemu(removing the topology
node) for the following configurations.
SMP, !SMP, NUMA (2 sockets)
FWIW,
Tested-by: Atish Patra <[email protected]>
--
Regards,
Atish
On 26/07/2022 09:12, Atish Patra wrote:
>
> Sorry for the delayed response here. I was planning to test the series
> last week itself
> but got dragged into something else and a qemu bug for NUMA.
No worries, thanks for testing/reviewing it.
>
> Thanks for the fixes. I have tested this on Qemu(removing the topology
> node) for the following configurations.
> SMP, !SMP, NUMA (2 sockets)
A NUMA test, great! I do need to get qemu set up properly..
>
> FWIW,
> Tested-by: Atish Patra <[email protected]>
Thanks,
Conor.