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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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