Re: [PATCH] MIPS: SMP: Implement parallel CPU bring up for EyeQ

[Thomas Bogendoerfer <tsbogend@alpha.franken.de>]

On Fri, May 02, 2025 at 05:32:54PM +0200, Gregory CLEMENT wrote:
> Hello, 
> 
> > Huacai Chen <chenhuacai@kernel.org> writes:
> >
> >> On Wed, Apr 30, 2025 at 3:09 PM Gregory CLEMENT
> >> <gregory.clement@bootlin.com> wrote:
> >>>
> >>> Hello Huacai,
> >>>
> >>> > Hi, Gregory,
> >>> >
> >>> > On Sun, Apr 27, 2025 at 6:13 PM Huacai Chen <chenhuacai@kernel.org> wrote:
> >>> >>
> >>> >> Hi, Gregory and Thomas,
> >>> >>
> >>> >> I'm sorry I'm late, but I have some questions about this patch.
> >>> >>
> >>> >> On Mon, Apr 14, 2025 at 3:12 AM Gregory CLEMENT
> >>> >> <gregory.clement@bootlin.com> wrote:
> >>> >> >
> >>> >> > Added support for starting CPUs in parallel on EyeQ to speed up boot time.
> >>> >> >
> >>> >> > On EyeQ5, booting 8 CPUs is now ~90ms faster.
> >>> >> > On EyeQ6, booting 32 CPUs is now ~650ms faster.
> >>> >> >
> >>> >> > Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> >>> >> > ---
> >>> >> > Hello,
> >>> >> >
> >>> >> > This patch allows CPUs to start in parallel. It has been tested on
> >>> >> > EyeQ5 and EyeQ6, which are both MIPS64 and use the I6500 design. These
> >>> >> > systems use CPS to support SMP.
> >>> >> >
> >>> >> > As noted in the commit log, on EyeQ6, booting 32 CPUs is now ~650ms
> >>> >> > faster.
> >>> >> >
> >>> >> > Currently, this support is only for EyeQ SoC. However, it should also
> >>> >> > work for other CPUs using CPS. I am less sure about MT ASE support,
> >>> >> > but this patch can be a good starting point. If anyone wants to add
> >>> >> > support for other systems, I can share some ideas, especially for the
> >>> >> > MIPS_GENERIC setup that needs to handle both types of SMP setups.
> >>> >> >
> >>> [...]
> >>> >> >   * A logical cpu mask containing only one VPE per core to
> >>> >> > @@ -74,6 +76,8 @@ static cpumask_t cpu_core_setup_map;
> >>> >> >
> >>> >> >  cpumask_t cpu_coherent_mask;
> >>> >> >
> >>> >> > +struct cpumask __cpu_primary_thread_mask __read_mostly;
> >>> >> > +
> >>> >> >  unsigned int smp_max_threads __initdata = UINT_MAX;
> >>> >> >
> >>> >> >  static int __init early_nosmt(char *s)
> >>> >> > @@ -374,10 +378,15 @@ asmlinkage void start_secondary(void)
> >>> >> >         set_cpu_core_map(cpu);
> >>> >> >
> >>> >> >         cpumask_set_cpu(cpu, &cpu_coherent_mask);
> >>> >> > +#ifdef CONFIG_HOTPLUG_PARALLEL
> >>> >> > +       cpuhp_ap_sync_alive();
> >>> >> This is a "synchronization point" due to the description from commit
> >>> >> 9244724fbf8ab394a7210e8e93bf037abc, which means things are parallel
> >>> >> before this point and serialized after this point.
> >>> >>
> >>> >> But unfortunately, set_cpu_sibling_map() and set_cpu_core_map() cannot
> >>> >> be executed in parallel. Maybe you haven't observed problems, but in
> >>> >> theory it's not correct.
> >>>
> >>> I am working on it. To address your remark, I have a few options that I
> >>> evaluate.
> >> I suggest to revert this patch temporary in mips-next.
> >
> >
> > As I previously mentioned, I haven't observed any issues until now. What
> > I'm evaluating is whether there is a real problem with this
> > implementation. Let's examine whether we need a new patch or if this one
> > is sufficient.
> >
> > I will have the resutls at the end of the week.
> 
> After hundreds of reboots on the EyeQ5, I did not encounter any failures
> during boot. However, while executing the set_cpu_core_map() and
> set_cpu_sibling_map() functions in parallel, modifications to shared
> resources could result in issues. To address this, I proposed the
> following fix:
> 
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index 1726744f2b2ec..5f30611f45a1c 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -374,13 +377,13 @@ asmlinkage void start_secondary(void)
>         calibrate_delay();
>         cpu_data[cpu].udelay_val = loops_per_jiffy;
>  
> +#ifdef CONFIG_HOTPLUG_PARALLEL
> +       cpuhp_ap_sync_alive();
> +#endif
>         set_cpu_sibling_map(cpu);
>         set_cpu_core_map(cpu);
>  
>         cpumask_set_cpu(cpu, &cpu_coherent_mask);
> -#ifdef CONFIG_HOTPLUG_PARALLEL
> -       cpuhp_ap_sync_alive();
> -#endif
>         notify_cpu_starting(cpu);
>  
>  #ifndef CONFIG_HOTPLUG_PARALLEL
> 
> It moved these two functions back in the serialized part of the boot. I
> was concerned about potential slowdowns during the boot process, but I
> didn't notice any issues during my test on EyeQ5. Therefore, we can make
> this change.
> 
> 
> Thomas,
> 
> how would you like to proceed? Do you want to squash this patch
> into the current commit, or do you prefere I create a separate patch for
> it, or a new version of the patch including this change?

please send a seperate patch with just the fix

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]