| Message ID | 1392205594-29186-1-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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).
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.
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
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.
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.