MIPS: SMP: Move the AP sync point before the non-parallel aware functions

[PATCH] MIPS: SMP: Move the AP sync point before the non-parallel aware functions

Posted by Gregory CLEMENT 9 months, 1 week ago

When CONFIG_HOTPLUG_PARALLEL is enabled, the code executing before
cpuhp_ap_sync_alive() is executed in parallel, while after it is
serialized. The functions set_cpu_sibling_map() and set_cpu_core_map()
were not designed to be executed in parallel, so by moving the
cpuhp_ap_sync_alive() before cpuhp_ap_sync_alive(), we then ensure
they will be called serialized.

The measurement done on EyeQ5 did not show any relevant boot time
increase after applying this patch.

Reported-by: Huacai Chen <chenhuacai@kernel.org>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
Hello,

As discussed last week [1], this is the patch that fixes the potential
issue with the functions set_cpu_sibling_map() and set_cpu_core_map().

Gregory

[1]: https://lore.kernel.org/all/aBVBHFGH2kICjnT3@alpha.franken.de/
---
 arch/mips/kernel/smp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index 1726744f2b2ec10a44420a7b9b9cd04f06c4d2f6..7901b59d8f60eddefc020cf2a137716af963f09e 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -374,13 +374,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

---
base-commit: 3b3704261e851e25983860e4c352f1f73786f4ab
change-id: 20250505-hotplug-paralell-fix-25224cd229c6

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

Re: [PATCH] MIPS: SMP: Move the AP sync point before the non-parallel aware functions

Posted by Thomas Bogendoerfer 8 months, 3 weeks ago

On Mon, May 05, 2025 at 02:57:58PM +0200, Gregory CLEMENT wrote:
> When CONFIG_HOTPLUG_PARALLEL is enabled, the code executing before
> cpuhp_ap_sync_alive() is executed in parallel, while after it is
> serialized. The functions set_cpu_sibling_map() and set_cpu_core_map()
> were not designed to be executed in parallel, so by moving the
> cpuhp_ap_sync_alive() before cpuhp_ap_sync_alive(), we then ensure
> they will be called serialized.
> 
> The measurement done on EyeQ5 did not show any relevant boot time
> increase after applying this patch.
> 
> Reported-by: Huacai Chen <chenhuacai@kernel.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
> Hello,
> 
> As discussed last week [1], this is the patch that fixes the potential
> issue with the functions set_cpu_sibling_map() and set_cpu_core_map().
> 
> Gregory

applied to mips-next with a Fixes tag added

Thomas.

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

Re: [PATCH] MIPS: SMP: Move the AP sync point before the non-parallel aware functions

Posted by Huacai Chen 9 months, 1 week ago

Hi, Gregory,

On Mon, May 5, 2025 at 8:58 PM Gregory CLEMENT
<gregory.clement@bootlin.com> wrote:
>
> When CONFIG_HOTPLUG_PARALLEL is enabled, the code executing before
> cpuhp_ap_sync_alive() is executed in parallel, while after it is
> serialized. The functions set_cpu_sibling_map() and set_cpu_core_map()
> were not designed to be executed in parallel, so by moving the
> cpuhp_ap_sync_alive() before cpuhp_ap_sync_alive(), we then ensure
> they will be called serialized.
set_cpu_sibling_map() and set_cpu_core_map() are the most obvious
functions that need serialization, but you'd better check other places
to make sure there are no similar functions executed in parallel.

Huacai

>
> The measurement done on EyeQ5 did not show any relevant boot time
> increase after applying this patch.
>
> Reported-by: Huacai Chen <chenhuacai@kernel.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
> Hello,
>
> As discussed last week [1], this is the patch that fixes the potential
> issue with the functions set_cpu_sibling_map() and set_cpu_core_map().
>
> Gregory
>
> [1]: https://lore.kernel.org/all/aBVBHFGH2kICjnT3@alpha.franken.de/
> ---
>  arch/mips/kernel/smp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index 1726744f2b2ec10a44420a7b9b9cd04f06c4d2f6..7901b59d8f60eddefc020cf2a137716af963f09e 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -374,13 +374,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
>
> ---
> base-commit: 3b3704261e851e25983860e4c352f1f73786f4ab
> change-id: 20250505-hotplug-paralell-fix-25224cd229c6
>
> Best regards,
> --
> Grégory CLEMENT, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>

Re: [PATCH] MIPS: SMP: Move the AP sync point before the non-parallel aware functions

Posted by Gregory CLEMENT 8 months, 3 weeks ago

Hello Huacai,

> Hi, Gregory,
>
> On Mon, May 5, 2025 at 8:58 PM Gregory CLEMENT
> <gregory.clement@bootlin.com> wrote:
>>
>> When CONFIG_HOTPLUG_PARALLEL is enabled, the code executing before
>> cpuhp_ap_sync_alive() is executed in parallel, while after it is
>> serialized. The functions set_cpu_sibling_map() and set_cpu_core_map()
>> were not designed to be executed in parallel, so by moving the
>> cpuhp_ap_sync_alive() before cpuhp_ap_sync_alive(), we then ensure
>> they will be called serialized.
> set_cpu_sibling_map() and set_cpu_core_map() are the most obvious
> functions that need serialization, but you'd better check other places
> to make sure there are no similar functions executed in parallel.

I conducted an exhaustive review of all called functions, which allowed
me to identify an opportunity for improving boot time:
[1]. Additionally, I noted that CPU delay calibration is the only
remaining part where concurrent access could occur. This was indeed the
case prior to my previous patch submission[1]. To ensure safety, I am
about sending a new fix addressing this issue.

Gregory

[1]: https://lore.kernel.org/linux-mips/20250520-smp_calib-v1-1-cd04f0a78648@bootlin.com/
>
> Huacai
>
>>
>> The measurement done on EyeQ5 did not show any relevant boot time
>> increase after applying this patch.
>>
>> Reported-by: Huacai Chen <chenhuacai@kernel.org>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> ---
>> Hello,
>>
>> As discussed last week [1], this is the patch that fixes the potential
>> issue with the functions set_cpu_sibling_map() and set_cpu_core_map().
>>
>> Gregory
>>
>> [1]: https://lore.kernel.org/all/aBVBHFGH2kICjnT3@alpha.franken.de/
>> ---
>>  arch/mips/kernel/smp.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
>> index 1726744f2b2ec10a44420a7b9b9cd04f06c4d2f6..7901b59d8f60eddefc020cf2a137716af963f09e 100644
>> --- a/arch/mips/kernel/smp.c
>> +++ b/arch/mips/kernel/smp.c
>> @@ -374,13 +374,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
>>
>> ---
>> base-commit: 3b3704261e851e25983860e4c352f1f73786f4ab
>> change-id: 20250505-hotplug-paralell-fix-25224cd229c6
>>
>> 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