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

From: Sudeep Holla <sudeep.holla@arm.com>
To: Conor Dooley <conor@kernel.org>
Cc: Leyfoon Tan <leyfoon.tan@starfivetech.com>,
	Andrew Jones <ajones@ventanamicro.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	"linux-kernel@vger.kernel.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
Date: Wed, 4 Jan 2023 10:41:16 +0000	[thread overview]
Message-ID: <20230104104116.ob666fm6agkoildy@bogus> (raw)
In-Reply-To: <Y7Rg28suWh1RUbkU@spud>

On Tue, Jan 03, 2023 at 05:07:39PM +0000, Conor Dooley wrote:
> 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.

From the DT properties.

> This property should not be valid on anything other than arm AFAICT.
>

Not sure if we restrict that on arm/arm64 only, but yes it is mostly used
on asymmetric CPU systems.

[...]

> > > > 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?

Not sure what you are referring as called each time. IIUC smp_prepare_cpus()
must be called only once on the primary during the boot to prepare for
the secondary boot. The difference is by this stage we know the max possible
CPU and supply the same to the call.

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

Yes the failure seems like similar issue we saw with cacheinfo and early
allocation on RISC-V. However I can't recall why we have done this way
on arm64 and not in smp_prepare_boot_cpu() like in RISC-V.

Not sure if that was any helpful.

-- 
Regards,
Sudeep

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv