Messages in this thread Patch in this message | | | Date | Wed, 04 Jan 2023 09:49:48 +0000 | From | Conor Dooley <> | Subject | RE: [PATCH] riscv: Move call to init cpu topology() to later initialization stage |
| |
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@starfivetech.com> wrote: > > >> -----Original Message----- >> From: Conor Dooley <conor@kernel.org> >> Sent: Wednesday, 4 January, 2023 1:08 AM
---8<--- >> To: Leyfoon Tan <leyfoon.tan@starfivetech.com>; Sudeep Holla >> <sudeep.holla@arm.com> >> Cc: Andrew Jones <ajones@ventanamicro.com>; Palmer Dabbelt >> <palmer@dabbelt.com>; Paul Walmsley <paul.walmsley@sifive.com>; Albert >> Ou <aou@eecs.berkeley.edu>; linux-riscv@lists.infradead.org; linux- >> kernel@vger.kernel.org; Ley Foon Tan <lftan.linux@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@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@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@ventanamicro.com> > >Regards >Ley Foon >
| |