riscv: arch-topology: fix default topology reporting

Commit Message

Conor Dooley July 6, 2022, 6:45 p.m. UTC

From: Conor Dooley <[e-mail]>

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: [e-mail]
Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
Reported-by: Brice Goglin <[e-mail]>
Link: https://github.com/open-mpi/hwloc/issues/536
Signed-off-by: Conor Dooley <[e-mail]>
---

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


base-commit: b6f1f2fa2bddd69ff46a190b8120bd440fd50563

Comments

Atish Patra July 6, 2022, 9:38 p.m. UTC | #1

On Wed, Jul 6, 2022 at 11:46 AM Conor Dooley <[e-mail]> wrote:
>
> From: Conor Dooley <[e-mail]>
>
> 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: [e-mail]
> Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
> Reported-by: Brice Goglin <[e-mail]>
> Link: https://github.com/open-mpi/hwloc/issues/536
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
>
> 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 ?

> base-commit: b6f1f2fa2bddd69ff46a190b8120bd440fd50563
> --
> 2.37.0
>

kernel test robot July 6, 2022, 11:38 p.m. UTC | #2

Hi Conor,

I love your patch! Yet something to improve:

[auto build test ERROR on b6f1f2fa2bddd69ff46a190b8120bd440fd50563]

url:    https://github.com/intel-lab-lkp/linux/commits/Conor-Dooley/riscv-arch-topology-fix-default-topology-reporting/20220707-024856
base:   b6f1f2fa2bddd69ff46a190b8120bd440fd50563
config: riscv-randconfig-r042-20220706 (https://download.01.org/0day-ci/archive/20220707/[e-mail]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project f553287b588916de09c66e3e32bf75e5060f967f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/824f4c3cb56ada865e6e3b14457c0582fa255cbf
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Conor-Dooley/riscv-arch-topology-fix-default-topology-reporting/20220707-024856
        git checkout 824f4c3cb56ada865e6e3b14457c0582fa255cbf
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash arch/riscv/kernel/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>):

>> arch/riscv/kernel/topology.c:17:37: error: use of undeclared identifier 'cpu_topology'
           struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
                                              ^
>> arch/riscv/kernel/topology.c:31:2: error: call to undeclared function 'update_siblings_masks'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           update_siblings_masks(cpuid);
           ^
   2 errors generated.


vim +/cpu_topology +17 arch/riscv/kernel/topology.c

    14	
    15	void store_cpu_topology(unsigned int cpuid)
    16	{
  > 17		struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
    18	
    19		if (cpuid_topo->package_id != -1)
    20			goto topology_populated;
    21	
    22		cpuid_topo->thread_id = -1;
    23		cpuid_topo->core_id = cpuid;
    24		cpuid_topo->package_id = cpu_to_node(cpuid);
    25	
    26		pr_debug("CPU%u: package %d core %d thread %d\n",
    27			 cpuid, cpuid_topo->package_id, cpuid_topo->core_id,
    28			 cpuid_topo->thread_id);
    29	
    30	topology_populated:
  > 31		update_siblings_masks(cpuid);

Sudeep Holla July 7, 2022, 9:47 a.m. UTC | #3

On Wed, Jul 06, 2022 at 02:38:01PM -0700, Atish Patra wrote:
> On Wed, Jul 6, 2022 at 11:46 AM Conor Dooley <[e-mail]> wrote:
> >
> > From: Conor Dooley <[e-mail]>
> >
> > 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: [e-mail]
> > Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
> > Reported-by: Brice Goglin <[e-mail]>
> > Link: https://github.com/open-mpi/hwloc/issues/536
> > Signed-off-by: Conor Dooley <[e-mail]>
> > ---
> >
> > 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.

> > base-commit: b6f1f2fa2bddd69ff46a190b8120bd440fd50563
> > --
> > 2.37.0
> >
> 
> 
> -- 
> Regards,
> Atish

Sudeep Holla July 7, 2022, 9:50 a.m. UTC | #4

On Wed, Jul 06, 2022 at 07:45:59PM +0100, Conor Dooley wrote:
> From: Conor Dooley <[e-mail]>
> 
> 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: [e-mail]
> Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
> Reported-by: Brice Goglin <[e-mail]>
> Link: https://github.com/open-mpi/hwloc/issues/536
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
> 
> 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/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"
>  

[...]

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

If the above store_cpu_topology calls update_siblings_masks if required,
probably you can drop this explicit call here.

Conor Dooley July 7, 2022, 10:12 a.m. UTC | #5

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 <[e-mail]> wrote:
>>>
>>> From: Conor Dooley <[e-mail]>
>>>
>>> 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: [e-mail]
>>> Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
>>> Reported-by: Brice Goglin <[e-mail]>
>>> Link: https://github.com/open-mpi/hwloc/issues/536
>>> Signed-off-by: Conor Dooley <[e-mail]>
>>> ---
>>>
>>> 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.

Conor Dooley July 7, 2022, 10:15 a.m. UTC | #6

On 07/07/2022 10:50, Sudeep Holla wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Jul 06, 2022 at 07:45:59PM +0100, Conor Dooley wrote:
>> From: Conor Dooley <[e-mail]>
>>
>> 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: [e-mail]
>> Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
>> Reported-by: Brice Goglin <[e-mail]>
>> Link: https://github.com/open-mpi/hwloc/issues/536
>> Signed-off-by: Conor Dooley <[e-mail]>
>> ---
>>
>> 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/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"
>>
> 
> [...]
> 
>>   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);
> 
> If the above store_cpu_topology calls update_siblings_masks if required,
> probably you can drop this explicit call here.

Yeah, store_cpu_topology() does call update_siblings_masks().
I'll respin tonight with this (and the lkp error) fixed.

Thanks Sudeep,
Conor.

Patch

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