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

[Gregory CLEMENT <gregory.clement@bootlin.com>]

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 don't know whether you have done reboot tests (for ~1000 times),
> Jiaxun Yang submitted similar patches for LoongArch [1], but during
> reboot tests we encountered problems that I have described in my
> previous reply.
>
> [1] https://lore.kernel.org/loongarch/20240716-loongarch-hotplug-v3-0-af59b3bb35c8@flygoat.com/

I saw that series and I wondered why the last patch was not merged.

I performed around 100 tests so far without encountering any issues; I
plan to automate them further to gather more data.

Gregpory

>
> Huacai
>
>>
>> Huacai
>>
>> > +#endif
>> >         notify_cpu_starting(cpu);
>> >
>> > +#ifndef CONFIG_HOTPLUG_PARALLEL
>> >         /* Notify boot CPU that we're starting & ready to sync counters */
>> >         complete(&cpu_starting);
>> > +#endif
>> >
>> >         synchronise_count_slave(cpu);
>> >
>> > @@ -386,11 +395,13 @@ asmlinkage void start_secondary(void)
>> >
>> >         calculate_cpu_foreign_map();
>> >
>> > +#ifndef CONFIG_HOTPLUG_PARALLEL
>> >         /*
>> >          * Notify boot CPU that we're up & online and it can safely return
>> >          * from __cpu_up
>> >          */
>> >         complete(&cpu_running);
>> > +#endif
>> >
>> >         /*
>> >          * irq will be enabled in ->smp_finish(), enabling it too early
>> > @@ -447,6 +458,12 @@ void __init smp_prepare_boot_cpu(void)
>> >         set_cpu_online(0, true);
>> >  }
>> >
>> > +#ifdef CONFIG_HOTPLUG_PARALLEL
>> > +int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *tidle)
>> > +{
>> > +       return mp_ops->boot_secondary(cpu, tidle);
>> > +}
>> > +#else
>> >  int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>> >  {
>> >         int err;
>> > @@ -466,6 +483,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>> >         wait_for_completion(&cpu_running);
>> >         return 0;
>> >  }
>> > +#endif
>> >
>> >  #ifdef CONFIG_PROFILING
>> >  /* Not really SMP stuff ... */
>> >
>> > ---
>> > base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
>> > change-id: 20250411-parallel-cpu-bringup-78999a9235ea
>> >
>> > Best regards,
>> > --
>> > Grégory CLEMENT, Bootlin
>> > Embedded Linux and Kernel engineering
>> > https://bootlin.com
>> >
>> >

-- 
Grégory CLEMENT, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com