Re: [Xen-devel] [PATCH 2/4] xen/x86: Drop erronious barriers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 6 Dec 2016, Stefano Stabellini wrote:
> On Tue, 6 Dec 2016, Andrew Cooper wrote:
> > On 05/12/2016 19:17, Stefano Stabellini wrote:
> > > On Mon, 5 Dec 2016, Andrew Cooper wrote:
> > >> None of these barriers serve any purpose, as they are not synchronising 
> > >> with
> > >> any anything on remote CPUs.
> > >>
> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > >> ---
> > >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> > >> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > >> CC: Julien Grall <julien.grall@xxxxxxx>
> > >>
> > >> Restricting to just the $ARCH maintainers, as this is a project-wide 
> > >> sweep.
> > >>
> > >> Julien/Stefano: I think the ARM smpboot inhereted the erronious barrier 
> > >> usage
> > >> from x86, but I don't know whether further development has gained a 
> > >> dependence
> > >> on them.
> > > We turned them into smp_wmb already (kudos to IanC).
> > 
> > Right, but the entire point I am trying to argue is that they are not
> > needed in the first place.

Just to be clear, on ARM the barriers are unneeded only if it is
unimportant that "init stuff" (which correspond to all the
initialization done in start_secondary up to smp_wmb) below is completed
before "write cpu_online_map". But it looks like we do want to complete
mmu, irq, timer initializations and set the current vcpu before marking
the cpu as online, right?


> This is the current code:
> 
>     CPU 1                                  CPU 0
>     -----                                  -----
> 
>     init stuff                             read cpu_online_map
> 
>     write barrier                          
> 
>     write cpu_online_map                   do more initialization
> 
>     write barrier
> 
>     init more stuff
> 
> 
> I agree that it's wrong, because the second write barrier in
> start_secondary is useless and in addition we are missing a read barrier
> in __cpu_up, corresponding to the first write barrier in
> start_secondary.
> 
> I think it should look like:
> 
> 
>     CPU 1                                  CPU 0
>     -----                                  -----
> 
>     init stuff                             read cpu_online_map
> 
>     write barrier                          read barrier 
> 
>     write cpu_online_map                   do more initialization
> 
>     init more stuff
> 
> 
> The patch is as follow.
> 
> Julien, what do you think?
> 
> Also, do we need to change the remaming smp_wmb() in start_secondary to
> wmb() to ensure execution ordering as well as memory access ordering?
> 
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 90ad1d0..c841a15 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -311,7 +311,6 @@ void start_secondary(unsigned long boot_phys_offset,
>  
>      /* Now report this CPU is up */
>      cpumask_set_cpu(cpuid, &cpu_online_map);
> -    smp_wmb();
>  
>      local_irq_enable();
>      local_abort_enable();
> @@ -408,6 +407,7 @@ int __cpu_up(unsigned int cpu)
>          cpu_relax();
>          process_pending_softirqs();
>      }
> +    smp_rmb();
>  
>      /*
>       * Nuke start of day info before checking one last time if the CPU
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel