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

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

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.

Gregory

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

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