RE: [PATCH] riscv: Move call to init_cpu_topology() to later initialization stage

[Leyfoon Tan]

> -----Original Message-----
> From: Conor Dooley <conor@xxxxxxxxxx>
> Sent: Wednesday, 4 January, 2023 1:08 AM
> To: Leyfoon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>; Sudeep Holla
> <sudeep.holla@xxxxxxx>
> Cc: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>; Palmer Dabbelt
> <palmer@xxxxxxxxxxx>; Paul Walmsley <paul.walmsley@xxxxxxxxxx>; Albert
> Ou <aou@xxxxxxxxxxxxxxxxx>; linux-riscv@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Ley Foon Tan <lftan.linux@xxxxxxxxx>
> Subject: Re: [PATCH] riscv: Move call to init_cpu_topology() to later
> initialization stage
> 
> Hello!
> 
> Couple comments for you.
> 
> +CC Sudeep: I've got a question for you below.
> 
> On Tue, Jan 03, 2023 at 07:53:38AM +0000, Leyfoon Tan wrote:
> > > From: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> > > Subject: Re: [PATCH] riscv: Move call to init_cpu_topology() to
> > > later initialization stage On Tue, Jan 03, 2023 at 11:53:16AM +0800,
> > > Ley Foon Tan wrote:
> > > > topology_parse_cpu_capacity() is failed to allocate memory with
> > > > kcalloc() after read "capacity-dmips-mhz" DT parameter in CPU DT
> 
> Uhh, so where did this "capacity-dmips-mhz" property actually come from?
> I had a quick check of qemu with grep & I don't see anything there that
> would add this property.
> This property should not be valid on anything other than arm AFAICT.

This DT parameter is not in default Qemu. I've added it for testing (see test steps in below). 
This is preparation to support asymmetric CPU topology for RISC-V.

> 
> > > > nodes. This
> > > > topology_parse_cpu_capacity() is called from init_cpu_topology(),
> > > > move call to init_cpu_topology() to later initialization  stage
> > > > (after memory allocation is available).
> > > >
> > > > Note, this refers to ARM64 implementation, call
> > > > init_cpu_topology() in smp_prepare_cpus().
> > > >
> > > > Tested on Qemu platform.
> > >
> > > Hi Ley,
> > >
> > > Can you provide the topologies (command lines) tested?
> > 2 clusters with 2 CPU cores each.
> 
> What's the actual commandline for this? I'm not the best with QEMU, so it'd
> really be appreciated, given the above.
I used the Qemu to dump the DTS for 'virt' machine from Qemu, then add the "capacity-dmips-mhz" DT parameter and modify the CPU topology for clusters.

1. Dump the dtb for 'virt' machine:
qemu-system-riscv64 -cpu rv64 -smp 4 -m 2048M -M virt,dumpdtb=qemu_virt.dtb

2. Convert dtb to dts
dtc -I dtb -O dts qemu_virt.dtb > qemu_virt.dts

3. Modifying qemu_virt.dts

4. Compile dts to dtb
dtc -I dts -O dtb qemu_virt.dts > qemu_virt.dtb

5. Pass in "-dtb qemu_virt.dtb" as one of Qemu's argument.

> 
> > > > Signed-off-by: Ley Foon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>
> > >
> > > Fixes tag?
> > Okay, will send out next revision with Fixes tag.
> 
> Please don't just send versions to add tags, Palmer can pick them up if/when
> he applies the patch.
Okay. Will let Palmer add tag below if he applies the patch.

Fixes: 03f11f03dbfe ("RISC-V: Parse cpu topology during boot. ")


> 
> > > >  arch/riscv/kernel/smpboot.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/kernel/smpboot.c
> > > > b/arch/riscv/kernel/smpboot.c index 3373df413c88..ddb2afba6d25
> > > > 100644
> > > > --- a/arch/riscv/kernel/smpboot.c
> > > > +++ b/arch/riscv/kernel/smpboot.c
> > > > @@ -39,7 +39,6 @@ static DECLARE_COMPLETION(cpu_running);
> > > >
> > > >  void __init smp_prepare_boot_cpu(void)  {
> > > > -	init_cpu_topology();
> > > >  }
> > > >
> > > >  void __init smp_prepare_cpus(unsigned int max_cpus) @@ -48,6
> > > > +47,8
> > > @@
> > > > void __init smp_prepare_cpus(unsigned int max_cpus)
> > > >  	int ret;
> > > >  	unsigned int curr_cpuid;
> > > >
> > > > +	init_cpu_topology();
> 
> I know arm64 does this, but there is any real reason for us to do so?
> @Sudeep, do you know why arm64 calls that each time?
> Or if it is worth "saving" that call on riscv, since arm64 is clearly happily calling
> it for many years & calling it later would likely head off a good few allocation
> issues (like the one we saw with the topology reworking a few months ago).
> 
> Thanks,
> Conor.
> 
> > > > +
> > > >  	curr_cpuid = smp_processor_id();
> > > >  	store_cpu_topology(curr_cpuid);
> > > >  	numa_store_cpu_info(curr_cpuid);
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > Otherwise,
> > >
> > > Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>

Regards
Ley Foon