[PATCH] arm64: smp: Add a memory barrier before we start secondary cores

[PATCH] arm64: smp: Add a memory barrier before we start secondary cores

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

Ensure that memory writes will be visible to the newly started core by
inserting a write barrier prior to starting. This adds robustness against
possible incomplete synchronisation of state.

Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@linaro.org>
---
 arch/arm64/kernel/smp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 9660750f34ba..f5520d8fafab 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -160,6 +160,11 @@ asmlinkage void secondary_start_kernel(void)
 	smp_store_cpu_info(cpu);
 
 	/*
+	 * Ensure memory writes are visible before we start.
+	 */
+	smp_wmb();
+
+	/*
 	 * OK, now it's safe to let the boot CPU continue.  Wait for
 	 * the CPU migration code to notice that the CPU is online
 	 * before we continue.
-- 
1.9.0.rc3

[PATCH] arm64: smp: Add a memory barrier before we start secondary cores

[Will Deacon]

On Wed, Feb 12, 2014 at 11:46:34AM +0000, Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
> 
> Ensure that memory writes will be visible to the newly started core by
> inserting a write barrier prior to starting. This adds robustness against
> possible incomplete synchronisation of state.

This is very vague. I still don't understand what this is being used for.

> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Mark Brown <broonie@linaro.org>
> ---
>  arch/arm64/kernel/smp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 9660750f34ba..f5520d8fafab 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -160,6 +160,11 @@ asmlinkage void secondary_start_kernel(void)
>  	smp_store_cpu_info(cpu);
>  
>  	/*
> +	 * Ensure memory writes are visible before we start.
> +	 */
> +	smp_wmb();
> +
> +	/*
>  	 * OK, now it's safe to let the boot CPU continue.  Wait for
>  	 * the CPU migration code to notice that the CPU is online
>  	 * before we continue.

So right after this comment we complete(&cpu_running), which implies a write
barrier if we wake somebody up. The secondary coming up should be waiting on
that completion, so *that* is how synchronisation is achieved.

Without a concrete example of precisely why this is required, I'm not at all
happy taking patches adding random memory barriers.

Will

[PATCH] arm64: smp: Add a memory barrier before we start secondary cores

[Mark Brown]

On Wed, Feb 12, 2014 at 12:27:28PM +0000, Will Deacon wrote:
> On Wed, Feb 12, 2014 at 11:46:34AM +0000, Mark Brown wrote:

> > Ensure that memory writes will be visible to the newly started core by
> > inserting a write barrier prior to starting. This adds robustness against
> > possible incomplete synchronisation of state.

> This is very vague. I still don't understand what this is being used for.

Me either to be honest, I wasn't entirely sure why Catalin had suggested
it.

> Without a concrete example of precisely why this is required, I'm not at all
> happy taking patches adding random memory barriers.

That's fine by me, I've no attachment to this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/12b5ba9a/attachment.sig>

[PATCH] arm64: smp: Add a memory barrier before we start secondary cores

[Catalin Marinas]

On Wed, Feb 12, 2014 at 12:47:17PM +0000, Mark Brown wrote:
> On Wed, Feb 12, 2014 at 12:27:28PM +0000, Will Deacon wrote:
> > On Wed, Feb 12, 2014 at 11:46:34AM +0000, Mark Brown wrote:
> 
> > > Ensure that memory writes will be visible to the newly started core by
> > > inserting a write barrier prior to starting. This adds robustness against
> > > possible incomplete synchronisation of state.
> 
> > This is very vague. I still don't understand what this is being used for.
> 
> Me either to be honest, I wasn't entirely sure why Catalin had suggested
> it.

Just in case some other CPU reads cpu_online mask (which is set before
complete) and also expects the topology to be up to date for that CPU.

> > Without a concrete example of precisely why this is required, I'm not at all
> > happy taking patches adding random memory barriers.
> 
> That's fine by me, I've no attachment to this.

I think we can drop this until Vincent clarifies the synchronisation
requirements (still wondering whether spinlocks are needed).

-- 
Catalin

[PATCH] arm64: smp: Add a memory barrier before we start secondary cores

[Mark Brown]

On Wed, Feb 12, 2014 at 01:43:32PM +0000, Catalin Marinas wrote:

> I think we can drop this until Vincent clarifies the synchronisation
> requirements (still wondering whether spinlocks are needed).

Even if they're not actually required for the topology code the fact
that we're having to think about what is needed suggests that the code
is going to end up being clearer and hence more maintainable going
forwards if we add it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/ce0335a1/attachment.sig>

[PATCH] arm64: smp: Add a memory barrier before we start secondary cores

[Catalin Marinas]

On 12 Feb 2014, at 14:56, Mark Brown <broonie@kernel.org> wrote:

> On Wed, Feb 12, 2014 at 01:43:32PM +0000, Catalin Marinas wrote:
> 
>> I think we can drop this until Vincent clarifies the synchronisation
>> requirements (still wondering whether spinlocks are needed).
> 
> Even if they're not actually required for the topology code the fact
> that we're having to think about what is needed suggests that the code
> is going to end up being clearer and hence more maintainable going
> forwards if we add it.

I think we should aim to understand the code better rather than just
adding the spinlocks. Some simple questions:

- Who uses the topology information and when? (the scheduler? first time after or
before secondaries are started?)

- Who updates the topology information? (primary and secondary startup
code?)

- What about hotplug?

- Can any of the above happen in parallel on different CPUs?

Catalin

[PATCH] arm64: smp: Add a memory barrier before we start secondary cores

[Mark Brown]

On Wed, Feb 12, 2014 at 06:23:14PM +0000, Catalin Marinas wrote:

> I think we should aim to understand the code better rather than just
> adding the spinlocks. Some simple questions:

I agree that understanding the code is good, my point is that if the
code needs serious thinking about to understand then it's probably not
good code even if it is correct given that it's not super performance
critical.

> - Who uses the topology information and when? (the scheduler? first time after or
> before secondaries are started?)

It's used directly by the architecture topology code only (to generate
information read by the scheduler).  The scheduler appears to deal with
its own concurrency requirements and it appears to use information it
gets immediately.

> - Who updates the topology information? (primary and secondary startup
> code?)

Each CPU that comes on line.

> - What about hotplug?

Removal currently doesn't do anything to these data structures (nor is
it expected to).  Starting the CPU will redo the startup code which
should produce the same result again (anything dynamic would need work
to support dynamic topology information anyway).

> - Can any of the above happen in parallel on different CPUs?

Everything looks serialised, though I may be missing some cases or have
lost something in an indirection.

A lack of locking also seems reasonably consistent with other
architectures from a quick survey.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140213/07be9ae2/attachment.sig>