[PATCH 1/4] arm64: topology: Implement basic CPU topology support

[Catalin Marinas]

On Wed, Feb 12, 2014 at 08:04:54AM +0000, Vincent Guittot wrote:
> On 11 February 2014 15:07, Catalin Marinas <catalin.marinas at arm.com> wrote:
> > On Tue, Feb 11, 2014 at 01:18:56PM +0000, Vincent Guittot wrote:
> >> On 11 February 2014 11:34, Will Deacon <will.deacon at arm.com> wrote:
> >> > On Tue, Feb 11, 2014 at 08:15:19AM +0000, Vincent Guittot wrote:
> >> >> On 10 February 2014 17:46, Mark Brown <broonie at kernel.org> wrote:
> >> >> > On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
> >> >> >> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
> >> >> >
> >> >> >> > +           if (cpu != cpuid)
> >> >> >> > +                   cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
> >> >> >> > +   }
> >> >> >> > +   smp_wmb();
> >> >> >
> >> >> >> I now noticed there are a couple of smp_wmb() calls in this patch. What
> >> >> >> are they for?
> >> >> >
> >> >> > To be honest I mostly cargo culted them from the ARM implementation; I
> >> >> > did look a bit but didn't fully dig into it - it seemed they were
> >> >> > required to ensure that the updates for the new CPU are visible over all
> >> >> > CPUs.  Vincent?
> >> >>
> >> >> Yes that's it. we must ensure that updates are made visible to other CPUs
> >> >
> >> > In relation to what? The smp_* barriers ensure ordering of observability
> >> > between a number of independent accesses, so you must be ensuring
> >> > ordering against something else. Also, you need to guarantee ordering on the
> >> > read-side too -- how is this achieved? I can't see any smp_rmb calls from a
> >> > quick grep, so I assume you're making use of address dependencies?
> >>
> >> The boot sequence ensures the rmb
> >
> > As Will said, smp_*mb() do not ensure absolute visibility, only relative
> > to subsequent memory accesses on the same processor. So just placing a
> 
> It's my time to be a bit confused, if smp_*mb() do not ensure absolute
> visibility on other CPUs, how can we ensure that ?

smb_wmb()/smb_rmb() do not provide any waiting, they are not
synchronisation primitives. You have to use spinlocks or some other
polling (and of course, barriers for relative ordering of memory
reads/writes).

> > barrier at the end of a function does not mean much, it only shows half
> > of the problem it is trying to solve.
> >
> > How are the secondary CPUs using this information? AFAICT, secondaries
> > call smp_store_cpu_info() which also go through each CPU in
> > update_siblings_mask(). Is there any race here that smp_wmb() is trying
> > to solve?
> 
> The fields will be used to construct topology so we must ensure their
> visibility

I wonder whether you need spinlocks around the topology updating code.

-- 
Catalin