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

[Conor Dooley]

Sudeep, see below - I got mixed up & what arm64 does now makes sense to me!
Your opinion on the patch, motivation aside, would be nice though.

Hey Leyfoon,


On 4 January 2023 05:35:41 GMT, Leyfoon Tan <leyfoon.tan at starfivetech.com> wrote:
>
>
>> -----Original Message-----
>> From: Conor Dooley <conor at kernel.org>
>> Sent: Wednesday, 4 January, 2023 1:08 AM

---8<---
>> To: Leyfoon Tan <leyfoon.tan at starfivetech.com>; Sudeep Holla
>> <sudeep.holla at arm.com>
>> Cc: Andrew Jones <ajones at ventanamicro.com>; Palmer Dabbelt
>> <palmer at dabbelt.com>; Paul Walmsley <paul.walmsley at sifive.com>; Albert
>> Ou <aou at eecs.berkeley.edu>; linux-riscv at lists.infradead.org; linux-
>> kernel at vger.kernel.org; Ley Foon Tan <lftan.linux at gmail.com>
>> Subject: Re: [PATCH] riscv: Move call to init_cpu_topology() to later
>> initialization stage
---8<---

btw, this above is pretty much useless can you can drop it from replies.

>> 
>> 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 at ventanamicro.com>
>> > > 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.

The property is only valid on arm, so how does arm64 deal with such asymmetric topologies without it?
Why should we "fix" something that may never be a valid dts?

>
>> 
>> > > > 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 at starfivetech.com>
>> > >
>> > > 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?

I got myself mixed up between places I fiddled with storing the topology, so you can ignore that question Sudeep.
Clearly it's the one in smp_callin() that gets called for each CPU.
Woops.

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

...but is it still worth moving the function call later to head off any allocation failures if core topology code changes?

>> > > > +
>> > > >  	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 at ventanamicro.com>
>
>Regards
>Ley Foon
>