Re: [PATCH] riscv: arch-topology: fix default topology reporting

[Conor.Dooley]

On 07/07/2022 10:47, Sudeep Holla wrote:
> On Wed, Jul 06, 2022 at 02:38:01PM -0700, Atish Patra wrote:
>> On Wed, Jul 6, 2022 at 11:46 AM Conor Dooley <mail@xxxxxxxxxxx> wrote:
>>>
>>> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>>
>>> RISC-V has no sane defaults to fall back on where there is no cpu-map
>>> in the devicetree.
>>> Without sane defaults, the package, core and thread IDs are all set to
>>> -1. This causes user-visible inaccuracies for tools like hwloc/lstopo
>>> which rely on the sysfs cpu topology files to detect a system's
>>> topology.
>>>
>>> Add sane defaults in ~the exact same way as ARM64.
>>>
>>> CC: stable@xxxxxxxxxxxxxxx
>>> Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
>>> Reported-by: Brice Goglin <Brice.Goglin@xxxxxxxx>
>>> Link: https://github.com/open-mpi/hwloc/issues/536
>>> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>> ---
>>>
>>> Sudeep suggested that this be backported rather than the changes to
>>> the devicetrees adding cpu-map since that property is optional.
>>> That patchset is still valid in it's own right.
>>>
>>>   arch/riscv/include/asm/topology.h | 13 +++++++++++++
>>>   arch/riscv/kernel/Makefile        |  1 +
>>>   arch/riscv/kernel/smpboot.c       |  4 ++++
>>>   arch/riscv/kernel/topology.c      | 32 +++++++++++++++++++++++++++++++
>>>   4 files changed, 50 insertions(+)
>>>   create mode 100644 arch/riscv/include/asm/topology.h
>>>   create mode 100644 arch/riscv/kernel/topology.c
>>>
>>> diff --git a/arch/riscv/include/asm/topology.h b/arch/riscv/include/asm/topology.h
>>> new file mode 100644
>>> index 000000000000..36bc6ecda898
>>> --- /dev/null
>>> +++ b/arch/riscv/include/asm/topology.h
>>> @@ -0,0 +1,13 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (c) 2022 Microchip Technology Inc. and its subsidiaries
>>> + */
>>> +
>>> +#ifndef _ASM_RISCV_TOPOLOGY_H
>>> +#define _ASM_RISCV_TOPOLOGY_H
>>> +
>>> +#include <asm-generic/topology.h>
>>> +
>>> +void store_cpu_topology(unsigned int cpuid);
>>> +
>>> +#endif /* _ASM_RISCV_TOPOLOGY_H */
>>> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
>>> index c71d6591d539..9518882ba6f9 100644
>>> --- a/arch/riscv/kernel/Makefile
>>> +++ b/arch/riscv/kernel/Makefile
>>> @@ -50,6 +50,7 @@ obj-y += riscv_ksyms.o
>>>   obj-y  += stacktrace.o
>>>   obj-y  += cacheinfo.o
>>>   obj-y  += patch.o
>>> +obj-y  += topology.o
>>>   obj-y  += probes/
>>>   obj-$(CONFIG_MMU) += vdso.o vdso/
>>>
>>> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
>>> index f1e4948a4b52..d551c7f452d4 100644
>>> --- a/arch/riscv/kernel/smpboot.c
>>> +++ b/arch/riscv/kernel/smpboot.c
>>> @@ -32,6 +32,7 @@
>>>   #include <asm/sections.h>
>>>   #include <asm/sbi.h>
>>>   #include <asm/smp.h>
>>> +#include <asm/topology.h>
>>>
>>>   #include "head.h"
>>>
>>> @@ -40,6 +41,8 @@ static DECLARE_COMPLETION(cpu_running);
>>>   void __init smp_prepare_boot_cpu(void)
>>>   {
>>>          init_cpu_topology();
>>> +
>>> +       store_cpu_topology(smp_processor_id());
>>>   }
>>>
>>>   void __init smp_prepare_cpus(unsigned int max_cpus)
>>> @@ -161,6 +164,7 @@ asmlinkage __visible void smp_callin(void)
>>>          mmgrab(mm);
>>>          current->active_mm = mm;
>>>
>>> +       store_cpu_topology(curr_cpuid);
>>>          notify_cpu_starting(curr_cpuid);
>>>          numa_add_cpu(curr_cpuid);
>>>          update_siblings_masks(curr_cpuid);
>>> diff --git a/arch/riscv/kernel/topology.c b/arch/riscv/kernel/topology.c
>>> new file mode 100644
>>> index 000000000000..db72862bd5b5
>>> --- /dev/null
>>> +++ b/arch/riscv/kernel/topology.c
>>> @@ -0,0 +1,32 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2022 Microchip Technology Inc. and its subsidiaries
>>> + *
>>> + * Based on the arm64 version, which was in turn based on arm32, which was
>>> + * ultimately based on sh's.
>>> + * The arm64 version was listed as:
>>> + * Copyright (C) 2011,2013,2014 Linaro Limited.
>>> + */
>>> +
>>> +#include <linux/arch_topology.h>
>>> +#include <linux/topology.h>
>>> +#include <asm/topology.h>
>>> +
>>> +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);
>>> +}
>>>
>>
>> This function is pretty much the same as the arm64 one except the
>> UP/mpidr check.
>> Can we move this to the common code as well ?
>>
> 
> While I completely agree with the idea, not sure if that makes backports
> (if required) any difficult. If so, I would rather keep this way for a
> release and then move both to the common place in arch_topology.

Yeah, that seems like a good idea. I'll let this patch just touch
RISC-V for the sake of backporting & create a second patch to move
to a common implementation.

Since I've not modified any real arch code before, I'd rather make
that a separate patch/series too for the sake of getting this
patch applied as a v5.19-rc(late) fix.